[refactor] Check CTxMemPool options in ctor

sedited · sedited · commit 29818f5f839b · 2023-12-01T23:01:14.000+01:00
This ensures that the tests run the same checks on the mempool options
that the init code also applies.
diff --git a/src/init.cpp b/src/init.cpp
@@ -1463,16 +1463,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     if (!result) {
         return InitError(util::ErrorString(result));
     }
-    mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1'000'000);
-
-    int64_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40;
-    if (mempool_opts.max_size_bytes < 0 || mempool_opts.max_size_bytes < descendant_limit_bytes) {
-        return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0)));
-    }
-    LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
 
     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
-        node.mempool = std::make_unique<CTxMemPool>(mempool_opts);
+        std::optional<bilingual_str> mempool_error{};
+        node.mempool = std::make_unique<CTxMemPool>(mempool_opts, mempool_error);
+        if (mempool_error) {
+            return InitError(mempool_error.value());
+        }
+        LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
+
 
         node.chainman = std::make_unique<ChainstateManager>(node.kernel->interrupt, chainman_opts, blockman_opts);
         ChainstateManager& chainman = *node.chainman;
diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
@@ -14,6 +14,7 @@
 #include <txmempool.h>
 
 #include <deque>
+#include <optional>
 #include <vector>
 
 namespace {
@@ -33,7 +34,9 @@ void initialize_miner()
 FUZZ_TARGET(mini_miner, .init = initialize_miner)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-    CTxMemPool pool{CTxMemPool::Options{}};
+    std::optional<bilingual_str> error{};
+    CTxMemPool pool{CTxMemPool::Options{}, error};
+    Assert(!error);
     std::vector<COutPoint> outpoints;
     std::deque<COutPoint> available_coins = g_available_coins;
     LOCK2(::cs_main, pool.cs);
@@ -109,7 +112,9 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner)
 FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-    CTxMemPool pool{CTxMemPool::Options{}};
+    std::optional<bilingual_str> error{};
+    CTxMemPool pool{CTxMemPool::Options{}, error};
+    Assert(!error);
     // Make a copy to preserve determinism.
     std::deque<COutPoint> available_coins = g_available_coins;
     std::vector<CTransactionRef> transactions;
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
@@ -125,7 +125,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
     mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 
     // ...and construct a CTxMemPool from it
-    return CTxMemPool{mempool_opts};
+    std::optional<bilingual_str> error{};
+    return CTxMemPool{mempool_opts, error};
+    Assert(!error);
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
@@ -52,7 +52,9 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
 
     CBlockHeaderAndShortTxIDs cmpctblock{*block};
 
-    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+    std::optional<bilingual_str> error{};
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
+    Assert(!error);
     PartiallyDownloadedBlock pdb{&pool};
 
     // Set of available transactions (mempool or extra_txn)
diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
@@ -38,7 +38,9 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
         return;
     }
 
-    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+    std::optional<bilingual_str> error{};
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
+    Assert(!error);
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
     {
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
@@ -127,7 +127,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
     mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 
     // ...and construct a CTxMemPool from it
-    return CTxMemPool{mempool_opts};
+    std::optional<bilingual_str> error{};
+    return CTxMemPool{mempool_opts, error};
+    Assert(!error);
 }
 
 void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, bool wtxid_in_mempool)
diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp
@@ -40,7 +40,9 @@ FUZZ_TARGET(validation_load_mempool, .init = initialize_validation_load_mempool)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider};
 
-    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+    std::optional<bilingual_str> error{};
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
+    Assert(!error);
 
     auto& chainstate{static_cast<DummyChainState&>(g_setup->m_node.chainman->ActiveChainstate())};
     chainstate.SetMempool(&pool);
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
@@ -23,6 +23,7 @@
 #include <test/util/setup_common.h>
 
 #include <memory>
+#include <optional>
 
 #include <boost/test/unit_test.hpp>
 
@@ -47,7 +48,9 @@ struct MinerTestingSetup : public TestingSetup {
         // pointer is not accessed, when the new one should be accessed
         // instead.
         m_node.mempool.reset();
-        m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node));
+        std::optional<bilingual_str> error{};
+        m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
+        Assert(!error);
         return *m_node.mempool;
     }
     BlockAssembler AssemblerForTest(CTxMemPool& tx_mempool);
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -175,7 +175,9 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
 
     m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES);
-    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node));
+    std::optional<bilingual_str> error{};
+    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
+    Assert(!error);
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
@@ -400,8 +400,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
     assert(int(nSigOpCostWithAncestors) >= 0);
 }
 
-CTxMemPool::CTxMemPool(const Options& opts)
-    : m_check_ratio{opts.check_ratio},
+CTxMemPool::CTxMemPool(const Options& opts, std::optional<bilingual_str>& error)
+    : m_check_ratio{std::clamp<int>(opts.check_ratio, 0, 1'000'000)},
       m_max_size_bytes{opts.max_size_bytes},
       m_expiry{opts.expiry},
       m_incremental_relay_feerate{opts.incremental_relay_feerate},
@@ -414,6 +414,10 @@ CTxMemPool::CTxMemPool(const Options& opts)
       m_persist_v1_dat{opts.persist_v1_dat},
       m_limits{opts.limits}
 {
+    int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;
+    if (opts.max_size_bytes < 0 || opts.max_size_bytes < descendant_limit_bytes) {
+        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
+    }
 }
 
 bool CTxMemPool::isSpent(const COutPoint& outpoint) const
diff --git a/src/txmempool.h b/src/txmempool.h
@@ -452,7 +452,7 @@ class CTxMemPool
      * accepting transactions becomes O(N^2) where N is the number of transactions
      * in the pool.
      */
-    explicit CTxMemPool(const Options& opts);
+    explicit CTxMemPool(const Options& opts, std::optional<bilingual_str>& error);
 
     /**
      * If sanity-checking is turned on, check makes sure the pool is

Original file line number	Diff line number	Diff line change
`@@ -125,7 +125,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte`
`125`	`125`	`mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();`
`126`	`126`
`127`	`127`	`// ...and construct a CTxMemPool from it`
`128`		`- return CTxMemPool{mempool_opts};`
	`128`	`+ std::optional<bilingual_str> error{};`
	`129`	`+ return CTxMemPool{mempool_opts, error};`
	`130`	`+ Assert(!error);`
`129`	`131`	`}`
`130`	`132`
`131`	`133`	`FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,9 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)`
`38`	`38`	`return;`
`39`	`39`	`}`
`40`	`40`
`41`		`- CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};`
	`41`	`+ std::optional<bilingual_str> error{};`
	`42`	`+ CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};`
	`43`	`+ Assert(!error);`
`42`	`44`
`43`	`45`	`LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)`
`44`	`46`	`{`
Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte`
`127`	`127`	`mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();`
`128`	`128`
`129`	`129`	`// ...and construct a CTxMemPool from it`
`130`		`- return CTxMemPool{mempool_opts};`
	`130`	`+ std::optional<bilingual_str> error{};`
	`131`	`+ return CTxMemPool{mempool_opts, error};`
	`132`	`+ Assert(!error);`
`131`	`133`	`}`
`132`	`134`
`133`	`135`	`void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, bool wtxid_in_mempool)`
Original file line number	Diff line number	Diff line change
`@@ -400,8 +400,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,`
`400`	`400`	`assert(int(nSigOpCostWithAncestors) >= 0);`
`401`	`401`	`}`
`402`	`402`
`403`		`-CTxMemPool::CTxMemPool(const Options& opts)`
`404`		`- : m_check_ratio{opts.check_ratio},`
	`403`	`+CTxMemPool::CTxMemPool(const Options& opts, std::optional<bilingual_str>& error)`
	`404`	`+ : m_check_ratio{std::clamp<int>(opts.check_ratio, 0, 1'000'000)},`
`405`	`405`	`m_max_size_bytes{opts.max_size_bytes},`
`406`	`406`	`m_expiry{opts.expiry},`
`407`	`407`	`m_incremental_relay_feerate{opts.incremental_relay_feerate},`
`@@ -414,6 +414,10 @@ CTxMemPool::CTxMemPool(const Options& opts)`
`414`	`414`	`m_persist_v1_dat{opts.persist_v1_dat},`
`415`	`415`	`m_limits{opts.limits}`
`416`	`416`	`{`
	`417`	`+ int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;`
	`418`	`+ if (opts.max_size_bytes < 0 \|\| opts.max_size_bytes < descendant_limit_bytes) {`
	`419`	`+ error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));`
	`420`	`+ }`
`417`	`421`	`}`
`418`	`422`
`419`	`423`	`bool CTxMemPool::isSpent(const COutPoint& outpoint) const`