rpc: use CScheduler for HTTPRPCTimer

pinheadmz commented at 11:37 pm on June 22, 2025: member

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 commented at 11:37 pm on June 22, 2025: contributor

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	fjahr, vasild, janb84, furszy
Stale ACK	maflcko

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.

in src/httprpc.cpp:45 in 7bc20b1a50 outdated

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

maflcko commented at 6:36 am on June 23, 2025:

0    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

pinheadmz commented at 6:12 pm on June 23, 2025:

great comment thanks, taken

in test/functional/wallet_keypool.py:132 in 7bc20b1a50 outdated

126@@ -127,7 +127,10 @@ def run_test(self):
127         nodes[0].keypoolrefill(3)
128 
129         # test walletpassphrase timeout
130-        time.sleep(1.1)
131+        # CScheduler relies on condition_variable::wait_until() which is slightly
132+        # less accurate than libevent's event trigger. We'll give it 2
133+        # seconds to execute a 1 second scheduled event.

maflcko commented at 6:38 am on June 23, 2025:

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.

vasild commented at 7:19 am on June 25, 2025:

Consider this:

 0--- i/test/functional/wallet_keypool.py
 1+++ w/test/functional/wallet_keypool.py
 2@@ -124,16 +124,13 @@ class KeyPoolTest(BitcoinTestFramework):
 3
 4         # refill keypool with three new addresses
 5         nodes[0].walletpassphrase('test', 1)
 6         nodes[0].keypoolrefill(3)
 7         
 8         # test walletpassphrase timeout
 9-        # CScheduler relies on condition_variable::wait_until() which is slightly
10-        # less accurate than libevent's event trigger. We'll give it 2
11-        # seconds to execute a 1 second scheduled event.
12-        time.sleep(2)
13+        nodes[0].wait_until(lambda: nodes[0].getwalletinfo()["unlocked_until"] == 0)
14         assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
15         
16         # drain the keypool
17         for _ in range(3):
18             nodes[0].getnewaddress()
19         assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getnewaddress)

It makes the test ~1 second faster - does not wait needlessly when the passphrase has already been cleared after about 1 second in normal environments. Also is more reliable because it would adapt to slow environments.

pinheadmz commented at 5:17 pm on June 25, 2025:

Thanks I’ll take this suggestion. My only concern is that technically there could be a future regression where a user setting timeout to 1 second ends up lasting 60 seconds… I’ll add timeout=5 for a reasonable clamp.

maflcko approved

maflcko commented at 6:39 am on June 23, 2025: member

lgtm ACK 7bc20b1a50a4f52693fa7fd2108a2cc402d03e54

Could start title with rpc:?

pinheadmz renamed this:
~~use CScheduler for HTTPRPCTimer~~
rpc: use CScheduler for HTTPRPCTimer
on Jun 23, 2025

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

pinheadmz force-pushed on Jun 23, 2025

in src/httprpc.cpp:54 in 466722cbe5 outdated

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

maflcko commented at 9:13 am on June 24, 2025:

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

maflcko commented at 9:14 am on June 24, 2025:

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

pinheadmz commented at 3:26 pm on June 24, 2025:

ok taken this, thanks.

new constructor is

0explicit HTTPRPCTimerInterface(const std::any& context)
1      : 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

in test/functional/wallet_keypool.py:133 in 466722cbe5 outdated

126@@ -127,7 +127,10 @@ def run_test(self):
127         nodes[0].keypoolrefill(3)
128 
129         # test walletpassphrase timeout
130-        time.sleep(1.1)
131+        # CScheduler relies on condition_variable::wait_until() which is slightly
132+        # less accurate than libevent's event trigger. We'll give it 2
133+        # seconds to execute a 1 second scheduled event.
134+        time.sleep(2)

maflcko commented at 9:15 am on June 24, 2025:

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

pinheadmz commented at 3:27 pm on June 24, 2025:

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

maflcko commented at 9:15 am on June 24, 2025: member

lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱
3BIFmQ7bHKY+gQInqAOOTQFAXFxXQJHAH2aGv/XMhziokLGtp5GIeKdYYzaDYg9TthqPZmgwLUlYlQk4qGLzXDg==

pinheadmz force-pushed on Jun 24, 2025

maflcko commented at 4:00 pm on June 24, 2025: member

re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱
30Z8+Lq6UK9Umi7X/Sw90DJybA2GqemOEJTj3easwUe4mQHVwlv4o89DbF9488dTerb2ROAWnXTiw5wZG95CsAg==

in test/functional/wallet_keypool.py:131 in e70c2087b0 outdated

126@@ -127,7 +127,10 @@ def run_test(self):
127         nodes[0].keypoolrefill(3)
128 
129         # test walletpassphrase timeout
130-        time.sleep(1.1)
131+        # CScheduler relies on condition_variable::wait_until() which is slightly
132+        # less accurate than libevent's event trigger. We'll give it 2

fjahr commented at 9:41 pm on June 24, 2025:

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.

pinheadmz commented at 5:11 pm on June 25, 2025:

Thanks I’ll remove reference to libevent (thats the whole point anyway!) and explain why there is a margin of error in the test

in src/httprpc.cpp:31 in e70c2087b0 outdated

27@@ -26,6 +28,8 @@
28 #include <string>
29 #include <vector>
30 
31+

fjahr commented at 9:46 pm on June 24, 2025:

nit: new blank line seems unnecessary

pinheadmz commented at 3:50 pm on June 25, 2025:

👍

in src/httprpc.cpp:55 in e70c2087b0 outdated

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

fjahr commented at 9:55 pm on June 24, 2025:

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

pinheadmz commented at 5:09 pm on June 25, 2025:

Good feedback I think you’re right, its cleaner to just store a reference to the scheduler here. It requires some weird pointer / any juggling inStartHTTPRPC() – to keep the std::any abstraction

fjahr commented at 10:00 pm on June 24, 2025: contributor

Concept ACK

vasild approved

vasild commented at 7:20 am on June 25, 2025: contributor

ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675

Would be nice to take #32796 (review)

DrahtBot requested review from fjahr on Jun 25, 2025

janb84 commented at 1:42 pm on June 25, 2025: contributor

ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675

PR makes a step to remove the dependency on libevent, this pr removes the dependency in the wallet re-locking functionality.

(I do agree with NIT comments above and hope to see #32796 (review) included)

code review ✅
ran test ✅

use CScheduler for HTTPRPCTimer

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

d06942c673

pinheadmz force-pushed on Jun 25, 2025

pinheadmz commented at 7:05 pm on June 25, 2025: member

Rebase to include suggestions from @fjahr and @vasild thanks!!

fjahr commented at 8:30 pm on June 25, 2025: contributor

Code review ACK d06942c6731d5db7326bc565655b33a379a5d9b0

DrahtBot requested review from janb84 on Jun 25, 2025

DrahtBot requested review from vasild on Jun 25, 2025

DrahtBot requested review from maflcko on Jun 25, 2025

vasild approved

vasild commented at 7:26 am on June 26, 2025: contributor

ACK d06942c6731d5db7326bc565655b33a379a5d9b0

janb84 commented at 7:45 am on June 26, 2025: contributor

re ACK d06942c6731d5db7326bc565655b33a379a5d9b0

Changes sinds last ACK:

Included suggestions (thanks) to change:

moved away from std::any abstraction in the HTTPRPCTimerInterface
changed testing

furszy commented at 6:16 pm on June 26, 2025: member

Because the scheduler is also used for the validation signals (see here), using it for RPC as well seems likely to delay block processing — we only allow up to 10 queued tasks at a time before pausing block processing (see here). So, in other words, adding 10 long-lived timers through RPC could end up blocking block processing entirely? Haven’t tested it but it seems to be the case. Probably better to use a different scheduler for RPC timers.

Update: Just discussed this offline with @pinheadmz, and it turns out there’s a separate list for validation signal events. So the pending callbacks limit only affects that list, not the underlying scheduler queue size. This makes it less of a problem than I initially thought. That said, the scheduler still runs the worker thread that executes the validation signal events, so adding more tasks to it doesn’t seem ideal. It might be better to use a separate scheduler for RPC. Or maybe in a follow-up?

pinheadmz commented at 2:17 pm on June 27, 2025: member

@furszy I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. walletpassphrase is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to CWallet::Lock() after the user needs a private key (and in the GUI an entirely different timer is used for this, based in Qt!).

We can also run some benchmarks, ie unlock 1000 wallets during IBD and try to evaluate the impact on validation signals. @maflcko do you have any instincts about this?

furszy commented at 2:28 pm on June 27, 2025: member

ACK d06942c6731d5db7326bc565655b33a379a5d9b0

I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. walletpassphrase is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to CWallet::Lock() after the user needs a private key

Sounds fair to me.

achow101 commented at 8:44 pm on June 27, 2025: member

walletpassphrase is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to CWallet::Lock() after the user needs a private key

WalletContext has its own CScheduler as well. I think it would probably make more sense to drop HTTPRPCTimer altogether and have walletpassphrase schedule the lock by itself instead of taking a trip through the interfaces.

Edit: It’s the same CScheduler, but would probably still make for a good cleanup.

pinheadmz commented at 12:43 pm on June 28, 2025: member

drop HTTPRPCTimer altogether

Interesting, I can try that approach… for that matter, could we rip out QtRPCTimerInterface and the base class RPCTimerInterface as well? I don’t think any of it is used outside of walletpassphrase and any future RPCs we add can just use the node/wallet scheduler as well…

rpc: use CScheduler for HTTPRPCTimer #32796

Code Coverage & Benchmarks

Reviews

Conflicts