pool: Add and use MemPoolOptions, ApplyArgsManOptions

Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.

We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).

1. We first set the default based on chainparams's
   DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million

With this patch, most CTxMemPool construction are along the lines of:

    MemPoolOptions mempool_opts{...default overrides...};
    ApplyArgsManOptions(argsman, mempool_opts);
    ...hard overrides...
    CTxMemPool pool{mempool_opts};

This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.

For example:

- For users of libbitcoinkernel, where we eventually want to remove
  ArgsManager, they simply won't call (or even know about)
  ApplyArgsManOptions.

- See src/init.cpp to see how the check_ratio CTxMemPool option works
  after this change.

A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.

The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.

[META] As this patchset progresses, we will move more and more
       CTxMemPool-relevant options into MemPoolOptions and add their
       ArgsMan-related logic to ApplyArgsManOptions.

12 files changed, 127 insertions, 16 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index fa716af619..bdb279d176 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -173,11 +173,13 @@ BITCOIN_CORE_H = \
   kernel/checks.h \
   kernel/coinstats.h \
   kernel/context.h \
+  kernel/mempool_options.h \
   key.h \
   key_io.h \
   logging.h \
   logging/timer.h \
   mapport.h \
+  mempool_args.h \
   memusage.h \
   merkleblock.h \
   net.h \
@@ -361,6 +363,7 @@ libbitcoin_node_a_SOURCES = \
   kernel/coinstats.cpp \
   kernel/context.cpp \
   mapport.cpp \
+  mempool_args.cpp \
   net.cpp \
   netgroup.cpp \
   net_processing.cpp \
diff --git a/src/init.cpp b/src/init.cpp
index dfbcff4b1d..3ad4dc2667 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -30,6 +30,7 @@
 #include <interfaces/init.h>
 #include <interfaces/node.h>
 #include <mapport.h>
+#include <mempool_args.h>
 #include <net.h>
 #include <net_permissions.h>
 #include <net_processing.h>
@@ -62,6 +63,7 @@
 #include <txorphanage.h>
 #include <util/asmap.h>
 #include <util/check.h>
+#include <util/designator.h>
 #include <util/moneystr.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -1426,10 +1428,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     assert(!node.mempool);
     assert(!node.chainman);
-    const int mempool_check_ratio = std::clamp<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0, 1000000);
+
+    CTxMemPool::Options mempool_opts{
+        Desig(estimator) node.fee_estimator.get(),
+        Desig(check_ratio) chainparams.DefaultConsistencyChecks() ? 1 : 0,
+    };
+    ApplyArgsManOptions(args, mempool_opts);
+    mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1'000'000);
 
     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
-        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
+        node.mempool = std::make_unique<CTxMemPool>(mempool_opts);
 
         const ChainstateManager::Options chainman_opts{
             chainparams,
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
new file mode 100644
index 0000000000..c3d54bf0f4
--- /dev/null
+++ b/src/kernel/mempool_options.h
@@ -0,0 +1,25 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#ifndef BITCOIN_KERNEL_MEMPOOL_OPTIONS_H
+#define BITCOIN_KERNEL_MEMPOOL_OPTIONS_H
+
+class CBlockPolicyEstimator;
+
+namespace kernel {
+/**
+ * Options struct containing options for constructing a CTxMemPool. Default
+ * constructor populates the struct with sane default values which can be
+ * modified.
+ *
+ * Most of the time, this struct should be referenced as CTxMemPool::Options.
+ */
+struct MemPoolOptions {
+    /* Used to estimate appropriate transaction fees. */
+    CBlockPolicyEstimator* estimator{nullptr};
+    /* The ratio used to determine how often sanity checks will run.  */
+    int check_ratio{0};
+};
+} // namespace kernel
+
+#endif // BITCOIN_KERNEL_MEMPOOL_OPTIONS_H
diff --git a/src/mempool_args.cpp b/src/mempool_args.cpp
new file mode 100644
index 0000000000..578463b81a
--- /dev/null
+++ b/src/mempool_args.cpp
@@ -0,0 +1,16 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <mempool_args.h>
+
+#include <kernel/mempool_options.h>
+
+#include <util/system.h>
+
+using kernel::MemPoolOptions;
+
+void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts)
+{
+    mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
+}
diff --git a/src/mempool_args.h b/src/mempool_args.h
new file mode 100644
index 0000000000..1732bdaac9
--- /dev/null
+++ b/src/mempool_args.h
@@ -0,0 +1,15 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_MEMPOOL_ARGS_H
+#define BITCOIN_MEMPOOL_ARGS_H
+
+class ArgsManager;
+namespace kernel {
+struct MemPoolOptions;
+};
+
+void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts);
+
+#endif // BITCOIN_MEMPOOL_ARGS_H
diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
index d4ec6ecfb1..4801635791 100644
--- a/src/test/fuzz/rbf.cpp
+++ b/src/test/fuzz/rbf.cpp
@@ -2,6 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <mempool_args.h>
 #include <policy/rbf.h>
 #include <primitives/transaction.h>
 #include <sync.h>
@@ -34,8 +35,11 @@ FUZZ_TARGET_INIT(rbf, initialize_rbf)
     if (!mtx) {
         return;
     }
-    CTxMemPool pool;
-    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
+
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
+    {
         const std::optional<CMutableTransaction> another_mtx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider);
         if (!another_mtx) {
             break;
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 4f40608c4f..2d88ee295b 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -3,6 +3,8 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <consensus/validation.h>
+#include <mempool_args.h>
+#include <node/context.h>
 #include <node/miner.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
@@ -15,6 +17,7 @@
 #include <validationinterface.h>
 
 using node::BlockAssembler;
+using node::NodeContext;
 
 namespace {
 
@@ -121,6 +124,19 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const CChainState& chain
     SetMockTime(time);
 }
 
+CTxMemPool MakeMempool(const NodeContext& node)
+{
+    // Take the default options for tests...
+    CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
+
+    // ...override specific options for this specific fuzz suite
+    mempool_opts.estimator = nullptr;
+    mempool_opts.check_ratio = 1;
+
+    // ...and construct a CTxMemPool from it
+    return CTxMemPool{mempool_opts};
+}
+
 FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
@@ -142,7 +158,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
     // The sum of the values of all spendable outpoints
     constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN};
 
-    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
+    CTxMemPool tx_pool_{MakeMempool(node)};
     MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
 
     chainstate.SetMempool(&tx_pool);
@@ -320,7 +336,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool)
         txids.push_back(ConsumeUInt256(fuzzed_data_provider));
     }
 
-    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
+    CTxMemPool tx_pool_{MakeMempool(node)};
     MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp
index c2aaf486c5..9532610f8d 100644
--- a/src/test/fuzz/validation_load_mempool.cpp
+++ b/src/test/fuzz/validation_load_mempool.cpp
@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparamsbase.h>
+#include <mempool_args.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
@@ -30,7 +31,8 @@ FUZZ_TARGET_INIT(validation_load_mempool, initialize_validation_load_mempool)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
 
-    CTxMemPool pool{};
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+
     auto fuzzed_fopen = [&](const fs::path&, const char*) {
         return fuzzed_file_provider.open();
     };
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index d9fff85bf5..60ddda1c0a 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -14,10 +14,12 @@
 #include <init.h>
 #include <init/common.h>
 #include <interfaces/chain.h>
+#include <mempool_args.h>
 #include <net.h>
 #include <net_processing.h>
 #include <node/blockstorage.h>
 #include <node/chainstate.h>
+#include <node/context.h>
 #include <node/miner.h>
 #include <noui.h>
 #include <policy/fees.h>
@@ -32,6 +34,8 @@
 #include <test/util/net.h>
 #include <timedata.h>
 #include <txdb.h>
+#include <txmempool.h>
+#include <util/designator.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/thread.h>
@@ -50,11 +54,12 @@
 
 using node::BlockAssembler;
 using node::CalculateCacheSizes;
+using node::fPruneMode;
+using node::fReindex;
 using node::LoadChainstate;
+using node::NodeContext;
 using node::RegenerateCommitments;
 using node::VerifyLoadedChainstate;
-using node::fPruneMode;
-using node::fReindex;
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 UrlDecodeFn* const URL_DECODE = nullptr;
@@ -149,6 +154,18 @@ BasicTestingSetup::~BasicTestingSetup()
     gArgs.ClearArgs();
 }
 
+CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node)
+{
+    CTxMemPool::Options mempool_opts{
+        Desig(estimator) node.fee_estimator.get(),
+        // Default to always checking mempool regardless of
+        // chainparams.DefaultConsistencyChecks for tests
+        Desig(check_ratio) 1,
+    };
+    ApplyArgsManOptions(*node.args, mempool_opts);
+    return mempool_opts;
+}
+
 ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
     : BasicTestingSetup(chainName, extra_args)
 {
@@ -161,7 +178,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
 
     m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
-    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), m_node.args->GetIntArg("-checkmempool", 1));
+    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node));
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 37407bcb92..ed2c5db7e6 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -90,6 +90,9 @@ struct BasicTestingSetup {
     ArgsManager m_args;
 };
 
+
+CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node);
+
 /** Testing setup that performs all steps up until right before
  * ChainstateManager gets initialized. Meant for testing ChainstateManager
  * initialization behaviour.
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 65c8b4ea60..84088aead6 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -451,8 +451,9 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee,
     assert(int(nSigOpCostWithAncestors) >= 0);
 }
 
-CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio)
-    : m_check_ratio(check_ratio), minerPolicyEstimator(estimator)
+CTxMemPool::CTxMemPool(const Options& opts)
+    : m_check_ratio{opts.check_ratio},
+      minerPolicyEstimator{opts.estimator}
 {
     _clear(); //lock free clear
 }
diff --git a/src/txmempool.h b/src/txmempool.h
index f5d5abc62e..32319ac181 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -14,6 +14,8 @@
 #include <utility>
 #include <vector>
 
+#include <kernel/mempool_options.h>
+
 #include <coins.h>
 #include <consensus/amount.h>
 #include <indirectmap.h>
@@ -560,15 +562,14 @@ public:
     indirectmap<COutPoint, const CTransaction*> mapNextTx GUARDED_BY(cs);
     std::map<uint256, CAmount> mapDeltas GUARDED_BY(cs);
 
+    using Options = kernel::MemPoolOptions;
+
     /** Create a new CTxMemPool.
      * Sanity checks will be off by default for performance, because otherwise
      * accepting transactions becomes O(N^2) where N is the number of transactions
      * in the pool.
-     *
-     * @param[in] estimator is used to estimate appropriate transaction fees.
-     * @param[in] check_ratio is the ratio used to determine how often sanity checks will run.
      */
-    explicit CTxMemPool(CBlockPolicyEstimator* estimator = nullptr, int check_ratio = 0);
+    explicit CTxMemPool(const Options& opts);
 
     /**
      * If sanity-checking is turned on, check makes sure the pool is