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	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.

maflcko

lgtm ACK 7bc20b1

Could start title with rpc:?

src/httprpc.cpp

test/functional/wallet_keypool.py

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==

src/httprpc.cpp

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?

Just for additional reference, the check is already sometimes failing on current master: #30797 (comment)

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.

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

src/httprpc.cpp

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?

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

vasild

ACK e70c208

Would be nice to take #32796 (comment)

janb84

ACK e70c208

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 (comment) included)

code review ✅
ran test ✅

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

pinheadmz

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

src/httprpc.cpp

pinheadmz · 2025-06-25T17:09:31Z

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))}


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

pinheadmz · 2025-06-25T17:11:34Z

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


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

test/functional/wallet_keypool.py

fjahr · 2025-06-25T20:30:37Z

Code review ACK d06942c

vasild

ACK d06942c

janb84

re ACK d06942c

Changes sinds last ACK:

Included suggestions (thanks) to change:

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

furszy

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 · 2025-06-27T14:17:36Z

@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

ACK d06942c

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 · 2025-06-27T20:44:21Z

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 · 2025-06-28T12:43:23Z

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...

pinheadmz · 2025-07-02T14:48:10Z

@achow101 I started working on calling WalletContext.scheduler directly from walletpassphrase() but there is one nice extra feature we get from using RPCRunLater and RPCTimerBase which is that timers are mapped by name and can be removed by name if, for example, the user calls the RPC again with a different timeout value. This might result in a change of behavior where the shorter timer will still lock the wallet, and the longer timer will sit in the queue and eventually do nothing. Current behavior I believe is to only honor the most recent walletpassphrase argument.

achow101 · 2025-07-02T21:44:44Z

which is that timers are mapped by name and can be removed by name if, for example, the user calls the RPC again with a different timeout value.

This behavior would already be changing in this PR. Deleting the HTTPRPCTimer does not de-schedule the task and it will still execute later.

Current behavior I believe is to only honor the most recent walletpassphrase argument.

This behavior would still be maintained even if the task runs multiple times because there is an explicit check in the relock function for whether the current run of that function is actually the most recently scheduled relock.

pinheadmz · 2025-07-02T23:01:34Z

bitcoin/src/wallet/rpc/encrypt.cpp

Lines 103 to 104 in a92e8b1

    
           // Skip if this is not the most recent rpcRunLater callback. 
        
           if (shared_wallet->nRelockTime != relock_time) return;

🤦 yup clear as day thanks

pinheadmz · 2025-07-03T11:06:40Z

Opened #32862 with the suggested approach, closing this in favor of that.

…Timer fcfd3db remove RPCTimerInterface and RPCRunLater (Matthew Zipkin) 8a17657 use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin) Pull request description: This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context. This is an alternative approach to #32796, described in #32796 (comment) ACKs for top commit: fjahr: Code Review ACK fcfd3db achow101: ACK fcfd3db furszy: ACK fcfd3db Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14

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

src/httprpc.cpp Outdated Show resolved Hide resolved

test/functional/wallet_keypool.py Outdated Show resolved Hide resolved

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

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

vasild approved these changes Jun 25, 2025

View reviewed changes

DrahtBot requested a review from fjahr June 25, 2025 07:20

janb84 reviewed Jun 25, 2025

View reviewed changes

use CScheduler for HTTPRPCTimer

d06942c

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 e70c208 to d06942c Compare June 25, 2025 17:26

pinheadmz commented Jun 25, 2025

View reviewed changes

DrahtBot requested review from janb84, vasild and maflcko June 25, 2025 20:30

vasild approved these changes Jun 26, 2025

View reviewed changes

janb84 reviewed Jun 26, 2025

View reviewed changes

furszy reviewed Jun 26, 2025

View reviewed changes

furszy reviewed Jun 27, 2025

View reviewed changes

pinheadmz mentioned this pull request Jul 3, 2025

rpc: use CScheduler for relocking wallet and remove RPCTimer #32862

Merged

pinheadmz closed this Jul 3, 2025

rpc: use CScheduler for HTTPRPCTimer #32796

rpc: use CScheduler for HTTPRPCTimer #32796

Uh oh!

Conversation

pinheadmz commented Jun 22, 2025

Uh oh!

DrahtBot commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

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!

maflcko commented Jun 24, 2025

Uh oh!

fjahr left a comment

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!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

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!

Uh oh!

fjahr commented Jun 25, 2025

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Jun 27, 2025

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented Jun 28, 2025

Uh oh!

pinheadmz commented Jul 2, 2025

Uh oh!

DrahtBot commented Jun 22, 2025 •

edited

Loading

furszy left a comment •

edited

Loading

achow101 commented Jun 27, 2025 •

edited

Loading