From 2c3e6154d36bb8a39019267512f14eadb77a114a Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 12 Aug 2026 15:48:21 -0500 Subject: [PATCH] fix: remove every snapshot lifecycle directory on explicit reindex A reindex wipes EvoDB, so the snapshot lifecycle markers disappear while the directories they describe stay behind. Develop already deleted chainstate_snapshot after chainstate initialization (DeleteSnapshotChainstateFromDisk), but chainstate_snapshot_INVALID and chainstate_todelete survived because the recovery pass that knows about them is deliberately skipped on reindex, and the partial deletion ran only after the new EvoDB was constructed. Remove all three directories durably at the top of LoadChainstate before EvoDB is recreated, deriving their names from the shared suffix constants, and drop the now-redundant DeleteSnapshotChainstateFromDisk free function (this was its only caller). feature_reindex.py now seeds all three artifacts and asserts both reindex flavors remove them. Co-Authored-By: Claude Fable 5 --- src/node/chainstate.cpp | 45 ++++++++++++++++++++++++------ src/validation.cpp | 13 --------- src/validation.h | 10 ------- test/functional/feature_reindex.py | 11 ++++++++ 4 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 87c91b2dcfa6..054261c8567f 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -37,6 +37,35 @@ #include namespace node { +static bool RemoveSnapshotChainstateArtifacts(const fs::path& data_dir, bilingual_str& error) +{ + // Explicit reindexing discards both coins databases and EvoDB, so remove + // every snapshot lifecycle directory in the same stroke. A directory must + // not outlive the markers that describe it: a chainstate_snapshot dir whose + // EvoDB markers were just wiped can no longer be revived by + // ActivateExistingSnapshot(), and a reindex is also the user's request to + // discard the _INVALID forensics directory and any interrupted-swap + // remnant, neither of which the (skipped) recovery pass will see. + const fs::path normal{data_dir / "chainstate"}; + fs::path snapshot{normal}; + snapshot += SNAPSHOT_CHAINSTATE_SUFFIX; + fs::path invalid{snapshot}; + invalid += SNAPSHOT_INVALID_SUFFIX; + fs::path to_delete{normal}; + to_delete += SNAPSHOT_TODELETE_SUFFIX; + for (const auto& path : {snapshot, invalid, to_delete}) { + if (!fs::exists(path)) continue; + try { + RemoveAllDurably(path); + } catch (const fs::filesystem_error& e) { + error = strprintf(_("Failed to remove snapshot chainstate artifact %s for reindex: %s"), + fs::PathToString(path), e.what()); + return false; + } + } + return true; +} + static bool RecoverSnapshotCleanup(CEvoDB& evodb, const fs::path& data_dir, bilingual_str& error) { const fs::path normal{data_dir / "chainstate"}; @@ -326,6 +355,13 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize LOCK(cs_main); + if (options.reindex || options.reindex_chainstate) { + bilingual_str cleanup_error; + if (!RemoveSnapshotChainstateArtifacts(options.data_dir, cleanup_error)) { + return {ChainstateLoadStatus::FAILURE, cleanup_error}; + } + } + evodb.reset(); // TODO: pass DbWrapperParams as options instead multiple params evodb = std::make_unique(util::DbWrapperParams{ @@ -344,15 +380,6 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // Load the fully validated chainstate. chainman.InitializeChainstate(options.mempool, *evodb, chain_helper); - // Wiping the shared EvoDB above erased the SNAPSHOT best-block marker that - // ActivateExistingSnapshot() requires, so a persisted snapshot chainstate can - // no longer be revived. Discard it here rather than letting startup fail with - // advice ("reindex") the user has just followed, which would never recover. - if ((options.reindex || options.reindex_chainstate) && !DeleteSnapshotChainstateFromDisk()) { - return {ChainstateLoadStatus::FAILURE, - _("Failed to remove the snapshot chainstate directory. Remove it manually before restarting.")}; - } - // Load a chain created from a UTXO snapshot, if any exist. bilingual_str snapshot_error; if (!chainman.DetectSnapshotChainstate(options.mempool, snapshot_error)) { diff --git a/src/validation.cpp b/src/validation.cpp index 2d5f23772751..1e5142023b90 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5570,19 +5570,6 @@ static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot) return destroyed && !fs::exists(db_path); } -bool DeleteSnapshotChainstateFromDisk() -{ - AssertLockHeld(::cs_main); - - auto snapshot_datadir = node::FindSnapshotChainstateDir(); - if (!snapshot_datadir) { - return true; - } - LogPrintf("[snapshot] discarding persisted snapshot chainstate at %s\n", - fs::PathToString(*snapshot_datadir)); - return DeleteCoinsDBFromDisk(*snapshot_datadir, /*is_snapshot=*/true); -} - bool ChainstateManager::ActivateSnapshot( AutoFile& coins_file, const SnapshotMetadata& metadata, diff --git a/src/validation.h b/src/validation.h index c325d8b637f2..2bab90895e41 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1294,16 +1294,6 @@ MnRewardEra GetMnRewardEraAfter(const CBlockIndex* pindexPrev, const ChainstateM */ const AssumeutxoData* ExpectedAssumeutxo(const int height, const CChainParams& params); -/** - * Remove a persisted snapshot chainstate's on-disk artifacts: its coins database - * and the base-blockhash file identifying it. Only valid while no snapshot - * Chainstate object exists, i.e. at startup before DetectSnapshotChainstate(). - * - * @returns false only if a snapshot chainstate was found but could not be fully - * removed; true when there was nothing to remove. - */ -bool DeleteSnapshotChainstateFromDisk() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** Identifies blocks that overwrote an existing coinbase output in the UTXO set (see BIP30) */ bool IsBIP30Repeat(const CBlockIndex& block_index); diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py index 0a12c216adbc..6b74a99e3595 100755 --- a/test/functional/feature_reindex.py +++ b/test/functional/feature_reindex.py @@ -11,6 +11,7 @@ """ import os +from pathlib import Path from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import MAGIC_BYTES from test_framework.util import assert_equal @@ -25,9 +26,19 @@ def reindex(self, justchainstate=False, txindex=0): self.generatetoaddress(self.nodes[0], 3, self.nodes[0].get_deterministic_priv_key().address) blockcount = self.nodes[0].getblockcount() self.stop_nodes() + chain_dir = Path(self.nodes[0].datadir) / self.nodes[0].chain + snapshot_artifacts = [ + chain_dir / "chainstate_snapshot", + chain_dir / "chainstate_snapshot_INVALID", + chain_dir / "chainstate_todelete", + ] + for artifact in snapshot_artifacts: + artifact.mkdir() + (artifact / "stale").touch() extra_args = [["-reindex-chainstate", "-txindex=0"]] if justchainstate else [["-reindex", f"-txindex={txindex}"]] self.start_nodes(extra_args) assert_equal(self.nodes[0].getblockcount(), blockcount) # start_node is blocking on reindex + assert all(not artifact.exists() for artifact in snapshot_artifacts) self.log.info("Success") # Check that blocks can be processed out of order