Simplify semantics of ChainStateFlushed callback #13106

1. 
   TheBlueMatt commented at 6:13 pm on April 27, 2018: member

   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.

2. scripted-diff: Rename SetBestChain callback ChainStateFlushed

   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-

   50b6533aa2
3. Simplify semantics of ChainStateFlushed callback

   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.

   9cb6cdc59f
4. TheBlueMatt force-pushed on Apr 27, 2018
5. 
   sipa commented at 9:29 pm on April 27, 2018: member

   utACK 9cb6cdc59f9eb826b70ebbb6353a5bcee74996e3

   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.

6. fanquake added the label Refactoring on Apr 27, 2018
7. 
   laanwj commented at 10:12 am on April 30, 2018: member

   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. utACK 9cb6cdc59f9eb826b70ebbb6353a5bcee74996e3
8. 
   ryanofsky commented at 6:41 pm on April 30, 2018: member

   utACK 9cb6cdc59f9eb826b70ebbb6353a5bcee74996e3.

   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?

9. 
   TheBlueMatt commented at 4:34 pm on May 1, 2018: member

   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.
10. in src/validation.cpp:2160 in 9cb6cdc59f

    2158-    if (fDoFullFlush || ((mode == FlushStateMode::ALWAYS || mode == FlushStateMode::PERIODIC) && nNow > nLastSetChain + (int64_t)DATABASE_WRITE_INTERVAL * 1000000)) {
    2159+    if (full_flush_completed) {
    2160         // Update best block in wallet (so we can detect restored wallets).
    2161-        GetMainSignals().SetBestChain(chainActive.GetLocator());
    2162-        nLastSetChain = nNow;
    2163+        GetMainSignals().ChainStateFlushed(chainActive.GetLocator());
    

    ---

    ---

    

    jimpo commented at 10:47 pm on May 1, 2018:

    What’s the reason not to inline this where full_flush_completed is set to true?

    ---

    

    TheBlueMatt commented at 4:48 am on May 2, 2018:

    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.
11. jimpo approved
12. 
    jimpo commented at 10:48 pm on May 1, 2018: contributor

    utACK 9cb6cdc.
13. laanwj merged this on May 2, 2018
14. laanwj closed this on May 2, 2018

    ---

15. laanwj referenced this in commit 598db389c3 on May 2, 2018
16. ryanofsky referenced this in commit 21f5680553 on May 2, 2018
17. laanwj referenced this in commit 11adab39e6 on May 3, 2018
18. stamhe referenced this in commit 3265df4832 on Jun 27, 2018
19. HashUnlimited referenced this in commit 07031436ac on Sep 6, 2018
20. HashUnlimited referenced this in commit 5cae543138 on Sep 6, 2018
21. lionello referenced this in commit a9dc588965 on Nov 7, 2018
22. joemphilips referenced this in commit cfbe0ea263 on Nov 9, 2018
23. UdjinM6 referenced this in commit 6f4ea3388e on May 21, 2021
24. UdjinM6 referenced this in commit 3b3579200d on May 21, 2021
25. TheArbitrator referenced this in commit 06c597c6d7 on Jun 4, 2021
26. UdjinM6 referenced this in commit 531bc38e59 on Jun 5, 2021
27. UdjinM6 referenced this in commit 0d3386bae5 on Jun 5, 2021
28. UdjinM6 referenced this in commit 56bd5aea1b on Jun 5, 2021
29. UdjinM6 referenced this in commit 732697586f on Jun 5, 2021
30. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt sipa laanwj ryanofsky jimpo

Labels
Refactoring