assumeutxo: Fix -reindex before snapshot was validated #29726
Conversation
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
tACK cfdef5c
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.
| @@ -278,6 +275,18 @@ def check_tx_counts(final: bool) -> None: | |||
|
|
|||
| assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT) | |||
|
|
|||
| self.log.info(f"Check that restarting with -reindex will delete the snapshot chainstate") | |||
| self.restart_node(1, extra_args=['-reindex=1', *self.extra_args[1]]) | |||
There was a problem hiding this comment.
This only covers -reindex but not -reindex-chainstate. It's easily possible when using node 2 for the test instead which is not pruned: fjahr@38c5d80
There was a problem hiding this comment.
Thanks! I took your suggestion (with a minor change to logging).
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
mzumsande
force-pushed
the
202403_assumeutxo_reindex_fix
branch
from
cfdef5c
to
b7ba60f
Compare
|
re-ACK b7ba60f |
|
crACK b7ba60f . Good fix |
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.
Github-Pull: bitcoin#29726
Rebased-From: e57f951
Co-authored-by: Fabian Jahr <fjahr@protonmail.com> Github-Pull: bitcoin#29726 Rebased-From: b7ba60f
In c711ca1 logic was introduced that
-reindexand-reindex-chainstatewill 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
-reindexhas deleted the snapshot (also covered by the test).