Merge bitcoin/bitcoin#21767: [Bundle 6/n] Prune g_chainman usage in auxiliary modules

7a799c9 index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1 bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6 miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade1 rest: Add GetChainman function and use it (Carl Dong)
fc1c282 rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)

Pull request description:

  Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

  The first 2 commits are fixups addressing review for the last bundle: #21391

  NEW note:
  1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!

  Note to reviewers:
  1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
  1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
  2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
  3. Remove `old_function`

ACKs for top commit:
  jarolrod:
    ACK  7a799c9
  ariard:
    Code Review ACK 7a799c9
  fjahr:
    re-ACK 7a799c9
  MarcoFalke:
    review ACK 7a799c9 🌠
  ryanofsky:
    Code review ACK 7a799c9. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
  jamesob:
    conditional ACK 7a799c9 ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))

Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e

Author: MarcoFalke

src/bench/block_assemble.cpp

@@ -37,7 +37,7 @@ static void AssembleBlock(benchmark::Bench& bench)
         LOCK(::cs_main); // Required for ::AcceptToMemoryPool.
 
         for (const auto& txr : txs) {
-            const MempoolAcceptResult res = ::AcceptToMemoryPool(::ChainstateActive(), *test_setup->m_node.mempool, txr, false /* bypass_limits */);
+            const MempoolAcceptResult res = ::AcceptToMemoryPool(test_setup->m_node.chainman->ActiveChainstate(), *test_setup->m_node.mempool, txr, false /* bypass_limits */);
             assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID);
         }
     }

src/bench/duplicate_inputs.cpp

@@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
     CMutableTransaction naughtyTx{};
 
     LOCK(cs_main);
-    CBlockIndex* pindexPrev = ::ChainActive().Tip();
+    assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain()));
+    CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveChain().Tip();
     assert(pindexPrev != nullptr);
     block.nBits = GetNextWorkRequired(pindexPrev, &block, chainparams.GetConsensus());
     block.nNonce = 0;

src/index/base.cpp

@@ -60,33 +60,34 @@ bool BaseIndex::Init()
     }
 
     LOCK(cs_main);
+    CChain& active_chain = m_chainstate->m_chain;
     if (locator.IsNull()) {
         m_best_block_index = nullptr;
     } else {
-        m_best_block_index = g_chainman.m_blockman.FindForkInGlobalIndex(::ChainActive(), locator);
+        m_best_block_index = m_chainstate->m_blockman.FindForkInGlobalIndex(active_chain, locator);
     }
-    m_synced = m_best_block_index.load() == ::ChainActive().Tip();
+    m_synced = m_best_block_index.load() == active_chain.Tip();
     if (!m_synced) {
         bool prune_violation = false;
         if (!m_best_block_index) {
             // index is not built yet
             // make sure we have all block data back to the genesis
-            const CBlockIndex* block = ::ChainActive().Tip();
+            const CBlockIndex* block = active_chain.Tip();
             while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
                 block = block->pprev;
             }
-            prune_violation = block != ::ChainActive().Genesis();
+            prune_violation = block != active_chain.Genesis();
         }
         // in case the index has a best block set and is not fully synced
         // check if we have the required blocks to continue building the index
         else {
             const CBlockIndex* block_to_test = m_best_block_index.load();
-            if (!ChainActive().Contains(block_to_test)) {
+            if (!active_chain.Contains(block_to_test)) {
                 // if the bestblock is not part of the mainchain, find the fork
                 // and make sure we have all data down to the fork
-                block_to_test = ::ChainActive().FindFork(block_to_test);
+                block_to_test = active_chain.FindFork(block_to_test);
             }
-            const CBlockIndex* block = ::ChainActive().Tip();
+            const CBlockIndex* block = active_chain.Tip();
             prune_violation = true;
             // check backwards from the tip if we have all block data until we reach the indexes bestblock
             while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
@@ -104,20 +105,20 @@ bool BaseIndex::Init()
     return true;
 }
 
-static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     AssertLockHeld(cs_main);
 
     if (!pindex_prev) {
-        return ::ChainActive().Genesis();
+        return chain.Genesis();
     }
 
-    const CBlockIndex* pindex = ::ChainActive().Next(pindex_prev);
+    const CBlockIndex* pindex = chain.Next(pindex_prev);
     if (pindex) {
         return pindex;
     }
 
-    return ::ChainActive().Next(::ChainActive().FindFork(pindex_prev));
+    return chain.Next(chain.FindFork(pindex_prev));
 }
 
 void BaseIndex::ThreadSync()
@@ -140,7 +141,7 @@ void BaseIndex::ThreadSync()
 
             {
                 LOCK(cs_main);
-                const CBlockIndex* pindex_next = NextSyncBlock(pindex);
+                const CBlockIndex* pindex_next = NextSyncBlock(pindex, m_chainstate->m_chain);
                 if (!pindex_next) {
                     m_best_block_index = pindex;
                     m_synced = true;
@@ -203,7 +204,7 @@ bool BaseIndex::Commit()
 bool BaseIndex::CommitInternal(CDBBatch& batch)
 {
     LOCK(cs_main);
-    GetDB().WriteBestBlock(batch, ::ChainActive().GetLocator(m_best_block_index));
+    GetDB().WriteBestBlock(batch, m_chainstate->m_chain.GetLocator(m_best_block_index));
     return true;
 }
 
@@ -279,7 +280,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator)
     const CBlockIndex* locator_tip_index;
     {
         LOCK(cs_main);
-        locator_tip_index = g_chainman.m_blockman.LookupBlockIndex(locator_tip_hash);
+        locator_tip_index = m_chainstate->m_blockman.LookupBlockIndex(locator_tip_hash);
     }
 
     if (!locator_tip_index) {
@@ -320,7 +321,7 @@ bool BaseIndex::BlockUntilSyncedToCurrentChain() const
         // Skip the queue-draining stuff if we know we're caught up with
         // ::ChainActive().Tip().
         LOCK(cs_main);
-        const CBlockIndex* chain_tip = ::ChainActive().Tip();
+        const CBlockIndex* chain_tip = m_chainstate->m_chain.Tip();
         const CBlockIndex* best_block_index = m_best_block_index.load();
         if (best_block_index->GetAncestor(chain_tip->nHeight) == chain_tip) {
             return true;
@@ -337,8 +338,10 @@ void BaseIndex::Interrupt()
     m_interrupt();
 }
 
-bool BaseIndex::Start()
+bool BaseIndex::Start(CChainState& active_chainstate)
 {
+    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
+    m_chainstate = &active_chainstate;
     // Need to register this ValidationInterface before running Init(), so that
     // callbacks are not missed if Init sets m_synced to true.
     RegisterValidationInterface(this);

src/index/base.h

@@ -12,6 +12,7 @@
 #include <validationinterface.h>
 
 class CBlockIndex;
+class CChainState;
 
 struct IndexSummary {
     std::string name;
@@ -75,8 +76,9 @@ class BaseIndex : public CValidationInterface
     /// to a chain reorganization), the index must halt until Commit succeeds or else it could end up
     /// getting corrupted.
     bool Commit();
-
 protected:
+    CChainState* m_chainstate{nullptr};
+
     void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) override;
 
     void ChainStateFlushed(const CBlockLocator& locator) override;
@@ -117,7 +119,7 @@ class BaseIndex : public CValidationInterface
 
     /// Start initializes the sync state and registers the instance as a
     /// ValidationInterface so that it stays in sync with blockchain updates.
-    [[nodiscard]] bool Start();
+    [[nodiscard]] bool Start(CChainState& active_chainstate);
 
     /// Stops the instance from staying in sync with blockchain updates.
     void Stop();

src/index/coinstatsindex.cpp

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

src/index/txindex.cpp

@@ -204,7 +204,7 @@ bool TxIndex::Init()
     // Attempt to migrate txindex from the old database to the new one. Even if
     // chain_tip is null, the node could be reindexing and we still want to
     // delete txindex records in the old database.
-    if (!m_db->MigrateData(*pblocktree, ::ChainActive().GetLocator())) {
+    if (!m_db->MigrateData(*pblocktree, m_chainstate->m_chain.GetLocator())) {
         return false;
     }
 

src/init.cpp

@@ -1549,21 +1549,21 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // ********************************************************* Step 8: start indexers
     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
         g_txindex = std::make_unique<TxIndex>(nTxIndexCache, false, fReindex);
-        if (!g_txindex->Start()) {
+        if (!g_txindex->Start(::ChainstateActive())) {
             return false;
         }
     }
 
     for (const auto& filter_type : g_enabled_filter_types) {
         InitBlockFilterIndex(filter_type, filter_index_cache, false, fReindex);
-        if (!GetBlockFilterIndex(filter_type)->Start()) {
+        if (!GetBlockFilterIndex(filter_type)->Start(::ChainstateActive())) {
             return false;
         }
     }
 
     if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
         g_coin_stats_index = std::make_unique<CoinStatsIndex>(/* cache size */ 0, false, fReindex);
-        if (!g_coin_stats_index->Start()) {
+        if (!g_coin_stats_index->Start(::ChainstateActive())) {
             return false;
         }
     }

src/miner.cpp

@@ -39,13 +39,21 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
     return nNewTime - nOldTime;
 }
 
-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 {
     CMutableTransaction tx{*block.vtx.at(0)};
     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
     block.vtx.at(0) = MakeTransactionRef(tx);
 
-    WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));
+    CBlockIndex* prev_block;
+    {
+        // TODO: Temporary scope to check correctness of refactored code.
+        // Should be removed manually after merge of
+        // https://github.com/bitcoin/bitcoin/pull/20158
+        LOCK(::cs_main);
+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));
+        prev_block = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);
+    }
     GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
 
     block.hashMerkleRoot = BlockMerkleRoot(block);

src/miner.h

@@ -203,6 +203,6 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block);
+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
 
 #endif // BITCOIN_MINER_H

src/rest.cpp

@@ -107,6 +107,27 @@ static CTxMemPool* GetMemPool(const std::any& context, HTTPRequest* req)
     return node_context->mempool.get();
 }
 
+/**
+ * Get the node context chainstatemanager.
+ *
+ * @param[in]  req The HTTP request, whose status code will be set if node
+ *                 context chainstatemanager is not found.
+ * @returns        Pointer to the chainstatemanager or nullptr if none found.
+ */
+static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req)
+{
+    auto node_context = util::AnyPtr<NodeContext>(context);
+    if (!node_context || !node_context->chainman) {
+        RESTERR(req, HTTP_INTERNAL_SERVER_ERROR,
+                strprintf("%s:%d (%s)\n"
+                          "Internal bug detected: Chainman disabled or instance not found!\n"
+                          "You may report this issue here: %s\n",
+                          __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT));
+        return nullptr;
+    }
+    return node_context->chainman;
+}
+
 static RetFormat ParseDataFormat(std::string& param, const std::string& strReq)
 {
     const std::string::size_type pos = strReq.rfind('.');
@@ -181,7 +202,9 @@ static bool rest_headers(const std::any& context,
     std::vector<const CBlockIndex *> headers;
     headers.reserve(count);
     {
-        ChainstateManager& chainman = EnsureAnyChainman(context);
+        ChainstateManager* maybe_chainman = GetChainman(context, req);
+        if (!maybe_chainman) return false;
+        ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         CChain& active_chain = chainman.ActiveChain();
         tip = active_chain.Tip();
@@ -252,7 +275,9 @@ static bool rest_block(const std::any& context,
     CBlockIndex* pblockindex = nullptr;
     CBlockIndex* tip = nullptr;
     {
-        ChainstateManager& chainman = EnsureAnyChainman(context);
+        ChainstateManager* maybe_chainman = GetChainman(context, req);
+        if (!maybe_chainman) return false;
+        ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         tip = chainman.ActiveChain().Tip();
         pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
@@ -541,7 +566,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
     std::string bitmapStringRepresentation;
     std::vector<bool> hits;
     bitmap.resize((vOutPoints.size() + 7) / 8);
-    ChainstateManager& chainman = EnsureAnyChainman(context);
+    ChainstateManager* maybe_chainman = GetChainman(context, req);
+    if (!maybe_chainman) return false;
+    ChainstateManager& chainman = *maybe_chainman;
     {
         auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
             for (const COutPoint& vOutPoint : vOutPoints) {
@@ -644,7 +671,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
 
     CBlockIndex* pblockindex = nullptr;
     {
-        ChainstateManager& chainman = EnsureAnyChainman(context);
+        ChainstateManager* maybe_chainman = GetChainman(context, req);
+        if (!maybe_chainman) return false;
+        ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         const CChain& active_chain = chainman.ActiveChain();
         if (blockheight > active_chain.Height()) {