Merge bitcoin/bitcoin#21866: [Bundle 7/7] validation: Farewell, global Chainstate!

6f99488 validation: Farewell, global Chainstate! (Carl Dong)
972c516 qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb tree-wide: Remove stray review-only assertion (Carl Dong)
f323248 qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de1 scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634 scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313 init: Use existing chainman (Carl Dong)

Pull request description:

  Based on:  #21767

  à la Mr. Sandman
  ```
  Mr. Chainman, bring me a tip (bung, bung, bung, bung)
  Make it the most work that I've ever seen (bung, bung, bung, bung)
  Rewind old tip till we're at the fork point (bung, bung, bung, bung)
  Then tell it that it's time to call Con-nectTip

  Chainman, I'm so alone (bung, bung, bung, bung)
  No local objects to call my own (bung, bung, bung, bung)
  Please make sure I have a ref
  Mr. Chainman, bring me a tip!
  ```

  This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
  I would like to call attention to bitcoin/bitcoin#21766, where a few leftover improvements were collated.

  - Remove globals:
    - `ChainstateManager g_chainman`
    - `CChainState& ChainstateActive()`
    - `CChain& ChainActive()`
  - Remove all review-only assertions.

ACKs for top commit:
  jamesob:
    reACK bitcoin/bitcoin@6f99488 based on the contents of
  ariard:
    Code Review ACK 6f99488.
  jnewbery:
    utACK 6f99488
  achow101:
    Code Review ACK 6f99488
  ryanofsky:
    Code review ACK 6f99488.

Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6

Author: fanquake

src/bench/duplicate_inputs.cpp

@@ -25,7 +25,6 @@ static void DuplicateInputs(benchmark::Bench& bench)
     CMutableTransaction naughtyTx{};
 
     LOCK(cs_main);
-    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());

src/index/base.cpp

@@ -340,7 +340,6 @@ void BaseIndex::Interrupt()
 
 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.

src/init.cpp

@@ -283,7 +283,7 @@ void Shutdown(NodeContext& node)
     init::UnsetGlobals();
     node.mempool.reset();
     node.fee_estimator.reset();
-    node.chainman = nullptr;
+    node.chainman.reset();
     node.scheduler.reset();
 
     try {
@@ -1175,8 +1175,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), check_ratio);
 
     assert(!node.chainman);
-    node.chainman = &g_chainman;
-    ChainstateManager& chainman = *Assert(node.chainman);
+    node.chainman = std::make_unique<ChainstateManager>();
+    ChainstateManager& chainman = *node.chainman;
 
     assert(!node.peerman);
     node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(),
@@ -1381,7 +1381,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                 // If the loaded chain has a wrong genesis, bail out immediately
                 // (we're likely using a testnet datadir, or the other way around).
                 if (!chainman.BlockIndex().empty() &&
-                        !g_chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
+                        !chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
                     return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
                 }
 
@@ -1396,7 +1396,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                 // If we're not mid-reindex (based on disk + args), add a genesis block on disk
                 // (otherwise we use the one already on disk).
                 // This is called again in ThreadImport after the reindex completes.
-                if (!fReindex && !::ChainstateActive().LoadGenesisBlock(chainparams)) {
+                if (!fReindex && !chainman.ActiveChainstate().LoadGenesisBlock(chainparams)) {
                     strLoadError = _("Error initializing block database");
                     break;
                 }
@@ -1545,21 +1545,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(::ChainstateActive())) {
+        if (!g_txindex->Start(chainman.ActiveChainstate())) {
             return false;
         }
     }
 
     for (const auto& filter_type : g_enabled_filter_types) {
         InitBlockFilterIndex(filter_type, filter_index_cache, false, fReindex);
-        if (!GetBlockFilterIndex(filter_type)->Start(::ChainstateActive())) {
+        if (!GetBlockFilterIndex(filter_type)->Start(chainman.ActiveChainstate())) {
             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(::ChainstateActive())) {
+        if (!g_coin_stats_index->Start(chainman.ActiveChainstate())) {
             return false;
         }
     }
@@ -1607,7 +1607,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
     // No locking, as this happens before any background thread is started.
     boost::signals2::connection block_notify_genesis_wait_connection;
-    if (::ChainActive().Tip() == nullptr) {
+    if (chainman.ActiveChain().Tip() == nullptr) {
         block_notify_genesis_wait_connection = uiInterface.NotifyBlockTip_connect(std::bind(BlockNotifyGenesisWait, std::placeholders::_2));
     } else {
         fHaveGenesis = true;

src/miner.cpp

@@ -45,15 +45,7 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
     block.vtx.at(0) = MakeTransactionRef(tx);
 
-    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);
-    }
+    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
     GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
 
     block.hashMerkleRoot = BlockMerkleRoot(block);
@@ -124,7 +116,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
 
     LOCK2(cs_main, m_mempool.cs);
-    assert(std::addressof(*::ChainActive().Tip()) == std::addressof(*m_chainstate.m_chain.Tip()));
     CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
     assert(pindexPrev != nullptr);
     nHeight = pindexPrev->nHeight + 1;
@@ -184,7 +175,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
 
     BlockValidationState state;
-    assert(std::addressof(::ChainstateActive()) == std::addressof(m_chainstate));
     if (!TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, false, false)) {
         throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
     }

src/net_processing.cpp

@@ -1361,7 +1361,6 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
       m_stale_tip_check_time(0),
       m_ignore_incoming_txs(ignore_incoming_txs)
 {
-    assert(std::addressof(g_chainman) == std::addressof(m_chainman));
     // Initialize global variables that cannot be constructed at startup.
     recentRejects.reset(new CRollingBloomFilter(120000, 0.000001));
 

src/node/blockstorage.cpp

@@ -248,7 +248,6 @@ bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight,
             // when the undo file is keeping up with the block file, we want to flush it explicitly
             // when it is lagging behind (more blocks arrive than are being connected), we let the
             // undo block write case handle it
-            assert(std::addressof(::ChainActive()) == std::addressof(active_chain));
             finalize_undo = (vinfoBlockFile[nFile].nHeightLast == (unsigned int)active_chain.Tip()->nHeight);
             nFile++;
             if (vinfoBlockFile.size() <= nFile) {

src/node/coin.cpp

@@ -13,7 +13,6 @@ void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
     assert(node.mempool);
     assert(node.chainman);
     LOCK2(cs_main, node.mempool->cs);
-    assert(std::addressof(::ChainstateActive()) == std::addressof(node.chainman->ActiveChainstate()));
     CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip();
     CCoinsViewMemPool mempool_view(&chain_view, *node.mempool);
     for (auto& coin : coins) {

src/node/coinstats.cpp

@@ -97,7 +97,6 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&
     if (!pindex) {
         {
             LOCK(cs_main);
-            assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));
             pindex = blockman.LookupBlockIndex(view->GetBestBlock());
         }
     }

src/node/context.cpp

@@ -12,6 +12,7 @@
 #include <policy/fees.h>
 #include <scheduler.h>
 #include <txmempool.h>
+#include <validation.h>
 
 NodeContext::NodeContext() {}
 NodeContext::~NodeContext() {}

src/node/context.h

@@ -44,7 +44,7 @@ struct NodeContext {
     std::unique_ptr<CTxMemPool> mempool;
     std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
     std::unique_ptr<PeerManager> peerman;
-    ChainstateManager* chainman{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
+    std::unique_ptr<ChainstateManager> chainman;
     std::unique_ptr<BanMan> banman;
     ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
     std::unique_ptr<interfaces::Chain> chain;