Merge #17407: node: Add reference to mempool in NodeContext

fa53881 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke)
8888ad0 test: Replace recursive lock with locking annotations (MarcoFalke)
fac07f2 node: Add reference to mempool in NodeContext (MarcoFalke)

Pull request description:

  This is the first step toward making the mempool a global that is not initialized before main.

  #### Motivation

  Currently the mempool is a global that is initialized before the `main` function. This is confusing and easy to get wrong. E.g. the mempool constructor queries state that has not been initialized, like randomness (fixed), or command line arguments (not an issue last time I checked). Also without having the chainstate (chain tip) initialized first, it doesn't make conceptually sense to have a mempool, since the mempool builds txs on top of the utxo set (chain tip).

  Finally, in the future someone might want to run a consensus-only full node (`-nowallet -noblockfilter -no... -nomempool` command line options) that only verifies blocks and updates the utxo set.

  This is conceptually the same change that has already been done for the connection manager `CConnman`.

ACKs for top commit:
  jnewbery:
    utACK fa53881
  ariard:
    Tested ACK fa53881.

Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694

Author: MarcoFalke

src/init.cpp

@@ -284,6 +284,7 @@ void Shutdown(NodeContext& node)
     GetMainSignals().UnregisterWithMempoolSignals(mempool);
     globalVerifyHandle.reset();
     ECC_Stop();
+    if (node.mempool) node.mempool = nullptr;
     LogPrintf("%s: done\n", __func__);
 }
 
@@ -1635,6 +1636,11 @@ bool AppInitMain(NodeContext& node)
         return false;
     }
 
+    // Now that the chain state is loaded, make mempool generally available in the node context. For example the
+    // connection manager, wallet, or RPC threads, which are all started after this, may use it from the node context.
+    assert(!node.mempool);
+    node.mempool = &::mempool;
+
     fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
     CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
     // Allowed to fail as this file IS missing on first startup.

src/node/context.h

@@ -10,6 +10,7 @@
 
 class BanMan;
 class CConnman;
+class CTxMemPool;
 class PeerLogicValidation;
 namespace interfaces {
 class Chain;
@@ -22,13 +23,13 @@ class ChainClient;
 //! This is used by init, rpc, and test code to pass object references around
 //! without needing to declare the same variables and parameters repeatedly, or
 //! to use globals. More variables could be added to this struct (particularly
-//! references to validation and mempool objects) to eliminate use of globals
+//! references to validation objects) to eliminate use of globals
 //! and make code more modular and testable. The struct isn't intended to have
 //! any member functions. It should just be a collection of references that can
 //! be used without pulling in unwanted dependencies or functionality.
-struct NodeContext
-{
+struct NodeContext {
     std::unique_ptr<CConnman> connman;
+    CTxMemPool* mempool{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
     std::unique_ptr<PeerLogicValidation> peer_logic;
     std::unique_ptr<BanMan> banman;
     std::unique_ptr<interfaces::Chain> chain;

src/qt/test/rpcnestedtests.cpp

@@ -32,7 +32,6 @@ void RPCNestedTests::rpcNestedTests()
     // do some test setup
     // could be moved to a more generic place when we add more tests on QT level
     tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]);
-    //mempool.setSanityCheck(1.0);
 
     TestingSetup test;
 

src/rpc/blockchain.cpp

@@ -15,6 +15,7 @@
 #include <hash.h>
 #include <index/blockfilterindex.h>
 #include <node/coinstats.h>
+#include <node/context.h>
 #include <node/utxo_snapshot.h>
 #include <policy/feerate.h>
 #include <policy/policy.h>
@@ -53,6 +54,15 @@ static Mutex cs_blockchange;
 static std::condition_variable cond_blockchange;
 static CUpdatedBlock latestblock;
 
+CTxMemPool& EnsureMemPool()
+{
+    CHECK_NONFATAL(g_rpc_node);
+    if (!g_rpc_node->mempool) {
+        throw JSONRPCError(RPC_CLIENT_MEMPOOL_DISABLED, "Mempool disabled or instance not found");
+    }
+    return *g_rpc_node->mempool;
+}
+
 /* Calculate the difficulty for a given block index.
  */
 double GetDifficulty(const CBlockIndex* blockindex)

src/rpc/blockchain.h

@@ -52,4 +52,6 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
 //! direct way to pass in state to RPC methods without globals.
 extern NodeContext* g_rpc_node;
 
+CTxMemPool& EnsureMemPool();
+
 #endif

src/rpc/protocol.h

@@ -63,6 +63,9 @@ enum RPCErrorCode
     RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet
     RPC_CLIENT_P2P_DISABLED         = -31, //!< No valid connection manager instance found
 
+    //! Chain errors
+    RPC_CLIENT_MEMPOOL_DISABLED     = -33, //!< No mempool instance found
+
     //! Wallet errors
     RPC_WALLET_ERROR                = -4,  //!< Unspecified problem with wallet (key not found etc.)
     RPC_WALLET_INSUFFICIENT_FUNDS   = -6,  //!< Not enough funds in wallet or account

src/test/miner_tests.cpp

@@ -34,17 +34,17 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
 
     LOCK(cs_main);
 
-    unsigned int initialPoolSize = mempool.size();
+    unsigned int initialPoolSize = m_node.mempool->size();
 
     BOOST_CHECK_EQUAL(
             false,
-            AcceptToMemoryPool(mempool, state, MakeTransactionRef(coinbaseTx),
+            AcceptToMemoryPool(*m_node.mempool, state, MakeTransactionRef(coinbaseTx),
                 nullptr /* plTxnReplaced */,
                 true /* bypass_limits */,
                 0 /* nAbsurdFee */));
 
     // Check that the transaction hasn't been added to mempool.
-    BOOST_CHECK_EQUAL(mempool.size(), initialPoolSize);
+    BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize);
 
     // Check that the validation state reflects the unsuccessful attempt.
     BOOST_CHECK(state.IsInvalid());

src/test/txvalidation_tests.cpp

@@ -17,16 +17,6 @@ bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsV
 
 BOOST_AUTO_TEST_SUITE(tx_validationcache_tests)
 
-static bool
-ToMemPool(const CMutableTransaction& tx)
-{
-    LOCK(cs_main);
-
-    TxValidationState state;
-    return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx),
-                              nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */);
-}
-
 BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 {
     // Make sure skipping validation of transactions that were
@@ -35,6 +25,14 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 
     CScript scriptPubKey = CScript() <<  ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
 
+    const auto ToMemPool = [this](const CMutableTransaction& tx) {
+        LOCK(cs_main);
+
+        TxValidationState state;
+        return AcceptToMemoryPool(*m_node.mempool, state, MakeTransactionRef(tx),
+            nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */);
+    };
+
     // Create a double-spend of mature coinbase txn:
     std::vector<CMutableTransaction> spends;
     spends.resize(2);
@@ -72,7 +70,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
         LOCK(cs_main);
         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
     }
-    mempool.clear();
+    m_node.mempool->clear();
 
     // Test 3: ... and should be rejected if spend2 is in the memory pool
     BOOST_CHECK(ToMemPool(spends[1]));
@@ -81,9 +79,9 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
         LOCK(cs_main);
         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
     }
-    mempool.clear();
+    m_node.mempool->clear();
 
-    // Final sanity test: first spend in mempool, second in block, that's OK:
+    // Final sanity test: first spend in *m_node.mempool, second in block, that's OK:
     std::vector<CMutableTransaction> oneSpend;
     oneSpend.push_back(spends[0]);
     BOOST_CHECK(ToMemPool(spends[1]));
@@ -94,7 +92,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
     }
     // spends[1] should have been removed from the mempool when the
     // block with spends[0] is accepted:
-    BOOST_CHECK_EQUAL(mempool.size(), 0U);
+    BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U);
 }
 
 // Run CheckInputs (using CoinsTip()) on the given transaction, for all script

src/test/txvalidationcache_tests.cpp

@@ -107,7 +107,6 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
     threadGroup.create_thread(std::bind(&CScheduler::serviceQueue, &scheduler));
     GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
 
-    mempool.setSanityCheck(1.0);
     pblocktree.reset(new CBlockTreeDB(1 << 20, true));
     g_chainstate = MakeUnique<CChainState>();
     ::ChainstateActive().InitCoinsDB(
@@ -131,6 +130,8 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
     }
     g_parallel_script_checks = true;
 
+    m_node.mempool = &::mempool;
+    m_node.mempool->setSanityCheck(1.0);
     m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
 }
@@ -144,6 +145,7 @@ TestingSetup::~TestingSetup()
     g_rpc_node = nullptr;
     m_node.connman.reset();
     m_node.banman.reset();
+    m_node.mempool = nullptr;
     UnloadBlockIndex();
     g_chainstate.reset();
     pblocktree.reset();