validation: discard snapshot state when the snapshot chainstate goes away

Two ways a persisted snapshot chainstate could outlive itself:

PopulateAndValidateSnapshot commits the SNAPSHOT best-block and dual-chainstate markers as its last step. If activation was then abandoned -- WriteSnapshotBaseBlockhash on an unwritable datadir, say -- only the coins directory was removed. The node went back to a single chainstate carrying a permanent dual-chainstate marker, which turns supported legacy missing-diff bootstrapping into unavailable history, and a stale SNAPSHOT marker that a future snapshot directory could satisfy ActivateExistingSnapshot with. Erase both when activation is rolled back.

LoadChainstate constructs CEvoDB with wipe=true for -reindex and -reindex-chainstate, erasing the SNAPSHOT marker while chainstate_snapshot and its base-blockhash file stay on disk. DetectSnapshotChainstate then found the directory and ActivateExistingSnapshot rejected it on the marker just erased, so startup failed with advice ('reindex is required') the user had already followed -- on every retry. Discard the persisted snapshot chainstate's on-disk artifacts alongside the EvoDB wipe. This is the Dash-shaped equivalent of the reindex-time cleanup in bitcoin#27596/bitcoin#29726; it runs before any chainstate has coins views, so no mempool transfer or leveldb-lock dance is needed.

Author: PastaPastaPasta

src/evo/evodb.cpp

@@ -140,3 +140,11 @@ bool CEvoDB::HasDualChainstateMarker()
     LOCK(cs);
     return db->Exists(EVODB_DUAL_CHAINSTATE);
 }
+
+void CEvoDB::EraseSnapshotMarkers()
+{
+    LOCK(cs);
+    auto& transaction = GetContext(GetCurrentIdentity()).cur_transaction;
+    transaction.Erase(std::make_pair(EVODB_BEST_BLOCK, uint8_t{1}));
+    transaction.Erase(EVODB_DUAL_CHAINSTATE);
+}

src/evo/evodb.h

@@ -240,6 +240,12 @@ class CEvoDB
     void WriteBestBlock(EvoDbIdentity identity, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs);
     void WriteDualChainstateMarker() EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool HasDualChainstateMarker() EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    //! Undo WriteBestBlock(SNAPSHOT) and WriteDualChainstateMarker(). Needed
+    //! when snapshot activation is abandoned after those markers were already
+    //! committed: a stale dual-chainstate marker turns supported legacy
+    //! bootstrapping into "unavailable history", and a stale SNAPSHOT marker
+    //! would let a later snapshot directory pass ActivateExistingSnapshot().
+    void EraseSnapshotMarkers() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     bool VerifyBestBlock(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs) { return VerifyBestBlock(EvoDbIdentity::NORMAL, hash); }
     void WriteBestBlock(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs) { WriteBestBlock(EvoDbIdentity::NORMAL, hash); }

src/node/chainstate.cpp

@@ -65,6 +65,15 @@ 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 (to_wipe_data && !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)) {

src/test/evo_db_tests.cpp

@@ -230,4 +230,35 @@ BOOST_AUTO_TEST_CASE(open_transaction_does_not_capture_other_threads)
     BOOST_CHECK(other_thread == PayloadFor(30));
 }
 
+BOOST_AUTO_TEST_CASE(snapshot_markers_can_be_discarded)
+{
+    const fs::path path = m_args.GetDataDirBase() / "evodb_marker_rollback";
+    {
+        CEvoDB db{util::DbWrapperParams{.path = path, .memory = false, .wipe = true}};
+        WriteMarker(db, EvoDbIdentity::SNAPSHOT, BlockHash(40));
+        {
+            auto tx = db.BeginTransaction(EvoDbIdentity::SNAPSHOT);
+            db.WriteDualChainstateMarker();
+            tx->Commit();
+        }
+        BOOST_REQUIRE(db.CommitRootTransaction(EvoDbIdentity::SNAPSHOT));
+        BOOST_REQUIRE(db.HasDualChainstateMarker());
+
+        // Abandoning snapshot activation after the markers were committed must
+        // leave no trace: a stale dual-chainstate marker breaks legacy
+        // bootstrapping and a stale best-block marker would revive a future
+        // snapshot directory.
+        {
+            auto tx = db.BeginTransaction(EvoDbIdentity::SNAPSHOT);
+            db.EraseSnapshotMarkers();
+            tx->Commit();
+        }
+        BOOST_REQUIRE(db.CommitRootTransaction(EvoDbIdentity::SNAPSHOT));
+    }
+    CEvoDB reopened{util::DbWrapperParams{.path = path, .memory = false, .wipe = false}};
+    uint256 hash;
+    BOOST_CHECK(!reopened.ReadBestBlock(EvoDbIdentity::SNAPSHOT, hash));
+    BOOST_CHECK(!reopened.HasDualChainstateMarker());
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/validation_chainstatemanager_tests.cpp

@@ -833,4 +833,26 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init_missing_evodb_marker, Sn
     this->LoadVerifyActivateChainstate();
 }
 
+//! Reindexing wipes the shared EvoDB, taking the SNAPSHOT best-block marker with
+//! it. Startup must discard the persisted snapshot chainstate too, or every
+//! subsequent start would fail on the erased marker and tell the user to reindex.
+BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_discarded_on_reindex, SnapshotTestSetup)
+{
+    this->SetupSnapshot();
+    BOOST_REQUIRE(node::FindSnapshotChainstateDir());
+
+    this->SimulateNodeRestart();
+    m_args.ForceSetArg("-reindex-chainstate", "1");
+    this->LoadVerifyActivateChainstate();
+    m_args.ForceSetArg("-reindex-chainstate", "0");
+
+    ChainstateManager& reindexed = *Assert(m_node.chainman);
+    LOCK(::cs_main);
+    BOOST_CHECK(!node::FindSnapshotChainstateDir());
+    BOOST_CHECK(!reindexed.IsSnapshotActive());
+    BOOST_CHECK_EQUAL(reindexed.GetAll().size(), 1);
+    uint256 stale_marker;
+    BOOST_CHECK(!m_node.evodb->ReadBestBlock(EvoDbIdentity::SNAPSHOT, stale_marker));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -5448,6 +5448,19 @@ 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,
@@ -5524,6 +5537,21 @@ bool ChainstateManager::ActivateSnapshot(
         LOCK(::cs_main);
         this->MaybeRebalanceCaches();
 
+        // PopulateAndValidateSnapshot commits the snapshot best-block and
+        // dual-chainstate markers as its last step, so a later failure (e.g.
+        // WriteSnapshotBaseBlockhash on an unwritable datadir) would leave them
+        // behind on a node that is back to a single chainstate. Erasing them is
+        // a no-op when they were never written.
+        CEvoDB& evo_db = this->ActiveChainstate().m_evoDb;
+        {
+            auto db_tx = evo_db.BeginTransaction(EvoDbIdentity::SNAPSHOT);
+            evo_db.EraseSnapshotMarkers();
+            db_tx->Commit();
+        }
+        if (!evo_db.CommitRootTransaction(EvoDbIdentity::SNAPSHOT)) {
+            AbortNode("Failed to roll back the snapshot EvoDB markers");
+        }
+
         // PopulateAndValidateSnapshot can return (in error) before the leveldb datadir
         // has been created, so only attempt removal if we got that far.
         if (auto snapshot_datadir = node::FindSnapshotChainstateDir()) {

src/validation.h

@@ -1162,6 +1162,16 @@ 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);