refactor: Add Chainstate::m_target_blockhash member

Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.

The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.

Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.

src/init.cpp

@@ -1880,7 +1880,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     } else {
         // Prior to setting NODE_NETWORK, check if we can provide historical blocks.
-        if (!WITH_LOCK(chainman.GetMutex(), return chainman.BackgroundSyncInProgress())) {
+        if (!WITH_LOCK(chainman.GetMutex(), return chainman.HistoricalChainstate())) {
             LogInfo("Setting NODE_NETWORK in non-prune mode");
             g_local_services = ServiceFlags(g_local_services | NODE_NETWORK);
         } else {

src/net_processing.cpp

@@ -5921,18 +5921,19 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                 return std::max(0, MAX_BLOCKS_IN_TRANSIT_PER_PEER - static_cast<int>(state.vBlocksInFlight.size()));
             };
 
-            // If a snapshot chainstate is in use, we want to find its next blocks
+            // If there are multiple chainstates, download blocks for the
-            // before the background chainstate to prioritize getting to network tip.
+            // current chainstate first, to prioritize getting to network tip
+            // before downloading historical blocks.
             FindNextBlocksToDownload(*peer, get_inflight_budget(), vToDownload, staller);
-            if (m_chainman.BackgroundSyncInProgress() && !IsLimitedPeer(*peer)) {
+            auto historical_blocks{m_chainman.GetHistoricalBlockRange()};
-                // If the background tip is not an ancestor of the snapshot block,
+            if (historical_blocks && !IsLimitedPeer(*peer)) {
+                // If the first needed historical block is not an ancestor of the last,
                 // we need to start requesting blocks from their last common ancestor.
-                const CBlockIndex *from_tip = LastCommonAncestor(m_chainman.GetBackgroundSyncTip(), m_chainman.GetSnapshotBaseBlock());
+                const CBlockIndex* from_tip = LastCommonAncestor(historical_blocks->first, historical_blocks->second);
                 TryDownloadingHistoricalBlocks(
                     *peer,
                     get_inflight_budget(),
-                    vToDownload, from_tip,
+                    vToDownload, from_tip, historical_blocks->second);
-                    Assert(m_chainman.GetSnapshotBaseBlock()));
             }
             for (const CBlockIndex *pindex : vToDownload) {
                 uint32_t nFetchFlags = GetFetchFlags(*peer);

src/node/chainstate.cpp

@@ -166,12 +166,14 @@ 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_cs{chainman.InitializeChainstate(options.mempool)};
 
     // Load a chain created from a UTXO snapshot, if any exist.
     bool has_snapshot = chainman.DetectSnapshotChainstate();
 
     if (has_snapshot && options.wipe_chainstate_db) {
+        // Reset chainstate target to network tip instead of snapshot block.
+        validated_cs.SetTargetBlock(nullptr);
         LogInfo("[snapshot] deleting snapshot chainstate due to reindexing");
         if (!chainman.DeleteSnapshotChainstate()) {
             return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};

src/test/validation_chainstate_tests.cpp

@@ -107,16 +107,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
 
     curr_tip = get_notify_tip();
 
-    BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2);
+    Chainstate& background_cs{*Assert(WITH_LOCK(::cs_main, return chainman.HistoricalChainstate()))};
-
-    Chainstate& background_cs{*Assert([&]() -> Chainstate* {
-        for (Chainstate* cs : chainman.GetAll()) {
-            if (cs != &chainman.ActiveChainstate()) {
-                return cs;
-            }
-        }
-        return nullptr;
-    }())};
 
     // Append the first block to the background chain.
     BlockValidationState state;

src/validation.cpp

@@ -1891,6 +1891,29 @@ const CBlockIndex* Chainstate::SnapshotBase() const
     return m_cached_snapshot_base;
 }
 
+const CBlockIndex* Chainstate::TargetBlock() const
+{
+    if (!m_target_blockhash) return nullptr;
+    if (!m_cached_target_block) m_cached_target_block = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_target_blockhash));
+    return m_cached_target_block;
+}
+
+void Chainstate::SetTargetBlock(CBlockIndex* block)
+{
+    if (block) {
+        m_target_blockhash = block->GetBlockHash();
+    } else {
+        m_target_blockhash.reset();
+    }
+    m_cached_target_block = block;
+}
+
+void Chainstate::SetTargetBlockHash(uint256 block_hash)
+{
+    m_target_blockhash = block_hash;
+    m_cached_target_block = nullptr;
+}
+
 void Chainstate::InitCoinsDB(
     size_t cache_size_bytes,
     bool in_memory,
@@ -3144,8 +3167,6 @@ bool Chainstate::ConnectTip(
     // If we are the background validation chainstate, check to see if we are done
     // validating the snapshot (i.e. our tip has reached the snapshot's base block).
     if (this != &m_chainman.ActiveChainstate()) {
-        // This call may set `m_disabled`, which is referenced immediately afterwards in
-        // ActivateBestChain, so that we stop connecting blocks past the snapshot base.
         m_chainman.MaybeCompleteSnapshotValidation();
     }
 
@@ -3439,12 +3460,8 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                     }
                 }
 
-                // This will have been toggled in
+                // Break this do-while to ensure we don't advance past the target block.
-                // ActivateBestChainStep -> ConnectTip -> MaybeCompleteSnapshotValidation,
+                if (ReachedTarget()) {
-                // if at all, so we should catch it here.
-                //
-                // Break this do-while to ensure we don't advance past the base snapshot.
-                if (m_disabled) {
                     break;
                 }
             } while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip)));
@@ -3486,7 +3503,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
         } // release cs_main
         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
 
-        bool disabled{false};
+        bool reached_target;
         {
             LOCK(m_chainman.GetMutex());
             if (exited_ibd) {
@@ -3500,15 +3517,14 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                 return false;
             }
 
-            disabled = m_disabled;
+            reached_target = ReachedTarget();
         }
 
-        if (disabled) {
+        if (reached_target) {
-            // Background chainstate has reached the snapshot base block, so exit.
+            // Chainstate has reached the target block, so exit.
-
+            //
-            // Restart indexes to resume indexing for all blocks unique to the snapshot
+            // Restart indexes so indexes can resync and index new blocks after
-            // chain. This resumes indexing "in order" from where the indexing on the
+            // the target block.
-            // background validation chain left off.
             //
             // This cannot be done while holding cs_main (within
             // MaybeCompleteSnapshotValidation) or a cs_main deadlock will occur.
@@ -3789,17 +3805,15 @@ void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex)
         return;
     }
 
-    bool is_active_chainstate = this == &m_chainman.ActiveChainstate();
+    const CBlockIndex* target_block{TargetBlock()};
-    if (is_active_chainstate) {
+    if (!target_block) {
-        // The active chainstate should always add entries that have more
+        // If no specific target block, add all entries that have more
         // work than the tip.
         setBlockIndexCandidates.insert(pindex);
-    } else if (!m_disabled) {
+    } else {
-        // For the background chainstate, we only consider connecting blocks
+        // If there is a target block, only consider connecting blocks
-        // towards the snapshot base (which can't be nullptr or else we'll
+        // towards the target block.
-        // never make progress).
+        if (target_block->GetAncestor(pindex->nHeight) == pindex) {
-        const CBlockIndex* snapshot_base{Assert(m_chainman.GetSnapshotBaseBlock())};
-        if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) {
             setBlockIndexCandidates.insert(pindex);
         }
     }
@@ -4483,7 +4497,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& blo
         return false;
     }
 
-    Chainstate* bg_chain{WITH_LOCK(cs_main, return BackgroundSyncInProgress() ? m_ibd_chainstate.get() : nullptr)};
+    Chainstate* bg_chain{WITH_LOCK(cs_main, return HistoricalChainstate())};
     BlockValidationState bg_state;
     if (bg_chain && !bg_chain->ActivateBestChain(bg_state, block)) {
         LogError("%s: [background] ActivateBestChain failed (%s)\n", __func__, bg_state.ToString());
@@ -5787,6 +5801,11 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
     const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip();
     assert(chaintip_loaded);
 
+    // Set snapshot block as the target block for the historical chainstate.
+    assert(m_ibd_chainstate.get());
+    assert(!m_ibd_chainstate->m_target_blockhash);
+    m_ibd_chainstate->SetTargetBlockHash(base_blockhash);
+
     // Transfer possession of the mempool to the snapshot chainstate.
     // Mempool is empty at this point because we're still in IBD.
     Assert(m_active_chainstate->m_mempool->size() == 0);
@@ -6048,23 +6067,25 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
 SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
 {
     AssertLockHeld(cs_main);
+
+    // If a snapshot does not need to be validated...
     if (m_ibd_chainstate.get() == &this->ActiveChainstate() ||
+            // Or if either chainstate is unusable...
             !this->IsUsable(m_snapshot_chainstate.get()) ||
             !this->IsUsable(m_ibd_chainstate.get()) ||
-            !m_ibd_chainstate->m_chain.Tip()) {
+            !m_snapshot_chainstate->m_from_snapshot_blockhash ||
-       // Nothing to do - this function only applies to the background
+            !m_ibd_chainstate->m_chain.Tip() ||
-       // validation chainstate.
+            // Or the validated chainstate is not targeting the snapshot block...
+            !m_ibd_chainstate->m_target_blockhash ||
+            *m_ibd_chainstate->m_target_blockhash != *m_snapshot_chainstate->m_from_snapshot_blockhash ||
+            // Or the validated chainstate has not reached the snapshot block yet...
+            !m_ibd_chainstate->ReachedTarget()) {
+       // Then a snapshot cannot be validated and there is nothing to do.
        return SnapshotCompletionResult::SKIPPED;
     }
     const int snapshot_tip_height = this->ActiveHeight();
     const int snapshot_base_height = *Assert(this->GetSnapshotBaseHeight());
     const CBlockIndex& index_new = *Assert(m_ibd_chainstate->m_chain.Tip());
-
-    if (index_new.nHeight < snapshot_base_height) {
-        // Background IBD not complete yet.
-        return SnapshotCompletionResult::SKIPPED;
-    }
-
     assert(SnapshotBlockhash());
     uint256 snapshot_blockhash = *Assert(SnapshotBlockhash());
 
@@ -6087,6 +6108,9 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
         LogError("[snapshot] !!! %s\n", user_error.original);
         LogError("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n");
 
+        // Reset chainstate target to network tip instead of snapshot block.
+        m_ibd_chainstate->SetTargetBlock(nullptr);
+
         m_active_chainstate = m_ibd_chainstate.get();
         m_snapshot_chainstate->m_disabled = true;
         assert(!this->IsUsable(m_snapshot_chainstate.get()));
@@ -6100,14 +6124,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
         GetNotifications().fatalError(user_error);
     };
 
-    if (index_new.GetBlockHash() != snapshot_blockhash) {
+    assert(index_new.GetBlockHash() == snapshot_blockhash);
-        LogWarning("[snapshot] supposed base block %s does not match the "
-          "snapshot base block %s (height %d). Snapshot is not valid.",
-          index_new.ToString(), snapshot_blockhash.ToString(), snapshot_base_height);
-        handle_invalid_snapshot();
-        return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH;
-    }
-
     assert(index_new.nHeight == snapshot_base_height);
 
     int curr_height = m_ibd_chainstate->m_chain.Height();
@@ -6286,6 +6303,12 @@ Chainstate& ChainstateManager::ActivateExistingSnapshot(uint256 base_blockhash)
         std::make_unique<Chainstate>(nullptr, m_blockman, *this, base_blockhash);
     LogInfo("[snapshot] switching active chainstate to %s", m_snapshot_chainstate->ToString());
 
+    // Set target block for historical chainstate to snapshot block.
+    assert(m_ibd_chainstate.get());
+    assert(!m_ibd_chainstate->m_target_blockhash);
+    m_ibd_chainstate->SetTargetBlockHash(base_blockhash);
+
+    // Transfer possession of the mempool to the chainstate.
     // Mempool is empty at this point because we're still in IBD.
     Assert(m_active_chainstate->m_mempool->size() == 0);
     Assert(!m_snapshot_chainstate->m_mempool);
@@ -6522,3 +6545,10 @@ std::pair<int, int> ChainstateManager::GetPruneRange(const Chainstate& chainstat
 
     return {prune_start, prune_end};
 }
+
+std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> ChainstateManager::GetHistoricalBlockRange() const
+{
+    const Chainstate* chainstate{HistoricalChainstate()};
+    if (!chainstate) return {};
+    return std::make_pair(chainstate->m_chain.Tip(), chainstate->TargetBlock());
+}

src/validation.h

@@ -561,6 +561,9 @@ protected:
     //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
     mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr};
 
+    //! Cached result of LookupBlockIndex(*m_target_blockhash)
+    mutable const CBlockIndex* m_cached_target_block GUARDED_BY(::cs_main){nullptr};
+
     std::optional<const char*> m_last_script_check_reason_logged GUARDED_BY(::cs_main){};
 
 public:
@@ -620,6 +623,12 @@ public:
      */
     const std::optional<uint256> m_from_snapshot_blockhash;
 
+    //! Target block for this chainstate. If this is not set, chainstate will
+    //! target the most-work, valid block. If this is set, ChainstateManager
+    //! considers this a "historical" chainstate since it will only contain old
+    //! blocks up to the target block, not newer blocks.
+    std::optional<uint256> m_target_blockhash GUARDED_BY(::cs_main);
+
     /**
      * The base of the snapshot this chainstate was created from.
      *
@@ -627,6 +636,26 @@ public:
      */
     const CBlockIndex* SnapshotBase() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    //! Return target block which chainstate tip is expected to reach, if this
+    //! is a historic chainstate being used to validate a snapshot, or null if
+    //! chainstate targets the most-work block.
+    const CBlockIndex* TargetBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    //! Set target block for this chainstate. If null, chainstate will target
+    //! the most-work valid block. If non-null chainstate will be a historic
+    //! chainstate and target the specified block.
+    void SetTargetBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    //! Set target block for this chainstate using just a block hash. Useful
+    //! when the block database has not been loaded yet.
+    void SetTargetBlockHash(uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+    //! Return true if chainstate reached target block.
+    bool ReachedTarget() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        const CBlockIndex* target_block{TargetBlock()};
+        assert(!target_block || target_block->GetAncestor(m_chain.Height()) == m_chain.Tip());
+        return target_block && target_block == m_chain.Tip();
+    }
+
     /**
      * The set of all CBlockIndex entries that have as much work as our current
      * tip or more, and transaction data needed to be validated (with
@@ -862,10 +891,6 @@ enum class SnapshotCompletionResult {
     // The UTXO set hash of the background validation chainstate does not match
     // the one expected by assumeutxo chainparams.
     HASH_MISMATCH,
-
-    // The blockhash of the current tip of the background validation chainstate does
-    // not match the one expected by the snapshot chainstate.
-    BASE_BLOCKHASH_MISMATCH,
 };
 
 /**
@@ -1121,22 +1146,19 @@ public:
     //! Returns nullptr if no snapshot has been loaded.
     const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    //! Return historical chainstate targeting a specific block, if any.
+    Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
+    {
+        auto* cs{m_ibd_chainstate.get()};
+        return IsUsable(cs) && cs->m_target_blockhash ? cs : nullptr;
+    }
+
     //! The most-work chain.
     Chainstate& ActiveChainstate() const;
     CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; }
     int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); }
     CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
 
-    //! The state of a background sync (for net processing)
-    bool BackgroundSyncInProgress() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) {
-        return IsUsable(m_snapshot_chainstate.get()) && IsUsable(m_ibd_chainstate.get());
-    }
-
-    //! The tip of the background sync chain
-    const CBlockIndex* GetBackgroundSyncTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) {
-        return BackgroundSyncInProgress() ? m_ibd_chainstate->m_chain.Tip() : nullptr;
-    }
-
     node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);
@@ -1337,6 +1359,9 @@ public:
     //! nullopt.
     std::optional<int> GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    //! Get range of historical blocks to download.
+    std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     //! If, due to invalidation / reconsideration of blocks, the previous
     //! best header is no longer valid / guaranteed to be the most-work
     //! header in our block-index not known to be invalid, recalculate it.