assumeutxo: Fix -reindex before snapshot was validated #29726

1. 
   mzumsande commented at 7:00 pm on March 25, 2024: contributor

   In c711ca186f8d8a28810be0beedcb615ddcf93163 logic was introduced that -reindex and -reindex-chainstate will delete the snapshot chainstate. This doesn’t work currently, instead of deleting the snapshot chainstate the node crashes with an assert (this can be triggered by applying the added test commit on master). Fix this, and another bug that would prevent the new active chainstate from having a mempool after -reindex has deleted the snapshot (also covered by the test).
2. init, validation: Fix -reindex option with an existing snapshot

   This didn't work for two reasons:
   1.) GetSnapshotCoinsDBPath() was used to retrieve the path.
       This requires coins_views to exist, but the initialisation only happens later
       (in CompleteChainstateInitialization) so the node hits an assert in
       CCoinsViewDB& CoinsDB() and crashes.
   
   2.) The snapshot was already activated, so it has the mempool attached.
       Therefore, the mempool needs to be transferred back to the ibd
       chainstate before deleting the snapshot chainstate.

   e57f951805
3. 
   DrahtBot commented at 7:00 pm on March 25, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	fjahr, BrandonOdiwuor, hernanmarino, byaye

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. 
   BrandonOdiwuor commented at 12:54 pm on March 27, 2024: contributor

   Concept ACK
5. in test/functional/feature_assumeutxo.py:279 in cfdef5c5d7 outdated

   274@@ -278,6 +275,18 @@ def check_tx_counts(final: bool) -> None:
   275 
   276         assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
   277 
   278+        self.log.info(f"Check that restarting with -reindex will delete the snapshot chainstate")
   279+        self.restart_node(1, extra_args=['-reindex=1', *self.extra_args[1]])
   

   ---

   ---

   

   fjahr commented at 5:26 pm on March 27, 2024:

   This only covers -reindex but not -reindex-chainstate. It’s easily possible when using node 2 for the test instead which is not pruned: https://github.com/fjahr/bitcoin/commit/38c5d80f73c7058f17ba8e89a3802ef981080864

   ---

   

   mzumsande commented at 5:25 pm on March 28, 2024:

   Thanks! I took your suggestion (with a minor change to logging).
6. 
   fjahr commented at 5:31 pm on March 27, 2024: contributor

   tACK cfdef5c5d7a2d2e0bc78aa2cde534e541af61d1c

   Good catch! I could reproduce the crash and that the change here fixes this.

   My suggested test extension can be pulled in here if you retouch or I can add it to one of my outstanding assumeutxo PRs as a small follow-up.

7. DrahtBot requested review from BrandonOdiwuor on Mar 27, 2024
8. 
   BrandonOdiwuor commented at 6:03 pm on March 27, 2024: contributor

   crACK cfdef5c5d7a2d2e0bc78aa2cde534e541af61d1c
9. test: add coverage for -reindex and assumeutxo

   Co-authored-by: Fabian Jahr <fjahr@protonmail.com>

   b7ba60f81a
10. mzumsande force-pushed on Mar 28, 2024
11. 
    mzumsande commented at 5:26 pm on March 28, 2024: contributor

    cfdef5c to b7ba60f: Included test coverage for -reindex-chainstate as suggested by @fjahr
12. 
    fjahr commented at 6:47 pm on March 29, 2024: contributor

    re-ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be
13. DrahtBot requested review from BrandonOdiwuor on Mar 29, 2024
14. BrandonOdiwuor approved
15. 
    BrandonOdiwuor commented at 3:21 pm on April 6, 2024: contributor

    re-ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be
16. 
    hernanmarino commented at 8:03 pm on April 9, 2024: contributor

    crACK b7ba60f81a33db876f88b5f9af1e5025d679b5be . Good fix
17. byaye approved
18. 
    byaye commented at 8:37 pm on April 12, 2024: none

    Tested ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be

    Test result before the fix

    Test result after the fix

19. ryanofsky merged this on Apr 16, 2024
20. ryanofsky closed this on Apr 16, 2024

    ---

21. mzumsande deleted the branch on Apr 16, 2024

Contributors
mzumsande DrahtBot BrandonOdiwuor fjahr hernanmarino byaye