refactor: Remove confusing BlockIndex global

Summary:
> The global ::BlockIndex() is problematic for several reasons:
> - It returns a mutable reference to the block tree, without the appropriate lock annotation (m_block_index is guarded by cs_main). The current code is fine, but in the future this might lead to accidental races and data corruption.
> - The rpc server shouldn't rely on node globals, but rather a context that is passed in to the RPC method.
> - Tests might want to spin up their own block tree, and thus should also not rely on a single global.
>
> Fix all issues by removing the global

This is a backport of [[bitcoin/bitcoin#19413 | core#19413]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9581

src/init.cpp

@@ -2572,7 +2572,7 @@ bool AppInitMain(Config &config, RPCServer &rpcServer,
                 // If the loaded chain has a wrong genesis, bail out immediately
                 // (we're likely using a testnet datadir, or the other way
                 // around).
-                if (!::BlockIndex().empty() &&
+                if (!chainman.BlockIndex().empty() &&
                     !LookupBlockIndex(params.hashGenesisBlock)) {
                     return InitError(_("Incorrect or no genesis block found. "
                                        "Wrong datadir for network?"));
@@ -2872,8 +2872,8 @@ bool AppInitMain(Config &config, RPCServer &rpcServer,
     //// debug print
     {
         LOCK(cs_main);
-        LogPrintf("block tree size = %u\n", ::BlockIndex().size());
-        chain_active_height = ::ChainActive().Height();
+        LogPrintf("block tree size = %u\n", chainman.BlockIndex().size());
+        chain_active_height = chainman.ActiveChain().Height();
     }
     LogPrintf("nBestHeight = %d\n", chain_active_height);
 

src/rpc/blockchain.cpp

@@ -1635,24 +1635,25 @@ static UniValue getchaintips(const Config &config,
     }
         .Check(request);
 
+    ChainstateManager &chainman = EnsureChainman(request.context);
     LOCK(cs_main);
 
     /**
-     * Idea:  the set of chain tips is ::ChainActive().tip, plus orphan blocks
+     * Idea: The set of chain tips is the active chain tip, plus orphan blocks
      * which do not have another orphan building off of them. Algorithm:
      *  - Make one pass through BlockIndex(), picking out the orphan
      * blocks, and also storing a set of the orphan block's pprev pointers.
      *  - Iterate through the orphan blocks. If the block isn't pointed to by
      * another orphan, it is a chain tip.
-     *  - add ::ChainActive().Tip()
+     *  - Add the active chain tip
      */
     std::set<const CBlockIndex *, CompareBlocksByHeight> setTips;
     std::set<const CBlockIndex *> setOrphans;
     std::set<const CBlockIndex *> setPrevs;
 
     for (const std::pair<const BlockHash, CBlockIndex *> &item :
-         ::BlockIndex()) {
-        if (!::ChainActive().Contains(item.second)) {
+         chainman.BlockIndex()) {
+        if (!chainman.ActiveChain().Contains(item.second)) {
             setOrphans.insert(item.second);
             setPrevs.insert(item.second->pprev);
         }
@@ -1666,7 +1667,7 @@ static UniValue getchaintips(const Config &config,
     }
 
     // Always report the currently active tip.
-    setTips.insert(::ChainActive().Tip());
+    setTips.insert(chainman.ActiveChain().Tip());
 
     /* Construct the output array.  */
     UniValue res(UniValue::VARR);
@@ -1676,11 +1677,11 @@ static UniValue getchaintips(const Config &config,
         obj.pushKV("hash", block->phashBlock->GetHex());
 
         const int branchLen =
-            block->nHeight - ::ChainActive().FindFork(block)->nHeight;
+            block->nHeight - chainman.ActiveChain().FindFork(block)->nHeight;
         obj.pushKV("branchlen", branchLen);
 
         std::string status;
-        if (::ChainActive().Contains(block)) {
+        if (chainman.ActiveChain().Contains(block)) {
             // This block is part of the currently active chain.
             status = "active";
         } else if (block->nStatus.isInvalid()) {

src/validation.cpp

@@ -935,11 +935,6 @@ bool CChainState::IsInitialBlockDownload() const {
 static CBlockIndex const *pindexBestForkTip = nullptr;
 static CBlockIndex const *pindexBestForkBase = nullptr;
 
-BlockMap &BlockIndex() {
-    LOCK(::cs_main);
-    return g_chainman.m_blockman.m_block_index;
-}
-
 static void AlertNotify(const std::string &strMessage) {
     uiInterface.NotifyAlertChanged();
 #if defined(HAVE_SYSTEM)

src/validation.h

@@ -1224,9 +1224,6 @@ CChainState &ChainstateActive();
 /** Please prefer the identical ChainstateManager::ActiveChain */
 CChain &ChainActive();
 
-/** Please prefer the identical ChainstateManager::BlockIndex */
-BlockMap &BlockIndex();
-
 /**
  * Global variable that points to the active block tree (protected by cs_main)
  */

src/wallet/test/wallet_tests.cpp

@@ -373,16 +373,17 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) {
     BOOST_CHECK_EQUAL(wtx.GetImmatureCredit(), 50 * COIN);
 }
 
-static int64_t AddTx(CWallet &wallet, uint32_t lockTime, int64_t mockTime,
-                     int64_t blockTime) {
+static int64_t AddTx(ChainstateManager &chainman, CWallet &wallet,
+                     uint32_t lockTime, int64_t mockTime, int64_t blockTime) {
     CMutableTransaction tx;
     CWalletTx::Confirmation confirm;
     tx.nLockTime = lockTime;
     SetMockTime(mockTime);
     CBlockIndex *block = nullptr;
     if (blockTime > 0) {
-        auto inserted =
-            ::BlockIndex().emplace(BlockHash(GetRandHash()), new CBlockIndex);
+        LOCK(cs_main);
+        auto inserted = chainman.BlockIndex().emplace(BlockHash(GetRandHash()),
+                                                      new CBlockIndex);
         assert(inserted.second);
         const BlockHash &hash = inserted.first->first;
         block = inserted.first->second;
@@ -406,24 +407,24 @@ static int64_t AddTx(CWallet &wallet, uint32_t lockTime, int64_t mockTime,
 // expanded to cover more corner cases of smart time logic.
 BOOST_AUTO_TEST_CASE(ComputeTimeSmart) {
     // New transaction should use clock time if lower than block time.
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 1, 100, 120), 100);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 1, 100, 120), 100);
 
     // Test that updating existing transaction does not change smart time.
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 1, 200, 220), 100);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 1, 200, 220), 100);
 
     // New transaction should use clock time if there's no block time.
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 2, 300, 0), 300);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 2, 300, 0), 300);
 
     // New transaction should use block time if lower than clock time.
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 3, 420, 400), 400);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 3, 420, 400), 400);
 
     // New transaction should use latest entry time if higher than
     // min(block time, clock time).
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 4, 500, 390), 400);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 4, 500, 390), 400);
 
     // If there are future entries, new transaction should use time of the
     // newest entry that is no more than 300 seconds ahead of the clock time.
-    BOOST_CHECK_EQUAL(AddTx(m_wallet, 5, 50, 600), 300);
+    BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 5, 50, 600), 300);
 
     // Reset mock time for other tests.
     SetMockTime(0);