Problem: The mempool estimator added in #34075 persists mined-block stats for restart. Shutdown saves and unregisters the fee estimator before draining queued validation callbacks. A queued block update can leave the saved stats behind the active chain tip, so the mempool estimator ignores its file on restart.
Fix: Save and unregister the fee estimator after the existing callback drain, which follows the first chainstate flush.
<!--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/36322.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt, ismaelsadeeq, davidgumberg, maflcko, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--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-->
391 | @@ -393,12 +392,6 @@ void Shutdown(NodeContext& node) 392 | DestroyAllBlockFilterIndexes(); 393 | node.indexes.clear(); // all instances are nullptr now 394 | 395 | - // Any future callbacks will be dropped. This should absolutely be safe - if
8f480eab9b5640cc87c2c6ea765f8842c1c131c0: Any reason why this comment is removed? The callbacks from ForceFlushStateToDisk will still be dropped?
doesn’t need responsible security disclosure.
Removed from the PR description.
The callbacks from ForceFlushStateToDisk will still be dropped?
I generally dislike code comments because they can rot quickly, but I over-corrected here.
391 | @@ -393,12 +392,6 @@ void Shutdown(NodeContext& node) 392 | DestroyAllBlockFilterIndexes(); 393 | node.indexes.clear(); // all instances are nullptr now 394 | 395 | - // Any future callbacks will be dropped. This should absolutely be safe - if 396 | - // missing a callback results in an unrecoverable situation, unclean shutdown 397 | - // would too. The only reason to do the above flushes is to let the wallet catch
8f480eab9b5640cc87c2c6ea765f8842c1c131c0: I guess this wallet comment is stale since 2581258ec200efb173ea6449ad09b2e7f1cc02e0, which dropped the flush for the wallet?
Thanks, I added a hint to the fix commit message
Of course this raises the question if https://github.com/bitcoin/bitcoin/commit/2581258ec200efb173ea6449ad09b2e7f1cc02e0 was safe to do, or if it re-introduced any "strange pruning edge cases"? But I guess this is unrelated and may have been fixed in the meantime?
lgtm ACK 8f480eab9b5640cc87c2c6ea765f8842c1c131c0
disclosed responsibly
I think this is just a small local shutdown bug, which doesn't need responsible security disclosure.
375 | @@ -385,6 +376,14 @@ void Shutdown(NodeContext& node) 376 | // CValidationInterface callbacks, flush them... 377 | if (node.validation_signals) node.validation_signals->FlushBackgroundCallbacks(); 378 | 379 | + // Save fee estimates after draining the current validation callback queue
In "init: drain callbacks before saving fee estimates" 8f480eab9b5640cc87c2c6ea765f8842c1c131c0
Why replace this comment? The previous one seems to be more accurate?
// Drop transactions we were still watching, record fee estimations and unregister
// fee estimator from validation interface.
The title is misleading
Adjusted as suggested.
Why replace this comment? The previous one seems to be more accurate?
I dislike comments that explain what the code does, since they can go stale and may be a sign that the code itself is hard to follow. I restored the shutdown actions and noted that they follow the callback drain. Let me know what you think.
Yeah, comments generally do become stale, even this one is not wholly accurate. We are not just recording estimates.
Just want to note that we are changing this, and it should be indicated with a rationale in the commit message if its intentional.
ACK 8f480eab9b5640cc87c2c6ea765f8842c1c131c0
The title is misleading, "init: fee estimates can lag the chain after shutdown" reads as "fee estimates cause the chain to lag." It should be "init: fee estimates can lag behind the chain after restart"
Shutdown saves and unregisters the fee estimator before draining queued validation callbacks, so a pending block update can be missing from the saved estimates.
The mempool estimator also ignores its saved file on restart when the saved tip no longer matches the active tip.
Queue a block update after the scheduler stops and check that the block-policy height in the saved estimates does not reflect it.
Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>
Save and unregister the fee estimator after the existing callback drain so queued block updates are reflected in the saved estimates.
The estimator comment now says its shutdown actions follow the callback drain.
The old paragraph below the indexes claimed the drain only let wallets catch up, although wallets are already unloaded.
The new comment beside the final chainstate flush keeps the accurate point that callbacks queued there can be dropped.
ACK 8c27c38526419de2eaca1d4857df2172ad1da1ab
ACK 8c27c38526419de2eaca1d4857df2172ad1da1ab
We can extend the current test beyond the current minimal one to demonstrate the effect of this bug, which depends on the last 5 blocks and the effect the missed block notification have to them, i.e indicating the healthiness of the mempool or not, but not a blocker for me.
74 | + 75 | + AutoFile estimates_file{fsbridge::fopen(BlockPolicyFeeEstPath(*m_node.args), "rb")}; 76 | + int version; 77 | + unsigned int best_seen_height; 78 | + estimates_file >> version >> best_seen_height; 79 | + BOOST_CHECK_NE(best_seen_height, queued_height); // TODO: The saved height must include the queued block update
https://github.com/bitcoin/bitcoin/pull/36322/changes/8c27c38526419de2eaca1d4857df2172ad1da1ab (test: characterize fee callback loss at shutdown)
yocto-nit since it's an ephemeral line:
BOOST_CHECK_LT(best_seen_height, queued_height); // TODO: The saved height must include the queued block update
396 | - // missing a callback results in an unrecoverable situation, unclean shutdown 397 | - // would too. The only reason to do the above flushes is to let the wallet catch 398 | - // up with our current chain to avoid any strange pruning edge cases and make 399 | - // next startup faster by avoiding rescan. 400 | - 401 | + // Callbacks queued by this final chainstate flush will not run during shutdown
This is not strictly true, but fine as-is:
// Callbacks queued by this final chainstate flush are unlikely to run during shutdown
Why is it not strictly true? The scheduler thread is dead and no other thread will pick them up?
review ACK 8c27c38526419de2eaca1d4857df2172ad1da1ab 🚸
<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: review ACK 8c27c38526419de2eaca1d4857df2172ad1da1ab 🚸
0mNqCtYJVje1xiG66kN+FDUtuGeXY64Sy/O3QM+aHAqAheZj4zf9te1HJ6Rhcyto63oAL5AgFzHRNmGDGqe+DA==
</details>
ACK 8c27c38526419de2eaca1d4857df2172ad1da1ab
This is a recurring bug. At some points it would be nice to ensure programmatically that callbacks are completed before unregistering their consumers.
ACK 8c27c38
This is a recurring bug. At some points it would be nice to ensure programmatically that callbacks are completed before unregistering their consumers.
Indeed, another fragile thing about the validation signals is that it also receives and accesses bare pointers, hence, shutdown has to be in order, so a follow-up should prevent this as well.
