assumeutxo: Fix -reindex before snapshot was validated #29726

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202403_assumeutxo_reindex_fix changing 2 files +13 −4

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).

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

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.
BrandonOdiwuor commented at 12:54 PM on March 27, 2024: contributor

Concept ACK
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).
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.
DrahtBot requested review from BrandonOdiwuor on Mar 27, 2024
BrandonOdiwuor commented at 6:03 PM on March 27, 2024: contributor

crACK cfdef5c5d7a2d2e0bc78aa2cde534e541af61d1c
test: add coverage for -reindex and assumeutxo
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
b7ba60f81a
mzumsande force-pushed on Mar 28, 2024
mzumsande commented at 5:26 PM on March 28, 2024: contributor

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

re-ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be
DrahtBot requested review from BrandonOdiwuor on Mar 29, 2024
BrandonOdiwuor approved
BrandonOdiwuor commented at 3:21 PM on April 6, 2024: contributor

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

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

Tested ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be

Test result before the fix <img width="837" alt="Screenshot 2024-04-12 at 17 34 14" src="https://github.com/bitcoin/bitcoin/assets/19962379/1449dd8e-dd6e-466e-b34c-b49d6a04c343">

Test result after the fix <img width="856" alt="Screenshot 2024-04-12 at 17 21 48" src="https://github.com/bitcoin/bitcoin/assets/19962379/fe76c207-1b76-4cb9-91f7-8d3626e029de">
ryanofsky merged this on Apr 16, 2024
ryanofsky closed this on Apr 16, 2024
mzumsande deleted the branch on Apr 16, 2024
luke-jr referenced this in commit 72b6daf880 on Apr 24, 2024
luke-jr referenced this in commit 6378aa208f on Apr 24, 2024
luke-jr referenced this in commit 6ec27a6673 on Jun 13, 2024
luke-jr referenced this in commit acf242b062 on Jun 13, 2024
Fabcien referenced this in commit 0631de3410 on Apr 7, 2025
bitcoin locked this on Apr 16, 2025

Type	Reviewers
ACK	fjahr, BrandonOdiwuor, hernanmarino, byaye

Contributors

Linked (view graph)

#29616 AssumeUTXO Mainnet Readiness Tracking

assumeutxo: Fix -reindex before snapshot was validated #29726

Code Coverage

Reviews