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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32796.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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)
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
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.
Consider this:
--- i/test/functional/wallet_keypool.py
+++ w/test/functional/wallet_keypool.py
@@ -124,16 +124,13 @@ class KeyPoolTest(BitcoinTestFramework):
# refill keypool with three new addresses
nodes[0].walletpassphrase('test', 1)
nodes[0].keypoolrefill(3)
# test walletpassphrase timeout
- # 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)
+ nodes[0].wait_until(lambda: nodes[0].getwalletinfo()["unlocked_until"] == 0)
assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
# drain the keypool
for _ in range(3):
nodes[0].getnewaddress()
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.
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.
lgtm ACK 7bc20b1a50a4f52693fa7fd2108a2cc402d03e54
Could start title with rpc:?
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))
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
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)
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?
lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱
<details><summary>Show signature</summary>
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==
</details>
re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱
<details><summary>Show signature</summary>
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==
</details>
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
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
27 | @@ -26,6 +28,8 @@ 28 | #include <string> 29 | #include <vector> 30 | 31 | +
nit: new blank line seems unnecessary
👍
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))}
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
Concept ACK
ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675
Would be nice to take #32796 (review)
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)
This removes the dependency on libevent for scheduled events,
like re-locking a wallet some time after decryption.
Code review ACK d06942c6731d5db7326bc565655b33a379a5d9b0
ACK d06942c6731d5db7326bc565655b33a379a5d9b0
re ACK d06942c6731d5db7326bc565655b33a379a5d9b0
Changes sinds last ACK:
Included suggestions (thanks) to change:
std::any abstraction in the HTTPRPCTimerInterface
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?
@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?
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.
walletpassphraseis currently the only RPC that schedules a new event, so all this PR will add to the queue are calls toCWallet::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.
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...
@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.
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.