From 09ef322acc0a88a9e119f74923399598984c68f6 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sun, 21 Apr 2024 10:03:21 +0200 Subject: [PATCH] [[refactor]] Check CTxMemPool options in constructor This ensures that the tests run the same checks on the mempool options that the init code also applies. --- src/init.cpp | 14 ++++++-------- src/test/fuzz/mini_miner.cpp | 12 +++++++++--- src/test/fuzz/package_eval.cpp | 15 +++++++++++---- src/test/fuzz/partially_downloaded_block.cpp | 6 +++++- src/test/fuzz/rbf.cpp | 10 ++++++++-- src/test/fuzz/tx_pool.cpp | 19 +++++++++++++------ src/test/fuzz/validation_load_mempool.cpp | 6 +++++- src/test/miner_tests.cpp | 6 +++++- src/test/util/setup_common.cpp | 4 +++- src/txmempool.cpp | 17 +++++++++++++++-- src/txmempool.h | 4 +++- 11 files changed, 83 insertions(+), 30 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 51ade4de93..7cd8bdc594 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1525,16 +1525,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (!result) { return InitError(util::ErrorString(result)); } - mempool_opts.check_ratio = std::clamp(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);) { - node.mempool = std::make_unique(mempool_opts); + bilingual_str mempool_error; + node.mempool = std::make_unique(mempool_opts, mempool_error); + if (!mempool_error.empty()) { + return InitError(mempool_error); + } + 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(*Assert(node.shutdown), chainman_opts, blockman_opts); ChainstateManager& chainman = *node.chainman; diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp index 84f9bb4ad0..3a1663364f 100644 --- a/src/test/fuzz/mini_miner.cpp +++ b/src/test/fuzz/mini_miner.cpp @@ -7,11 +7,13 @@ #include #include -#include #include +#include #include #include #include +#include +#include #include #include @@ -33,7 +35,9 @@ void initialize_miner() FUZZ_TARGET(mini_miner, .init = initialize_miner) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; - CTxMemPool pool{CTxMemPool::Options{}}; + bilingual_str error; + CTxMemPool pool{CTxMemPool::Options{}, error}; + Assert(error.empty()); std::vector outpoints; std::deque available_coins = g_available_coins; LOCK2(::cs_main, pool.cs); @@ -109,7 +113,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{}}; + bilingual_str error; + CTxMemPool pool{CTxMemPool::Options{}, error}; + Assert(error.empty()); // Make a copy to preserve determinism. std::deque available_coins = g_available_coins; std::vector transactions; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index c201118bce..fb5f1fa71e 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -15,7 +15,9 @@ #include #include #include +#include #include +#include #include #include @@ -107,7 +109,7 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains SetMockTime(time); } -CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node) +std::unique_ptr MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node) { // Take the default options for tests... CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; @@ -126,8 +128,13 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); + bilingual_str error; // ...and construct a CTxMemPool from it - return CTxMemPool{mempool_opts}; + auto mempool{std::make_unique(std::move(mempool_opts), error)}; + // ... ignore the error since it might be beneficial to fuzz even when the + // mempool size is unreasonably small + Assert(error.empty() || error.original.starts_with("-maxmempool must be at least ")); + return mempool; } FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) @@ -149,8 +156,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) auto outpoints_updater = std::make_shared(mempool_outpoints); node.validation_signals->RegisterSharedValidationInterface(outpoints_updater); - CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)}; - MockedTxPool& tx_pool = *static_cast(&tx_pool_); + auto tx_pool_{MakeMempool(fuzzed_data_provider, node)}; + MockedTxPool& tx_pool = *static_cast(tx_pool_.get()); chainstate.SetMempool(&tx_pool); diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 2bf47930f4..791d457710 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include @@ -52,7 +54,9 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) CBlockHeaderAndShortTxIDs cmpctblock{*block}; - CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)}; + bilingual_str error; + CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; + Assert(error.empty()); 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 index 64785948f6..4c7e70e3b0 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -13,6 +13,8 @@ #include #include #include +#include +#include #include #include @@ -56,7 +58,9 @@ FUZZ_TARGET(rbf, .init = initialize_rbf) return; } - CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)}; + bilingual_str error; + CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; + Assert(error.empty()); LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS) { @@ -90,7 +94,9 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) std::optional child = ConsumeDeserializable(fuzzed_data_provider, TX_WITH_WITNESS); if (!child) return; - CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)}; + bilingual_str error; + CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; + Assert(error.empty()); // Add a bunch of parent-child pairs to the mempool, and remember them. std::vector mempool_txs; diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 9f0aedf29b..4b0b724136 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -15,7 +15,9 @@ #include #include #include +#include #include +#include #include #include @@ -116,7 +118,7 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains SetMockTime(time); } -CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node) +std::unique_ptr MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node) { // Take the default options for tests... CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; @@ -126,7 +128,12 @@ 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}; + bilingual_str error; + auto mempool{std::make_unique(std::move(mempool_opts), error)}; + // ... ignore the error since it might be beneficial to fuzz even when the + // mempool size is unreasonably small + Assert(error.empty() || error.original.starts_with("-maxmempool must be at least ")); + return mempool; } void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, bool wtxid_in_mempool) @@ -198,8 +205,8 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN}; SetMempoolConstraints(*node.args, fuzzed_data_provider); - CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)}; - MockedTxPool& tx_pool = *static_cast(&tx_pool_); + auto tx_pool_{MakeMempool(fuzzed_data_provider, node)}; + MockedTxPool& tx_pool = *static_cast(tx_pool_.get()); chainstate.SetMempool(&tx_pool); @@ -376,8 +383,8 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) } SetMempoolConstraints(*node.args, fuzzed_data_provider); - CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)}; - MockedTxPool& tx_pool = *static_cast(&tx_pool_); + auto tx_pool_{MakeMempool(fuzzed_data_provider, node)}; + MockedTxPool& tx_pool = *static_cast(tx_pool_.get()); chainstate.SetMempool(&tx_pool); diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp index 00678742c9..51140ae039 100644 --- a/src/test/fuzz/validation_load_mempool.cpp +++ b/src/test/fuzz/validation_load_mempool.cpp @@ -13,7 +13,9 @@ #include #include #include +#include #include +#include #include #include @@ -40,7 +42,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)}; + bilingual_str error; + CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error}; + Assert(error.empty()); auto& chainstate{static_cast(g_setup->m_node.chainman->ActiveChainstate())}; chainstate.SetMempool(&pool); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index d50af4c175..042a8a9ba5 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -14,8 +14,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -46,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(MemPoolOptionsForTest(m_node)); + bilingual_str error; + m_node.mempool = std::make_unique(MemPoolOptionsForTest(m_node), error); + Assert(error.empty()); 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 index e9566cf50b..ab757b9640 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -227,7 +227,9 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto m_node.validation_signals = std::make_unique(std::make_unique(*m_node.scheduler)); m_node.fee_estimator = std::make_unique(FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES); - m_node.mempool = std::make_unique(MemPoolOptionsForTest(m_node)); + bilingual_str error{}; + m_node.mempool = std::make_unique(MemPoolOptionsForTest(m_node), error); + Assert(error.empty()); m_cache_sizes = CalculateCacheSizes(m_args); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index c845944d32..10674c07ac 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -26,6 +27,7 @@ #include #include +#include #include #include #include @@ -395,8 +397,19 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, assert(int(nSigOpCostWithAncestors) >= 0); } -CTxMemPool::CTxMemPool(const Options& opts) - : m_opts{opts} +//! Clamp option values and populate the error if options are not valid. +static CTxMemPool::Options&& Flatten(CTxMemPool::Options&& opts, bilingual_str& error) +{ + opts.check_ratio = std::clamp(opts.check_ratio, 0, 1'000'000); + 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)); + } + return std::move(opts); +} + +CTxMemPool::CTxMemPool(Options opts, bilingual_str& error) + : m_opts{Flatten(std::move(opts), error)} { } diff --git a/src/txmempool.h b/src/txmempool.h index c9f6cdbfac..52f186f0ff 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -43,6 +43,8 @@ class CChain; class ValidationSignals; +struct bilingual_str; + /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */ static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF; @@ -442,7 +444,7 @@ public: * accepting transactions becomes O(N^2) where N is the number of transactions * in the pool. */ - explicit CTxMemPool(const Options& opts); + explicit CTxMemPool(Options opts, bilingual_str& error); /** * If sanity-checking is turned on, check makes sure the pool is