Merge bitcoin/bitcoin#24515: Only load BlockMan in BlockMan member functions

f865cf8 Add and use BlockManager::GetAllBlockIndices (Carl Dong)
28ba031 Add and use CBlockIndexHeightOnlyComparator (Carl Dong)
12eb05d move-only: Move CBlockIndexWorkComparator to blockstorage (Carl Dong)
c600ee3 Only load BlockMan in BlockMan member functions (Carl Dong)
42e56d9 style-only: No need for std::pair for vSortedByHeight (Carl Dong)
3bbb6fe style-only: Various blockstorage.cpp cleanups (Carl Dong)
5be9ee3 refactor: more const annotations for uses of CBlockIndex* (Anthony Towns)

Pull request description:

  The only important commit is "Only load BlockMan in BlockMan member functions", everything else is all just small style changes.

  Here's the commit message, reproduced:
  ```
  This commit effectively splits the "load block index itself" logic from
  "derive Chainstate variables from loaded block index" logic.

  This means that BlockManager::LoadBlockIndex{,DB} will only load what's
  relevant to the BlockManager.
  ```

ACKs for top commit:
  ajtowns:
    ACK f865cf8 ; code review only
  MarcoFalke:
    review ACK f865cf8 🗂

Tree-SHA512: 7b204d782834e06fd7329d022e2ae860181b4e8105c33bfb928539a4ec24161dc7438a9c4d4ee279dcad77de310c160b997bb8aa18923243d0fd55ccf4ad7c3a

Author: MarcoFalke

src/index/coinstatsindex.cpp

@@ -277,7 +277,7 @@ bool CoinStatsIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* n
 
     {
         LOCK(cs_main);
-        CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip->GetBlockHash())};
+        const CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip->GetBlockHash())};
         const auto& consensus_params{Params().GetConsensus()};
 
         do {

src/node/blockstorage.cpp

@@ -28,10 +28,45 @@ bool fHavePruned = false;
 bool fPruneMode = false;
 uint64_t nPruneTarget = 0;
 
+bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
+{
+    // First sort by most total work, ...
+    if (pa->nChainWork > pb->nChainWork) return false;
+    if (pa->nChainWork < pb->nChainWork) return true;
+
+    // ... then by earliest time received, ...
+    if (pa->nSequenceId < pb->nSequenceId) return false;
+    if (pa->nSequenceId > pb->nSequenceId) return true;
+
+    // Use pointer address as tie breaker (should only happen with blocks
+    // loaded from disk, as those all have id 0).
+    if (pa < pb) return false;
+    if (pa > pb) return true;
+
+    // Identical blocks.
+    return false;
+}
+
+bool CBlockIndexHeightOnlyComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
+{
+    return pa->nHeight < pb->nHeight;
+}
+
 static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false);
 static FlatFileSeq BlockFileSeq();
 static FlatFileSeq UndoFileSeq();
 
+std::vector<CBlockIndex*> BlockManager::GetAllBlockIndices()
+{
+    AssertLockHeld(cs_main);
+    std::vector<CBlockIndex*> rv;
+    rv.reserve(m_block_index.size());
+    for (auto& [_, block_index] : m_block_index) {
+        rv.push_back(&block_index);
+    }
+    return rv;
+}
+
 CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash)
 {
     AssertLockHeld(cs_main);
@@ -203,55 +238,27 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash)
         return nullptr;
     }
 
-    // Return existing or create new
-    auto [mi, inserted] = m_block_index.try_emplace(hash);
+    const auto [mi, inserted]{m_block_index.try_emplace(hash)};
     CBlockIndex* pindex = &(*mi).second;
     if (inserted) {
         pindex->phashBlock = &((*mi).first);
     }
     return pindex;
 }
 
-bool BlockManager::LoadBlockIndex(
-    const Consensus::Params& consensus_params,
-    ChainstateManager& chainman)
+bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params)
 {
     if (!m_block_tree_db->LoadBlockIndexGuts(consensus_params, [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) {
         return false;
     }
 
     // Calculate nChainWork
-    std::vector<std::pair<int, CBlockIndex*>> vSortedByHeight;
-    vSortedByHeight.reserve(m_block_index.size());
-    for (auto& [_, block_index] : m_block_index) {
-        CBlockIndex* pindex = &block_index;
-        vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));
-    }
-    sort(vSortedByHeight.begin(), vSortedByHeight.end());
-
-    // Find start of assumed-valid region.
-    int first_assumed_valid_height = std::numeric_limits<int>::max();
-
-    for (const auto& [height, block] : vSortedByHeight) {
-        if (block->IsAssumedValid()) {
-            auto chainstates = chainman.GetAll();
-
-            // If we encounter an assumed-valid block index entry, ensure that we have
-            // one chainstate that tolerates assumed-valid entries and another that does
-            // not (i.e. the background validation chainstate), since assumed-valid
-            // entries should always be pending validation by a fully-validated chainstate.
-            auto any_chain = [&](auto fnc) { return std::any_of(chainstates.cbegin(), chainstates.cend(), fnc); };
-            assert(any_chain([](auto chainstate) { return chainstate->reliesOnAssumedValid(); }));
-            assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); }));
-
-            first_assumed_valid_height = height;
-            break;
-        }
-    }
+    std::vector<CBlockIndex*> vSortedByHeight{GetAllBlockIndices()};
+    std::sort(vSortedByHeight.begin(), vSortedByHeight.end(),
+              CBlockIndexHeightOnlyComparator());
 
-    for (const std::pair<int, CBlockIndex*>& item : vSortedByHeight) {
+    for (CBlockIndex* pindex : vSortedByHeight) {
         if (ShutdownRequested()) return false;
-        CBlockIndex* pindex = item.second;
         pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex);
         pindex->nTimeMax = (pindex->pprev ? std::max(pindex->pprev->nTimeMax, pindex->nTime) : pindex->nTime);
 
@@ -275,43 +282,6 @@ bool BlockManager::LoadBlockIndex(
             pindex->nStatus |= BLOCK_FAILED_CHILD;
             m_dirty_blockindex.insert(pindex);
         }
-        if (pindex->IsAssumedValid() ||
-                (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) &&
-                 (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) {
-
-            // Fill each chainstate's block candidate set. Only add assumed-valid
-            // blocks to the tip candidate set if the chainstate is allowed to rely on
-            // assumed-valid blocks.
-            //
-            // If all setBlockIndexCandidates contained the assumed-valid blocks, the
-            // background chainstate's ActivateBestChain() call would add assumed-valid
-            // blocks to the chain (based on how FindMostWorkChain() works). Obviously
-            // we don't want this since the purpose of the background validation chain
-            // is to validate assued-valid blocks.
-            //
-            // Note: This is considering all blocks whose height is greater or equal to
-            // the first assumed-valid block to be assumed-valid blocks, and excluding
-            // them from the background chainstate's setBlockIndexCandidates set. This
-            // does mean that some blocks which are not technically assumed-valid
-            // (later blocks on a fork beginning before the first assumed-valid block)
-            // might not get added to the background chainstate, but this is ok,
-            // because they will still be attached to the active chainstate if they
-            // actually contain more work.
-            //
-            // Instead of this height-based approach, an earlier attempt was made at
-            // detecting "holistically" whether the block index under consideration
-            // relied on an assumed-valid ancestor, but this proved to be too slow to
-            // be practical.
-            for (CChainState* chainstate : chainman.GetAll()) {
-                if (chainstate->reliesOnAssumedValid() ||
-                        pindex->nHeight < first_assumed_valid_height) {
-                    chainstate->setBlockIndexCandidates.insert(pindex);
-                }
-            }
-        }
-        if (pindex->nStatus & BLOCK_FAILED_MASK && (!chainman.m_best_invalid || pindex->nChainWork > chainman.m_best_invalid->nChainWork)) {
-            chainman.m_best_invalid = pindex;
-        }
         if (pindex->pprev) {
             pindex->BuildSkip();
         }
@@ -355,9 +325,9 @@ bool BlockManager::WriteBlockIndexDB()
     return true;
 }
 
-bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman)
+bool BlockManager::LoadBlockIndexDB()
 {
-    if (!LoadBlockIndex(::Params().GetConsensus(), chainman)) {
+    if (!LoadBlockIndex(::Params().GetConsensus())) {
         return false;
     }
 
@@ -382,9 +352,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman)
     LogPrintf("Checking all blk files are present...\n");
     std::set<int> setBlkDataFiles;
     for (const auto& [_, block_index] : m_block_index) {
-        const CBlockIndex* pindex = &block_index;
-        if (pindex->nStatus & BLOCK_HAVE_DATA) {
-            setBlkDataFiles.insert(pindex->nFile);
+        if (block_index.nStatus & BLOCK_HAVE_DATA) {
+            setBlkDataFiles.insert(block_index.nFile);
         }
     }
     for (std::set<int>::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) {
@@ -408,13 +377,13 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman)
     return true;
 }
 
-CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
+const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
 {
     const MapCheckpoints& checkpoints = data.mapCheckpoints;
 
     for (const MapCheckpoints::value_type& i : reverse_iterate(checkpoints)) {
         const uint256& hash = i.second;
-        CBlockIndex* pindex = LookupBlockIndex(hash);
+        const CBlockIndex* pindex = LookupBlockIndex(hash);
         if (pindex) {
             return pindex;
         }

src/node/blockstorage.h

@@ -62,6 +62,11 @@ struct CBlockIndexWorkComparator {
     bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const;
 };
 
+struct CBlockIndexHeightOnlyComparator {
+    /* Only compares the height of two block indices, doesn't try to tie-break */
+    bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const;
+};
+
 /**
  * Maintains a tree of blocks (stored in `m_block_index`) which is consulted
  * to determine where the most-work tip is.
@@ -118,6 +123,8 @@ class BlockManager
 public:
     BlockMap m_block_index GUARDED_BY(cs_main);
 
+    std::vector<CBlockIndex*> GetAllBlockIndices() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     /**
      * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
      * Pruned nodes may have entries where B is missing data.
@@ -127,16 +134,15 @@ class BlockManager
     std::unique_ptr<CBlockTreeDB> m_block_tree_db GUARDED_BY(::cs_main);
 
     bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
-    bool LoadBlockIndexDB(ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /**
      * Load the blocktree off disk and into memory. Populate certain metadata
      * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral
      * collections like m_dirty_blockindex.
      */
-    bool LoadBlockIndex(
-        const Consensus::Params& consensus_params,
-        ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool LoadBlockIndex(const Consensus::Params& consensus_params)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /** Clear all data members. */
     void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
@@ -163,7 +169,7 @@ class BlockManager
     uint64_t CalculateCurrentUsage();
 
     //! Returns last CBlockIndex* that is a checkpoint
-    CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     ~BlockManager()
     {

src/node/interfaces.cpp

@@ -490,7 +490,7 @@ class ChainImpl : public Chain
     {
         LOCK(cs_main);
         const CChainState& active = Assert(m_node.chainman)->ActiveChainstate();
-        if (CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) {
+        if (const CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) {
             return fork->nHeight;
         }
         return std::nullopt;
@@ -557,7 +557,7 @@ class ChainImpl : public Chain
         // used to limit the range, and passing min_height that's too low or
         // max_height that's too high will not crash or change the result.
         LOCK(::cs_main);
-        if (CBlockIndex* block = chainman().m_blockman.LookupBlockIndex(block_hash)) {
+        if (const CBlockIndex* block = chainman().m_blockman.LookupBlockIndex(block_hash)) {
             if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height);
             for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) {
                 // Check pprev to not segfault if min_height is too low

src/node/miner.cpp

@@ -50,7 +50,7 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
     block.vtx.at(0) = MakeTransactionRef(tx);
 
-    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
+    const CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
     GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
 
     block.hashMerkleRoot = BlockMerkleRoot(block);

src/rest.cpp

@@ -284,8 +284,8 @@ static bool rest_block(const std::any& context,
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
     CBlock block;
-    CBlockIndex* pblockindex = nullptr;
-    CBlockIndex* tip = nullptr;
+    const CBlockIndex* pblockindex = nullptr;
+    const CBlockIndex* tip = nullptr;
     {
         ChainstateManager* maybe_chainman = GetChainman(context, req);
         if (!maybe_chainman) return false;

src/rpc/blockchain.cpp

@@ -104,7 +104,8 @@ static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* b
     return blockindex == tip ? 1 : -1;
 }
 
-CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) {
+static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman)
+{
     LOCK(::cs_main);
     CChain& active_chain = chainman.ActiveChain();
 
@@ -121,7 +122,7 @@ CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainma
         return active_chain[height];
     } else {
         const uint256 hash{ParseHashV(param, "hash_or_height")};
-        CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
+        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
 
         if (!pindex) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
@@ -488,7 +489,7 @@ static RPCHelpMan getblockhash()
     if (nHeight < 0 || nHeight > active_chain.Height())
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");
 
-    CBlockIndex* pblockindex = active_chain[nHeight];
+    const CBlockIndex* pblockindex = active_chain[nHeight];
     return pblockindex->GetBlockHash().GetHex();
 },
     };
@@ -766,7 +767,7 @@ static RPCHelpMan pruneblockchain()
     // too low to be a block time (corresponds to timestamp from Sep 2001).
     if (heightParam > 1000000000) {
         // Add a 2 hour buffer to include blocks which might have had old timestamps
-        CBlockIndex* pindex = active_chain.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0);
+        const CBlockIndex* pindex = active_chain.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0);
         if (!pindex) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Could not find block with at least the specified timestamp.");
         }
@@ -860,7 +861,7 @@ static RPCHelpMan gettxoutsetinfo()
 {
     UniValue ret(UniValue::VOBJ);
 
-    CBlockIndex* pindex{nullptr};
+    const CBlockIndex* pindex{nullptr};
     const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())};
     CCoinsStats stats{hash_type};
     stats.index_requested = request.params[2].isNull() || request.params[2].get_bool();
@@ -1764,7 +1765,7 @@ static RPCHelpMan getblockstats()
 {
     ChainstateManager& chainman = EnsureAnyChainman(request.context);
     LOCK(cs_main);
-    CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)};
+    const CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)};
     CHECK_NONFATAL(pindex != nullptr);
 
     std::set<std::string> stats;
@@ -2124,7 +2125,7 @@ static RPCHelpMan scantxoutset()
         g_should_abort_scan = false;
         int64_t count = 0;
         std::unique_ptr<CCoinsViewCursor> pcursor;
-        CBlockIndex* tip;
+        const CBlockIndex* tip;
         NodeContext& node = EnsureAnyNodeContext(request.context);
         {
             ChainstateManager& chainman = EnsureChainman(node);
@@ -2312,7 +2313,7 @@ UniValue CreateUTXOSnapshot(
 {
     std::unique_ptr<CCoinsViewCursor> pcursor;
     CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED};
-    CBlockIndex* tip;
+    const CBlockIndex* tip;
 
     {
         // We need to lock cs_main to ensure that the coinsdb isn't written to

src/rpc/rawtransaction.cpp

@@ -67,7 +67,7 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue&
         LOCK(cs_main);
 
         entry.pushKV("blockhash", hashBlock.GetHex());
-        CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock);
+        const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock);
         if (pindex) {
             if (active_chainstate.m_chain.Contains(pindex)) {
                 entry.pushKV("confirmations", 1 + active_chainstate.m_chain.Height() - pindex->nHeight);
@@ -207,7 +207,7 @@ static RPCHelpMan getrawtransaction()
 
     bool in_active_chain = true;
     uint256 hash = ParseHashV(request.params[0], "parameter 1");
-    CBlockIndex* blockindex = nullptr;
+    const CBlockIndex* blockindex = nullptr;
 
     if (hash == Params().GenesisBlock().hashMerkleRoot) {
         // Special exception for the genesis block coinbase transaction
@@ -302,7 +302,7 @@ static RPCHelpMan gettxoutproof()
         }
     }
 
-    CBlockIndex* pblockindex = nullptr;
+    const CBlockIndex* pblockindex = nullptr;
     uint256 hashBlock;
     ChainstateManager& chainman = EnsureAnyChainman(request.context);
     if (!request.params[1].isNull()) {