refactor: Replace ChainstateManager IBD and snapshot chainstates with…

… flat list of chainstates

Also add m_validity and m_target_block members to Chainstate class it is
possible to determine chainstate properties directly from Chainstate objects
and it is not neccessary for validation code make calls to ChainstateManager
and decide what to do by looking at assumeutxo download state.

Goal is to remove hardcoded logic handling assumeutxo snapshots from most
validation code. Also to make it easy to fix locking issues like the fact that
cs_main is currently held unnecessarily validating snapshots, and the fact that
background chainstate is not immediately deleted when it is no longer used,
wasting disk space and adding a long startup delay next time the node is
restarted.

This follows up on some previous discussions:

bitcoin#24232 (comment)
bitcoin#27746 (comment)
bitcoin#28562 (comment)

src/bench/wallet_create_tx.cpp

@@ -70,7 +70,7 @@ void generateFakeBlock(const CChainParams& params,
 
     // notify wallet
     const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip());
-    wallet.blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(pindex, &block));
+    wallet.blockConnected({}, kernel::MakeBlockInfo(pindex, &block));
 }
 
 struct PreSelectInputs {

src/bitcoin-chainstate.cpp

@@ -145,12 +145,9 @@ int main(int argc, char* argv[])
         }
     }
 
-    for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
-        BlockValidationState state;
-        if (!chainstate->ActivateBestChain(state, nullptr)) {
-            std::cerr << "Failed to connect best block (" << state.ToString() << ")" << std::endl;
-            goto epilogue;
-        }
+    if (auto result = chainman.ActivateBestChains(); !result) {
+        std::cerr << util::ErrorString(result).original << std::endl;
+        goto epilogue;
     }
 
     // Main program logic starts here
@@ -162,7 +159,7 @@ int main(int argc, char* argv[])
         LOCK(chainman.GetMutex());
         std::cout
         << "\t" << "Reindexing: " << std::boolalpha << node::fReindex.load() << std::noboolalpha << std::endl
-        << "\t" << "Snapshot Active: " << std::boolalpha << chainman.IsSnapshotActive() << std::noboolalpha << std::endl
+        << "\t" << "Snapshot Active: " << std::boolalpha << (chainman.MostWorkChainstate().SnapshotBase() != nullptr) << std::noboolalpha << std::endl
         << "\t" << "Active Height: " << chainman.ActiveHeight() << std::endl
         << "\t" << "Active IBD: " << std::boolalpha << chainman.IsInitialBlockDownload() << std::noboolalpha << std::endl;
         CBlockIndex* tip = chainman.ActiveTip();
@@ -295,7 +292,7 @@ int main(int argc, char* argv[])
     GetMainSignals().FlushBackgroundCallbacks();
     {
         LOCK(cs_main);
-        for (Chainstate* chainstate : chainman.GetAll()) {
+        for (auto& chainstate : chainman.m_chainstates) {
             if (chainstate->CanFlushToDisk()) {
                 chainstate->ForceFlushStateToDisk();
                 chainstate->ResetCoinsViews();

src/index/base.cpp

@@ -87,7 +87,7 @@ bool BaseIndex::Init()
     // m_chainstate member gives indexing code access to node internals. It is
     // removed in followup https://github.com/bitcoin/bitcoin/pull/24230
     m_chainstate = WITH_LOCK(::cs_main,
-        return &m_chain->context()->chainman->GetChainstateForIndexing());
+        return &m_chain->context()->chainman->ValidatedChainstate());
     // Register to validation interface before setting the 'm_synced' flag, so that
     // callbacks are not missed once m_synced is true.
     RegisterValidationInterface(this);
@@ -261,13 +261,11 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
 
 void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex)
 {
-    // Ignore events from the assumed-valid chain; we will process its blocks
-    // (sequentially) after it is fully verified by the background chainstate. This
-    // is to avoid any out-of-order indexing.
+    // Ignore events from unvalidated chains to avoid out-of-order indexing.
     //
     // TODO at some point we could parameterize whether a particular index can be
     // built out of order, but for now just do the conservative simple thing.
-    if (role == ChainstateRole::ASSUMEDVALID) {
+    if (!role.validated) {
         return;
     }
 
@@ -318,9 +316,8 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr<const
 
 void BaseIndex::ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator)
 {
-    // Ignore events from the assumed-valid chain; we will process its blocks
-    // (sequentially) after it is fully verified by the background chainstate.
-    if (role == ChainstateRole::ASSUMEDVALID) {
+    // Ignore events from unvalidated chains to avoid out-of-order indexing.
+    if (!role.validated) {
         return;
     }
 

src/init.cpp

@@ -291,7 +291,7 @@ void Shutdown(NodeContext& node)
     // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing
     if (node.chainman) {
         LOCK(cs_main);
-        for (Chainstate* chainstate : node.chainman->GetAll()) {
+        for (auto& chainstate : node.chainman->m_chainstates) {
             if (chainstate->CanFlushToDisk()) {
                 chainstate->ForceFlushStateToDisk();
             }
@@ -322,7 +322,7 @@ void Shutdown(NodeContext& node)
 
     if (node.chainman) {
         LOCK(cs_main);
-        for (Chainstate* chainstate : node.chainman->GetAll()) {
+        for (auto& chainstate : node.chainman->m_chainstates) {
             if (chainstate->CanFlushToDisk()) {
                 chainstate->ForceFlushStateToDisk();
                 chainstate->ResetCoinsViews();
@@ -1625,7 +1625,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     if (chainman.m_blockman.IsPruneMode()) {
         if (!fReindex) {
             LOCK(cs_main);
-            for (Chainstate* chainstate : chainman.GetAll()) {
+            for (auto& chainstate : chainman.m_chainstates) {
                 uiInterface.InitMessage(_("Pruning blockstore…").translated);
                 chainstate->PruneAndFlush();
             }
@@ -1941,7 +1941,7 @@ bool StartIndexBackgroundSync(NodeContext& node)
     std::optional<const CBlockIndex*> indexes_start_block;
     std::string older_index_name;
     ChainstateManager& chainman = *Assert(node.chainman);
-    const Chainstate& chainstate = WITH_LOCK(::cs_main, return chainman.GetChainstateForIndexing());
+    const Chainstate& chainstate = WITH_LOCK(::cs_main, return chainman.ValidatedChainstate());
     const CChain& index_chain = chainstate.m_chain;
 
     for (auto index : node.indexes) {

src/interfaces/chain.h

@@ -7,6 +7,7 @@
 
 #include <blockfilter.h>
 #include <common/settings.h>
+#include <kernel/chain.h>
 #include <primitives/transaction.h> // For CTransactionRef
 
 #include <functional>
@@ -27,7 +28,6 @@ class Coin;
 class uint256;
 enum class MemPoolRemovalReason;
 enum class RBFTransactionState;
-enum class ChainstateRole;
 struct bilingual_str;
 struct CBlockLocator;
 struct FeeCalculation;

src/kernel/chain.cpp

@@ -26,13 +26,3 @@ interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data
     return info;
 }
 } // namespace kernel
-
-std::ostream& operator<<(std::ostream& os, const ChainstateRole& role) {
-    switch(role) {
-        case ChainstateRole::NORMAL: os << "normal"; break;
-        case ChainstateRole::ASSUMEDVALID: os << "assumedvalid"; break;
-        case ChainstateRole::BACKGROUND: os << "background"; break;
-        default: os.setstate(std::ios_base::failbit);
-    }
-    return os;
-}

src/kernel/chain.h

@@ -19,21 +19,16 @@ interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* block_index, const CBlock
 
 } // namespace kernel
 
-//! This enum describes the various roles a specific Chainstate instance can take.
-//! Other parts of the system sometimes need to vary in behavior depending on the
-//! existence of a background validation chainstate, e.g. when building indexes.
-enum class ChainstateRole {
-    // Single chainstate in use, "normal" IBD mode.
-    NORMAL,
-
-    // Doing IBD-style validation in the background. Implies use of an assumed-valid
-    // chainstate.
-    BACKGROUND,
-
-    // Active assumed-valid chainstate. Implies use of a background IBD chainstate.
-    ASSUMEDVALID,
+struct ChainstateRole {
+    //! Whether this is an event from the chainstate syncing to the most-work
+    //! block, as opposed a chainstate downloading historic blocks and being
+    //! used to validate an assumeutxo snapshot.
+    bool most_work{true};
+
+    //! Whether this is an event from chainstate that's been fully validated
+    //! starting from the genesis block. False if is from an assumeutxo snapshot
+    //! chainstate that has not been validated yet.
+    bool validated{true};
 };
 
-std::ostream& operator<<(std::ostream& os, const ChainstateRole& role);
-
 #endif // BITCOIN_KERNEL_CHAIN_H

src/net_processing.cpp

@@ -1937,9 +1937,15 @@ void PeerManagerImpl::BlockConnected(
         }
     }
 
+<<<<<<< HEAD
     // The following task can be skipped since we don't maintain a mempool for
     // the ibd/background chainstate.
     if (role == ChainstateRole::BACKGROUND) {
+||||||| parent of 4ee775ed1e39 (refactor: Replace ChainstateManager IBD and snapshot chainstates with flat list of chainstates)
+    if (role == ChainstateRole::BACKGROUND) {
+=======
+    if (!role.most_work) {
+>>>>>>> 4ee775ed1e39 (refactor: Replace ChainstateManager IBD and snapshot chainstates with flat list of chainstates)
         return;
     }
     m_orphanage.EraseForBlock(*pblock);
@@ -5939,12 +5945,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
             // If a snapshot chainstate is in use, we want to find its next blocks
             // before the background chainstate to prioritize getting to network tip.
             FindNextBlocksToDownload(*peer, get_inflight_budget(), vToDownload, staller);
-            if (m_chainman.BackgroundSyncInProgress() && !IsLimitedPeer(*peer)) {
+            auto historical_blocks = m_chainman.GetHistoricalBlockRange();
+            if (historical_blocks && !IsLimitedPeer(*peer)) {
                 TryDownloadingHistoricalBlocks(
                     *peer,
                     get_inflight_budget(),
-                    vToDownload, m_chainman.GetBackgroundSyncTip(),
-                    Assert(m_chainman.GetSnapshotBaseBlock()));
+                    vToDownload, historical_blocks->first, historical_blocks->second);
             }
             for (const CBlockIndex *pindex : vToDownload) {
                 uint32_t nFetchFlags = GetFetchFlags(*peer);

src/node/blockstorage.cpp

@@ -285,7 +285,7 @@ void BlockManager::FindFilesToPruneManual(
         count++;
     }
     LogPrintf("[%s] Prune (Manual): prune_height=%d removed %d blk/rev pairs\n",
-        chain.GetRole(), last_block_can_prune, count);
+        chain.GetName(), last_block_can_prune, count);
 }
 
 void BlockManager::FindFilesToPrune(
@@ -296,8 +296,9 @@ void BlockManager::FindFilesToPrune(
 {
     LOCK2(cs_main, cs_LastBlockFile);
     // Distribute our -prune budget over all chainstates.
+    const int num_chainstates = chainman.HistoricalChainstate() ? 2 : 1;
     const auto target = std::max(
-        MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / chainman.GetAll().size());
+        MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / num_chainstates);
 
     if (chain.m_chain.Height() < 0 || target == 0) {
         return;
@@ -353,7 +354,7 @@ void BlockManager::FindFilesToPrune(
     }
 
     LogPrint(BCLog::PRUNE, "[%s] target=%dMiB actual=%dMiB diff=%dMiB min_height=%d max_prune_height=%d removed %d blk/rev pairs\n",
-             chain.GetRole(), target / 1024 / 1024, nCurrentUsage / 1024 / 1024,
+             chain.GetName(), target / 1024 / 1024, nCurrentUsage / 1024 / 1024,
              (int64_t(target) - int64_t(nCurrentUsage)) / 1024 / 1024,
              min_block_to_prune, last_block_can_prune, count);
 }
@@ -1181,16 +1182,8 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
         }
 
         // scan for better chains in the block chain database, that are not yet connected in the active best chain
-
-        // We can't hold cs_main during ActivateBestChain even though we're accessing
-        // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
-        // the relevant pointers before the ABC call.
-        for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
-            BlockValidationState state;
-            if (!chainstate->ActivateBestChain(state, nullptr)) {
-                chainman.GetNotifications().fatalError(strprintf("Failed to connect best block (%s)", state.ToString()));
-                return;
-            }
+        if (auto result = chainman.ActivateBestChains(); !result) {
+            chainman.GetNotifications().fatalError(util::ErrorString(result).original);
         }
     } // End scope of ImportingNow
 }

src/node/chainstate.cpp

@@ -87,8 +87,8 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
     }
 
-    auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+    auto is_coinsview_empty = [&](Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+        return options.reindex || options.reindex_chainstate || chainstate.CoinsTip().GetBestBlock().IsNull();
     };
 
     assert(chainman.m_total_coinstip_cache > 0);
@@ -103,7 +103,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     // At this point we're either in reindex or we've loaded a useful
     // block tree into BlockIndex()!
 
-    for (Chainstate* chainstate : chainman.GetAll()) {
+    for (auto& chainstate : chainman.m_chainstates) {
         LogPrintf("Initializing chainstate %s\n", chainstate->ToString());
 
         chainstate->InitCoinsDB(
@@ -132,7 +132,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         chainstate->InitCoinsCache(chainman.m_total_coinstip_cache * init_cache_fraction);
         assert(chainstate->CanFlushToDisk());
 
-        if (!is_coinsview_empty(chainstate)) {
+        if (!is_coinsview_empty(*chainstate)) {
             // LoadChainTip initializes the chain based on CoinsTip()'s best block
             if (!chainstate->LoadChainTip()) {
                 return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
@@ -142,9 +142,9 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     }
 
     if (!options.reindex) {
-        auto chainstates{chainman.GetAll()};
+        const auto& chainstates{chainman.m_chainstates};
         if (std::any_of(chainstates.begin(), chainstates.end(),
-                        [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
+                        [](auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
             return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
                                                              chainman.GetConsensus().SegwitHeight)};
         };
@@ -182,16 +182,23 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     chainman.m_total_coinsdb_cache = cache_sizes.coins_db;
 
     // Load the fully validated chainstate.
-    chainman.InitializeChainstate(options.mempool);
+    Chainstate& validated_chainstate = chainman.InitializeChainstate(options.mempool);
 
     // Load a chain created from a UTXO snapshot, if any exist.
+<<<<<<< HEAD
     bool has_snapshot = chainman.DetectSnapshotChainstate();
+||||||| parent of 4ee775ed1e39 (refactor: Replace ChainstateManager IBD and snapshot chainstates with flat list of chainstates)
+    bool has_snapshot = chainman.DetectSnapshotChainstate(options.mempool);
+=======
+    Chainstate* snapshot_chainstate = chainman.DetectSnapshotChainstate();
+>>>>>>> 4ee775ed1e39 (refactor: Replace ChainstateManager IBD and snapshot chainstates with flat list of chainstates)
 
-    if (has_snapshot && (options.reindex || options.reindex_chainstate)) {
+    if (snapshot_chainstate && (options.reindex || options.reindex_chainstate)) {
         LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");
-        if (!chainman.DeleteSnapshotChainstate()) {
+        if (!chainman.DeleteChainstate(*snapshot_chainstate)) {
             return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};
         }
+        snapshot_chainstate = nullptr;
     }
 
     auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
@@ -207,23 +214,29 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     // snapshot is actually validated? Because this entails unusual
     // filesystem operations to move leveldb data directories around, and that seems
     // too risky to do in the middle of normal runtime.
-    auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation();
+    auto snapshot_completion{SnapshotCompletionResult::SKIPPED};
+    if (snapshot_chainstate) {
+        snapshot_completion = chainman.MaybeCompleteSnapshotValidation(validated_chainstate, *snapshot_chainstate);
+    }
 
     if (snapshot_completion == SnapshotCompletionResult::SKIPPED) {
         // do nothing; expected case
     } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) {
         LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n");
-        if (!chainman.ValidatedSnapshotCleanup()) {
-            return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")};
+        const auto validated_path{validated_chainstate.CoinsDB().StoragePath()};
+        const auto snapshot_path{snapshot_chainstate->CoinsDB().StoragePath()};
+        // Since we're going to be moving around the underlying leveldb filesystem content
+        // for each chainstate, make sure that the chainstates (and their constituent
+        // CoinsViews members) have been destructed first.
+        chainman.RemoveChainstate(validated_chainstate);
+        chainman.RemoveChainstate(*snapshot_chainstate);
+        if (validated_path && snapshot_path) {
+            chainman.ValidatedSnapshotCleanup(*validated_path, *snapshot_path);
+        } else {
+            LogPrintf("[snapshot] snapshot chainstate cleanup cannot happen with "
+                      "in-memory chainstates. You are testing, right?\n");
         }
 
-        // Because ValidatedSnapshotCleanup() has torn down chainstates with
-        // ChainstateManager::ResetChainstates(), reinitialize them here without
-        // duplicating the blockindex work above.
-        assert(chainman.GetAll().empty());
-        assert(!chainman.IsSnapshotActive());
-        assert(!chainman.IsSnapshotValidated());
-
         chainman.InitializeChainstate(options.mempool);
 
         // A reload of the block index is required to recompute setBlockIndexCandidates
@@ -245,14 +258,14 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
 
 ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
 {
-    auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+    auto is_coinsview_empty = [&](Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+        return options.reindex || options.reindex_chainstate || chainstate.CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
 
-    for (Chainstate* chainstate : chainman.GetAll()) {
-        if (!is_coinsview_empty(chainstate)) {
+    for (auto& chainstate : chainman.m_chainstates) {
+        if (!is_coinsview_empty(*chainstate)) {
             const CBlockIndex* tip = chainstate->m_chain.Tip();
             if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
                 return {ChainstateLoadStatus::FAILURE, _("The block database contains a block which appears to be from the future. "

src/node/interfaces.cpp

@@ -812,7 +812,7 @@ class ChainImpl : public Chain
     }
     bool hasAssumedValidChain() override
     {
-        return chainman().IsSnapshotActive();
+        return WITH_LOCK(::cs_main, return chainman().MostWorkChainstate().SnapshotBase());
     }
 
     NodeContext* context() override { return &m_node; }