Merge #21525: [Bundle 4.5/n] Followup fixups to bundle 4

693414d27181cf967f787a2ca72344e52c58c7f0 node/ifaces: ChainImpl: Use an accessor for ChainMan (Carl Dong)
98c4e252f0d09bebb2e4ad3289407459c2cda5d5 node/ifaces: NodeImpl: Use an accessor for ChainMan (Carl Dong)
7e8b5ee814b0b8c34acb20637ed4fc988ccba555 validation: Make BlockManager::LookupBlockIndex const (Carl Dong)
88aead263c61d86e5f836028f517cfbf2a575498 node: Avoid potential UB by asserting assumptions (Carl Dong)
1dd8ed7a8491e51b76eeb236b15b794d9254f674 net_processing: Move comments to declarations (Carl Dong)
07156eb387ea580be5e2ce4a1744992ce7575903 node/coinstats: Replace #include with fwd-declaration (Carl Dong)
7b8e976cd5ac78a22f1be2b2fed8562c693af5d9 miner: Add chainstate member to BlockAssembler (Carl Dong)
e62067e7bcad5a559899afff2e4a8e8b7e9f4301 Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock" (Carl Dong)
eede0647b06b6009080c4e536a2705e911d6ee19 Revert "scripted-diff: Invoke CreateNewBlock with chainstate" (Carl Dong)
0c1b2bc549aec77b247f0103652d883227841ac5 Revert "miner: Remove old CreateNewBlock w/o chainstate param" (Carl Dong)

Pull request description:

  Chronological history of this changeset:
  1. Bundle 4 (#21270) got merged
  2. Posthumous reviews were posted
  3. These changes were prepended in bundle 5
  4. More reviews were added in bundle 5
  5. Someone suggested that we split the prepended changes up to another PR
  6. This is that PR

  In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion.

  Addresses posthumous reviews on bundle 4:
    - From jnewbery:
      - https://github.com/bitcoin/bitcoin/pull/21270#issuecomment-796738048
        - I didn't fix this one, but I added a `TODO` comment so that we don't lost track of it
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592291225
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592296942
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592299738
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592301704
    - From MarcoFalke:
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593096212
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593097032
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593097867
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593100570

  Addresses reviews on bundle 5:
  - Checking chainman existence before locking cs_main
    - MarcoFalke
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601776
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601876
  - Appropriate locking, usage of chainman, and control flow in `src/node/interfaces.cpp`
    - MarcoFalke
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601383
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597029360
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597029921
    - ryanofsky
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597163828
  - Style/comment formatting changes
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597026552
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597027186
  - Making LookupBlockIndex const
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597035062

ACKs for top commit:
  MarcoFalke:
    review ACK 693414d27181cf967f787a2ca72344e52c58c7f0 🛐
  ryanofsky:
    Code review ACK 693414d27181cf967f787a2ca72344e52c58c7f0. I reviewed this previously as part of #21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667!
  jamesob:
    ACK 693414d27181cf967f787a2ca72344e52c58c7f0 ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f))

Tree-SHA512: 9bdc199f70400d01764e1bd03c25bdb6cff26dcef60e4ca3b649baf8d017a2dfc1f058099067962b4b6ccd32d078002b1389d733039f4c337558cb70324c0ee3

4 files changed, 51 insertions, 22 deletions

diff --git a/src/node/coin.cpp b/src/node/coin.cpp
index 263dcff657..23d4fa2aae 100644
--- a/src/node/coin.cpp
+++ b/src/node/coin.cpp
@@ -11,6 +11,7 @@
 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();
diff --git a/src/node/coinstats.h b/src/node/coinstats.h
index 83f228aa7e..975651dcc4 100644
--- a/src/node/coinstats.h
+++ b/src/node/coinstats.h
@@ -8,11 +8,11 @@
 
 #include <amount.h>
 #include <uint256.h>
-#include <validation.h>
 
 #include <cstdint>
 #include <functional>
 
+class BlockManager;
 class CCoinsView;
 
 enum class CoinStatsHashType {
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 0f4094e14b..7ad02f81dc 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -65,6 +65,8 @@ namespace node {
 namespace {
 class NodeImpl : public Node
 {
+private:
+    ChainstateManager& chainman() { return *Assert(m_context->chainman); }
 public:
     explicit NodeImpl(NodeContext* context) { setContext(context); }
     void initLogging() override { InitLogging(*Assert(m_context->args)); }
@@ -183,21 +185,28 @@ public:
     int getNumBlocks() override
     {
         LOCK(::cs_main);
-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
-        return m_context->chainman->ActiveChain().Height();
+        assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));
+        return chainman().ActiveChain().Height();
     }
     uint256 getBestBlockHash() override
     {
-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
-        const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_context->chainman->ActiveChain().Tip());
+        const CBlockIndex* tip;
+        {
+            // 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(::ChainActive()) == std::addressof(chainman().ActiveChain()));
+            tip = chainman().ActiveChain().Tip();
+        }
         return tip ? tip->GetBlockHash() : Params().GenesisBlock().GetHash();
     }
     int64_t getLastBlockTime() override
     {
         LOCK(::cs_main);
-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
-        if (m_context->chainman->ActiveChain().Tip()) {
-            return m_context->chainman->ActiveChain().Tip()->GetBlockTime();
+        assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));
+        if (chainman().ActiveChain().Tip()) {
+            return chainman().ActiveChain().Tip()->GetBlockTime();
         }
         return Params().GenesisBlock().GetBlockTime(); // Genesis block's time of current network
     }
@@ -206,14 +215,22 @@ public:
         const CBlockIndex* tip;
         {
             LOCK(::cs_main);
-            assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
-            tip = m_context->chainman->ActiveChain().Tip();
+            assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));
+            tip = chainman().ActiveChain().Tip();
         }
         return GuessVerificationProgress(Params().TxData(), tip);
     }
     bool isInitialBlockDownload() override {
-        assert(std::addressof(::ChainstateActive()) == std::addressof(m_context->chainman->ActiveChainstate()));
-        return m_context->chainman->ActiveChainstate().IsInitialBlockDownload();
+        const CChainState* active_chainstate;
+        {
+            // 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);
+            active_chainstate = &m_context->chainman->ActiveChainstate();
+            assert(std::addressof(::ChainstateActive()) == std::addressof(*active_chainstate));
+        }
+        return active_chainstate->IsInitialBlockDownload();
     }
     bool getReindex() override { return ::fReindex; }
     bool getImporting() override { return ::fImporting; }
@@ -239,8 +256,8 @@ public:
     bool getUnspentOutput(const COutPoint& output, Coin& coin) override
     {
         LOCK(::cs_main);
-        assert(std::addressof(::ChainstateActive()) == std::addressof(m_context->chainman->ActiveChainstate()));
-        return m_context->chainman->ActiveChainstate().CoinsTip().GetCoin(output, coin);
+        assert(std::addressof(::ChainstateActive()) == std::addressof(chainman().ActiveChainstate()));
+        return chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin);
     }
     WalletClient& walletClient() override
     {
@@ -414,6 +431,8 @@ public:
 
 class ChainImpl : public Chain
 {
+private:
+    ChainstateManager& chainman() { return *Assert(m_node.chainman); }
 public:
     explicit ChainImpl(NodeContext& node) : m_node(node) {}
     std::optional<int> getHeight() override
@@ -450,8 +469,8 @@ public:
     bool checkFinalTx(const CTransaction& tx) override
     {
         LOCK(cs_main);
-        assert(std::addressof(::ChainActive()) == std::addressof(m_node.chainman->ActiveChain()));
-        return CheckFinalTx(m_node.chainman->ActiveChain().Tip(), tx);
+        assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));
+        return CheckFinalTx(chainman().ActiveChain().Tip(), tx);
     }
     std::optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
@@ -516,8 +535,8 @@ public:
     double guessVerificationProgress(const uint256& block_hash) override
     {
         LOCK(cs_main);
-        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));
-        return GuessVerificationProgress(Params().TxData(), m_node.chainman->m_blockman.LookupBlockIndex(block_hash));
+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman().m_blockman));
+        return GuessVerificationProgress(Params().TxData(), chainman().m_blockman.LookupBlockIndex(block_hash));
     }
     bool hasBlocks(const uint256& block_hash, int min_height, std::optional<int> max_height) override
     {
@@ -529,8 +548,8 @@ public:
         // 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);
-        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));
-        if (CBlockIndex* block = m_node.chainman->m_blockman.LookupBlockIndex(block_hash)) {
+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman().m_blockman));
+        if (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
@@ -621,8 +640,16 @@ public:
     }
     bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !isInitialBlockDownload(); }
     bool isInitialBlockDownload() override {
-        assert(std::addressof(::ChainstateActive()) == std::addressof(m_node.chainman->ActiveChainstate()));
-        return m_node.chainman->ActiveChainstate().IsInitialBlockDownload();
+        const CChainState* active_chainstate;
+        {
+            // 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);
+            active_chainstate = &chainman().ActiveChainstate();
+            assert(std::addressof(::ChainstateActive()) == std::addressof(*active_chainstate));
+        }
+        return active_chainstate->IsInitialBlockDownload();
     }
     bool shutdownRequested() override { return ShutdownRequested(); }
     int64_t getAdjustedTime() override { return GetAdjustedTime(); }
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index f47e85aceb..691b2791d7 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -38,6 +38,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
     bool callback_set = false;
 
     { // cs_main scope
+    assert(node.chainman);
     LOCK(cs_main);
     assert(std::addressof(::ChainstateActive()) == std::addressof(node.chainman->ActiveChainstate()));
     // If the transaction is already confirmed in the chain, don't do anything