rpc: use CScheduler for HTTPRPCTimer #32796

pinheadmz · 2025-06-22T23:37:22Z

This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase.

This PR is cherry-picked from #32061 and is part of removing libevent from the project

This does result in a slightly less reliable timing compared to libevent as described in #32793. In the test I added a little bit more time for the event to execute. I don't think this a big deal but reviewers lemme know what you think.

DrahtBot · 2025-06-22T23:37:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32796.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko
Concept ACK	fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko

lgtm ACK 7bc20b1

Could start title with rpc:?

maflcko · 2025-06-23T06:36:58Z

src/httprpc.cpp

@@ -38,22 +42,16 @@ static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
 class HTTPRPCTimer : public RPCTimerBase
 {
 public:
-    HTTPRPCTimer(struct event_base* eventBase, std::function<void()>& func, int64_t millis) :
-        ev(eventBase, false, func)
+    HTTPRPCTimer(NodeContext* context, std::function<void()>& func, int64_t millis)


Suggested change

HTTPRPCTimer(NodeContext* context, std::function<void()>& func, int64_t millis)

HTTPRPCTimer(NodeContext& context, std::function<void()>& func, int64_t millis)

nit: When unconditionally and immediately dereferencing, it could make sense to disallow nullptr. You can achieve this, for example, by using *Assert(m_context) in the constructor call below

great comment thanks, taken

maflcko · 2025-06-23T06:38:58Z

test/functional/wallet_keypool.py

+        # CScheduler relies on condition_variable::wait_until() which is slightly
+        # less accurate than libevent's event trigger. We'll give it 2
+        # seconds to execute a 1 second scheduled event.


Seems fine. Apart from the possible cv delay/overhead there could also be leftover (expensive tasks) in the queue. See #18488 and #14289. Though, with the bdb removal (and the bdb flush removal from the scheduler thread), this may be more fine now.

maflcko

lgtm ACK 466722c 🎱

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱
BIFmQ7bHKY+gQInqAOOTQFAXFxXQJHAH2aGv/XMhziokLGtp5GIeKdYYzaDYg9TthqPZmgwLUlYlQk4qGLzXDg==

maflcko · 2025-06-24T09:13:12Z

src/httprpc.cpp

 };

 class HTTPRPCTimerInterface : public RPCTimerInterface
 {
 public:
-    explicit HTTPRPCTimerInterface(struct event_base* _base) : base(_base)
+    explicit HTTPRPCTimerInterface(const std::any& context) : m_context(std::any_cast<NodeContext*>(context))


Suggested change

explicit HTTPRPCTimerInterface(const std::any& context) : m_context(std::any_cast<NodeContext*>(context))

explicit HTTPRPCTimerInterface(const std::any& context)

: m_context{*Assert(std::any_cast<NodeContext*>(context))}

nit: Could probably assert early on construction, rather than when a timer is created, but either is equally fine.

ok taken this, thanks.

new constructor is

explicit HTTPRPCTimerInterface(const std::any& context) : m_context{*Assert(std::any_cast<NodeContext*>(context))}

so its like "passed in an any which i convert to a NodeContext pointer which I assert is not null, then de-reference back to a NodeContext and store that as a reference in the class

maflcko · 2025-06-24T09:15:12Z

test/functional/wallet_keypool.py

+        # CScheduler relies on condition_variable::wait_until() which is slightly
+        # less accurate than libevent's event trigger. We'll give it 2
+        # seconds to execute a 1 second scheduled event.
+        time.sleep(2)


if this turns out problematic (intermittently fail), one could also try mockscheduler.

Good idea, but would that reduce confidence in the test coverage for RPCRunLater() etc?

This removes the dependency on libevent for scheduled events, like re-locking a wallet some time after decryption.

maflcko · 2025-06-24T16:00:00Z

re-ACK e70c208 💱

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱
0Z8+Lq6UK9Umi7X/Sw90DJybA2GqemOEJTj3easwUe4mQHVwlv4o89DbF9488dTerb2ROAWnXTiw5wZG95CsAg==

fjahr

Concept ACK

fjahr · 2025-06-24T21:41:10Z

test/functional/wallet_keypool.py

@@ -127,7 +127,10 @@ def run_test(self):
        nodes[0].keypoolrefill(3)

        # test walletpassphrase timeout
-        time.sleep(1.1)
+        # CScheduler relies on condition_variable::wait_until() which is slightly
+        # less accurate than libevent's event trigger. We'll give it 2


Seems weird to mention libevent here as a point of reference since it's in the past after this change is applied. I would put this in the commit message and just say that it's not accurate enough here.

fjahr · 2025-06-24T21:46:35Z

src/httprpc.cpp

@@ -26,6 +28,8 @@
 #include <string>
 #include <vector>

+


nit: new blank line seems unnecessary

fjahr · 2025-06-24T21:55:18Z

src/httprpc.cpp

 };

 class HTTPRPCTimerInterface : public RPCTimerInterface
 {
 public:
-    explicit HTTPRPCTimerInterface(struct event_base* _base) : base(_base)
+    explicit HTTPRPCTimerInterface(const std::any& context)
+      : m_context{*Assert(std::any_cast<NodeContext*>(context))}


Did you consider avoiding the dependency to node context and just give the Inteface it's own CScheduler instead?

pinheadmz mentioned this pull request Jun 13, 2025

Remove libevent as a dependency (HTTP / cli / torcontrol) #31194

Open

4 tasks

maflcko approved these changes Jun 23, 2025

View reviewed changes

DrahtBot mentioned this pull request Jun 23, 2025

Replace libevent with our own HTTP and socket-handling implementation #32061

Open

pinheadmz changed the title ~~use CScheduler for HTTPRPCTimer~~ rpc: use CScheduler for HTTPRPCTimer Jun 23, 2025

DrahtBot added the RPC/REST/ZMQ label Jun 23, 2025

pinheadmz force-pushed the http-scheduler branch from 7bc20b1 to 466722c Compare June 23, 2025 18:12

maflcko reviewed Jun 24, 2025

View reviewed changes

use CScheduler for HTTPRPCTimer

e70c208

This removes the dependency on libevent for scheduled events, like re-locking a wallet some time after decryption.

pinheadmz force-pushed the http-scheduler branch from 466722c to e70c208 Compare June 24, 2025 15:25

fjahr reviewed Jun 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: use CScheduler for HTTPRPCTimer #32796

rpc: use CScheduler for HTTPRPCTimer #32796

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	HTTPRPCTimer(NodeContext* context, std::function<void()>& func, int64_t millis)
	HTTPRPCTimer(NodeContext& context, std::function<void()>& func, int64_t millis)

	explicit HTTPRPCTimerInterface(const std::any& context) : m_context(std::any_cast<NodeContext*>(context))
	explicit HTTPRPCTimerInterface(const std::any& context)
	: m_context{Assert(std::any_cast<NodeContext>(context))}

rpc: use CScheduler for HTTPRPCTimer #32796

Are you sure you want to change the base?

rpc: use CScheduler for HTTPRPCTimer #32796

Conversation

Uh oh!

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!