Simplify semantics of ChainStateFlushed callback #13106
Conversation
This much more accurately captures the meaning of the callback. -BEGIN VERIFY SCRIPT- sed -i 's/SetBestChain/ChainStateFlushed/g' src/validationinterface.h src/validationinterface.cpp src/wallet/wallet.h src/wallet/wallet.cpp src/validation.cpp src/index/txindex.h src/index/txindex.cpp -END VERIFY SCRIPT-
Previously, ChainStateFlushed would fire either if a full flush completed (which can happen due to memory limits, forced flush, or on its own DATABASE_WRITE_INTERVAL timer) *or* on a ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is both less clear for clients (as there are no guarantees about a flush having actually happened prior to the call), and reults in extra flushes not clearly intended by the code. We drop the second case, providing a strong guarantee without removing the periodit timer-based flushing.
4be88be to
9cb6cdc
Compare
|
utACK 9cb6cdc I believe the old semantics made sense as long as the chainstate was brought up to date with the index before wallets/node were running. As this is no longer the case we should only emit a signal when the on-disk state is actually fully flushed. |
|
Making the guarantees of this callback concrete and documenting that is a good idea as it makes the code easier to understand and reason about. |
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 9cb6cdc.
New semantics are definitely nicer, but could you update the commit message to say what practical effect of this change will be? According to the discussion in the other issue: #11857 (comment), it seems this change doesn't fix any existing race condition, and since there is a difference between DATABASE_FLUSH_INTERVAL and DATABASE_WRITE_INTERVAL values (24h vs 1h), does this mean in practice an extra day or so of blocks might now get scanned on startup?
|
Chatted about it offline and there should be no additional rescan on startup unless you crash during the partial flushing of the UTXO DB. You always rescan from the last locator written to the UTXO DB tip, but if the UTXO DB tip falls behind where the locator is you'll receive the BlockConnected callbacks during the re-connect anyway. |
| // Update best block in wallet (so we can detect restored wallets). | ||
| GetMainSignals().SetBestChain(chainActive.GetLocator()); | ||
| nLastSetChain = nNow; | ||
| GetMainSignals().ChainStateFlushed(chainActive.GetLocator()); |
There was a problem hiding this comment.
What's the reason not to inline this where full_flush_completed is set to true?
There was a problem hiding this comment.
Less diff/fewer lockorder deps added by adding an ordering from cs_LastBlockFile. Doesn't really matter, though, should be easy to review that as well.
9cb6cdc Simplify semantics of ChainStateFlushed callback (Matt Corallo) 50b6533 scripted-diff: Rename SetBestChain callback ChainStateFlushed (Matt Corallo) Pull request description: Previously, ChainStateFlushed would fire either if a full flush completed (which can happen due to memory limits, forced flush, or on its own DATABASE_WRITE_INTERVAL timer) *or* on a ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is both less clear for clients (as there are no guarantees about a flush having actually happened prior to the call), and reults in extra flushes not clearly intended by the code. We drop the second case, providing a strong guarantee without removing the periodit timer-based flushing. This is a follow-up to discussion in #11857. Tree-SHA512: 22ba3a0954d265d28413dbf87040790ca5b439820ee7bbadab14028295ec190de82ce5cd664426c82e58b706dc84278868026fa8d066702eb6e6962c9ace1f8e
Trivial: s/SetBestChain/ChainStateFlushed in comments after bitcoin#13106
…ments after bitcoin#13106 21f5680 Trivial: s/SetBestChain/ChainStateFlushed in comments after bitcoin#13106 (Russell Yanofsky) Pull request description: Tree-SHA512: 373896dd73c3ba202739433b22320a4b7ea6dc80ef84547b4ed72c7ae0d6746b109c8b1879359c43374d1816fe384cc31b24b87879ddefe993887140c2f0df9c
9cb6cdc Simplify semantics of ChainStateFlushed callback (Matt Corallo) 50b6533 scripted-diff: Rename SetBestChain callback ChainStateFlushed (Matt Corallo) Pull request description: Previously, ChainStateFlushed would fire either if a full flush completed (which can happen due to memory limits, forced flush, or on its own DATABASE_WRITE_INTERVAL timer) *or* on a ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is both less clear for clients (as there are no guarantees about a flush having actually happened prior to the call), and reults in extra flushes not clearly intended by the code. We drop the second case, providing a strong guarantee without removing the periodit timer-based flushing. This is a follow-up to discussion in bitcoin#11857. Tree-SHA512: 22ba3a0954d265d28413dbf87040790ca5b439820ee7bbadab14028295ec190de82ce5cd664426c82e58b706dc84278868026fa8d066702eb6e6962c9ace1f8e
…ments after bitcoin#13106 21f5680 Trivial: s/SetBestChain/ChainStateFlushed in comments after bitcoin#13106 (Russell Yanofsky) Pull request description: Tree-SHA512: 373896dd73c3ba202739433b22320a4b7ea6dc80ef84547b4ed72c7ae0d6746b109c8b1879359c43374d1816fe384cc31b24b87879ddefe993887140c2f0df9c
Previously, ChainStateFlushed would fire either if a full flush
completed (which can happen due to memory limits, forced flush, or
on its own DATABASE_WRITE_INTERVAL timer) or on a
ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is
both less clear for clients (as there are no guarantees about a
flush having actually happened prior to the call), and reults in
extra flushes not clearly intended by the code. We drop the second
case, providing a strong guarantee without removing the periodit
timer-based flushing.
This is a follow-up to discussion in #11857.