From fc02f77ca604f0221171bfde3059b34f5d0fb1cd Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sun, 24 Oct 2021 02:26:19 -0400 Subject: [PATCH 01/16] ArgsMan: Add Get*Arg functions returning optional This allows the caller to not provide a default at all and just check inside the optional to see if the arg was set or not. --- src/util/system.cpp | 52 +++++++++++++++++++++++++++++++++++++++------ src/util/system.h | 8 +++++++ 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 6e2638a5d6c..e83ad11e80a 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -612,36 +612,76 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const } std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const +{ + return GetArg(strArg).value_or(strDefault); +} + +std::optional ArgsManager::GetArg(const std::string& strArg) const { const util::SettingsValue value = GetSetting(strArg); - return SettingToString(value, strDefault); + return SettingToString(value); +} + +std::optional SettingToString(const util::SettingsValue& value) +{ + if (value.isNull()) return std::nullopt; + if (value.isFalse()) return "0"; + if (value.isTrue()) return "1"; + if (value.isNum()) return value.getValStr(); + return value.get_str(); } std::string SettingToString(const util::SettingsValue& value, const std::string& strDefault) { - return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str(); + return SettingToString(value).value_or(strDefault); } int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const +{ + return GetIntArg(strArg).value_or(nDefault); +} + +std::optional ArgsManager::GetIntArg(const std::string& strArg) const { const util::SettingsValue value = GetSetting(strArg); - return SettingToInt(value, nDefault); + return SettingToInt(value); +} + +std::optional SettingToInt(const util::SettingsValue& value) +{ + if (value.isNull()) return std::nullopt; + if (value.isFalse()) return 0; + if (value.isTrue()) return 1; + if (value.isNum()) return value.getInt(); + return LocaleIndependentAtoi(value.get_str()); } int64_t SettingToInt(const util::SettingsValue& value, int64_t nDefault) { - return value.isNull() ? nDefault : value.isFalse() ? 0 : value.isTrue() ? 1 : value.isNum() ? value.getInt() : LocaleIndependentAtoi(value.get_str()); + return SettingToInt(value).value_or(nDefault); } bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const +{ + return GetBoolArg(strArg).value_or(fDefault); +} + +std::optional ArgsManager::GetBoolArg(const std::string& strArg) const { const util::SettingsValue value = GetSetting(strArg); - return SettingToBool(value, fDefault); + return SettingToBool(value); +} + +std::optional SettingToBool(const util::SettingsValue& value) +{ + if (value.isNull()) return std::nullopt; + if (value.isBool()) return value.get_bool(); + return InterpretBool(value.get_str()); } bool SettingToBool(const util::SettingsValue& value, bool fDefault) { - return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); + return SettingToBool(value).value_or(fDefault); } bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue) diff --git a/src/util/system.h b/src/util/system.h index 07d7a533aaf..04c66341d3d 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -161,8 +161,13 @@ struct SectionInfo }; std::string SettingToString(const util::SettingsValue&, const std::string&); +std::optional SettingToString(const util::SettingsValue&); + int64_t SettingToInt(const util::SettingsValue&, int64_t); +std::optional SettingToInt(const util::SettingsValue&); + bool SettingToBool(const util::SettingsValue&, bool); +std::optional SettingToBool(const util::SettingsValue&); class ArgsManager { @@ -335,6 +340,7 @@ protected: * @return command-line argument or default value */ std::string GetArg(const std::string& strArg, const std::string& strDefault) const; + std::optional GetArg(const std::string& strArg) const; /** * Return path argument or default value @@ -356,6 +362,7 @@ protected: * @return command-line argument (0 if invalid number) or default value */ int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const; + std::optional GetIntArg(const std::string& strArg) const; /** * Return boolean argument or default value @@ -365,6 +372,7 @@ protected: * @return command-line argument or default value */ bool GetBoolArg(const std::string& strArg, bool fDefault) const; + std::optional GetBoolArg(const std::string& strArg) const; /** * Set an argument if it doesn't already have a value From ccbaf546a68d6cda8ed3efd0598c0e4121b366bb Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 12:12:04 -0400 Subject: [PATCH 02/16] scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit Better to be explicit when it comes to sizes to avoid unintentional bugs. We use MB and KB all over the place. -BEGIN VERIFY SCRIPT- find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@\0_MB@g" -END VERIFY SCRIPT- --- src/init.cpp | 6 +++--- src/net_processing.cpp | 2 +- src/node/interfaces.cpp | 2 +- src/policy/policy.h | 2 +- src/rpc/fees.cpp | 2 +- src/rpc/mempool.cpp | 2 +- src/validation.cpp | 10 +++++----- 7 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index f20c55dcb1b..dfbcff4b1d9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -413,7 +413,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantx=", strprintf("Keep at most unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-minimumchainwork=", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s, signet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), signetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); @@ -929,7 +929,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb } // mempool limits - int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000; + int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000; int64_t nMempoolSizeMin = args.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT) * 1000 * 40; if (nMempoolSizeMax < 0 || nMempoolSizeMax < nMempoolSizeMin) return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(nMempoolSizeMin / 1000000.0))); @@ -1411,7 +1411,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // cache size calculations CacheSizes cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size()); - int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000; + int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000; LogPrintf("Cache configuration:\n"); LogPrintf("* Using %.1f MiB for block index database\n", cache_sizes.block_tree_db * (1.0 / 1024 / 1024)); if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 751a03f01ce..e958bef78f1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4599,7 +4599,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi // transactions to us, regardless of feefilter state. if (pto.IsBlockOnlyConn()) return; - CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK(); + CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000).GetFeePerK(); static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}}; if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 1905a4df29d..12b40aa2454 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -685,7 +685,7 @@ public: CFeeRate mempoolMinFee() override { if (!m_node.mempool) return {}; - return m_node.mempool->GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); + return m_node.mempool->GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000); } CFeeRate relayMinFee() override { return ::minRelayTxFee; } CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; } diff --git a/src/policy/policy.h b/src/policy/policy.h index cd46652efca..027b9c6a376 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -32,7 +32,7 @@ static constexpr unsigned int MAX_P2SH_SIGOPS{15}; /** The maximum number of sigops we're willing to relay/mine in a single tx */ static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5}; /** Default for -maxmempool, maximum megabytes of mempool memory usage */ -static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE{300}; +static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or BIP 125 replacement **/ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000}; /** Default for -bytespersigop */ diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp index 1873bc1587d..e6e8324c27c 100644 --- a/src/rpc/fees.cpp +++ b/src/rpc/fees.cpp @@ -89,7 +89,7 @@ static RPCHelpMan estimatesmartfee() FeeCalculation feeCalc; CFeeRate feeRate{fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative)}; if (feeRate != CFeeRate(0)) { - CFeeRate min_mempool_feerate{mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000)}; + CFeeRate min_mempool_feerate{mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000)}; CFeeRate min_relay_feerate{::minRelayTxFee}; feeRate = std::max({feeRate, min_mempool_feerate, min_relay_feerate}); result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK())); diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 97ec95a166b..07b12bc047f 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -657,7 +657,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool) ret.pushKV("bytes", (int64_t)pool.GetTotalTxSize()); ret.pushKV("usage", (int64_t)pool.DynamicMemoryUsage()); ret.pushKV("total_fee", ValueFromAmount(pool.GetTotalFee())); - int64_t maxmempool{gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000}; + int64_t maxmempool{gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000}; ret.pushKV("maxmempool", maxmempool); ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK())); ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK())); diff --git a/src/validation.cpp b/src/validation.cpp index b775c859121..4252914353a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -377,7 +377,7 @@ void CChainState::MaybeUpdateMempoolForReorg( LimitMempoolSize( *m_mempool, this->CoinsTip(), - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, + gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); } @@ -644,7 +644,7 @@ private: { AssertLockHeld(::cs_main); AssertLockHeld(m_pool.cs); - CAmount mempoolRejectFee = m_pool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size); + CAmount mempoolRejectFee = m_pool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000).GetFee(package_size); if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool min fee not met", strprintf("%d < %d", package_fee, mempoolRejectFee)); } @@ -1082,7 +1082,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool // is still within limits and package submission happens atomically. if (!args.m_package_submission && !bypass_limits) { - LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); if (!m_pool.exists(GenTxid::Txid(hash))) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); } @@ -1148,7 +1148,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // It may or may not be the case that all the transactions made it into the mempool. Regardless, // make sure we haven't exceeded max mempool size. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, + gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, @@ -2292,7 +2292,7 @@ CoinsCacheSizeState CChainState::GetCoinsCacheSizeState() AssertLockHeld(::cs_main); return this->GetCoinsCacheSizeState( m_coinstip_cache_size_bytes, - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); + gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000); } CoinsCacheSizeState CChainState::GetCoinsCacheSizeState( From 0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 24 Jun 2022 12:30:14 -0400 Subject: [PATCH 03/16] fuzz/rbf: Add missing TestingSetup MarcoFalke mentioned that this is likely a bug since "any log messages should be muted, not accumulated and turned into an OOM when fuzzing for a long time". --- src/test/fuzz/rbf.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index 8dcaa609b54..d4ec6ecfb1c 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -15,7 +16,17 @@ #include #include -FUZZ_TARGET(rbf) +namespace { +const BasicTestingSetup* g_setup; +} // namespace + +void initialize_rbf() +{ + static const auto testing_setup = MakeNoLogFileContext<>(); + g_setup = testing_setup.get(); +} + +FUZZ_TARGET_INIT(rbf, initialize_rbf) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); From f1941e8bfd2eecc478c7660434b1ebf6a64095a0 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 18 Mar 2022 13:51:37 -0400 Subject: [PATCH 04/16] 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. --- src/Makefile.am | 3 +++ src/init.cpp | 12 +++++++++-- src/kernel/mempool_options.h | 25 +++++++++++++++++++++++ src/mempool_args.cpp | 16 +++++++++++++++ src/mempool_args.h | 15 ++++++++++++++ src/test/fuzz/rbf.cpp | 8 ++++++-- src/test/fuzz/tx_pool.cpp | 20 ++++++++++++++++-- src/test/fuzz/validation_load_mempool.cpp | 4 +++- src/test/util/setup_common.cpp | 25 +++++++++++++++++++---- src/test/util/setup_common.h | 3 +++ src/txmempool.cpp | 5 +++-- src/txmempool.h | 9 ++++---- 12 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 src/kernel/mempool_options.h create mode 100644 src/mempool_args.cpp create mode 100644 src/mempool_args.h diff --git a/src/Makefile.am b/src/Makefile.am index fa716af6193..bdb279d1761 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 dfbcff4b1d9..3ad4dc2667a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -62,6 +63,7 @@ #include #include #include +#include #include #include #include @@ -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(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(mempool_opts.check_ratio, 0, 1'000'000); for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { - node.mempool = std::make_unique(node.fee_estimator.get(), mempool_check_ratio); + node.mempool = std::make_unique(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 00000000000..c3d54bf0f41 --- /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 00000000000..578463b81ae --- /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 + +#include + +#include + +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 00000000000..1732bdaac93 --- /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 d4ec6ecfb1c..4801635791a 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 #include #include #include @@ -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 another_mtx = ConsumeDeserializable(fuzzed_data_provider); if (!another_mtx) { break; diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 4f40608c4fa..2d88ee295bb 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 +#include +#include #include #include #include @@ -15,6 +17,7 @@ #include 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(&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(&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 c2aaf486c5d..9532610f8da 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 +#include #include #include #include @@ -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 d9fff85bf51..60ddda1c0a1 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -14,10 +14,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -32,6 +34,8 @@ #include #include #include +#include +#include #include #include #include @@ -50,11 +54,12 @@ using node::BlockAssembler; using node::CalculateCacheSizes; -using node::LoadChainstate; -using node::RegenerateCommitments; -using node::VerifyLoadedChainstate; using node::fPruneMode; using node::fReindex; +using node::LoadChainstate; +using node::NodeContext; +using node::RegenerateCommitments; +using node::VerifyLoadedChainstate; const std::function 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& 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(); - m_node.mempool = std::make_unique(m_node.fee_estimator.get(), m_node.args->GetIntArg("-checkmempool", 1)); + m_node.mempool = std::make_unique(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 37407bcb92f..ed2c5db7e69 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 65c8b4ea606..84088aead6d 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 f5d5abc62e4..32319ac1818 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -14,6 +14,8 @@ #include #include +#include + #include #include #include @@ -560,15 +562,14 @@ public: indirectmap mapNextTx GUARDED_BY(cs); std::map 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 From 82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 28 Oct 2021 13:46:19 -0400 Subject: [PATCH 05/16] mempool: Pass in -maxmempool instead of referencing gArgs - Store the mempool size limit (-maxmempool) in CTxMemPool as a member. - Remove the requirement to explicitly specify a mempool size limit for CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the stored mempool size limit where possible. - Remove all now-unnecessary instances of: gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000 The code change in CChainState::GetCoinsCacheSizeState() is correct since the coinscache should not repurpose "extra" mempool memory headroom for itself if the mempool doesn't even exist. --- src/kernel/mempool_options.h | 6 ++++++ src/mempool_args.cpp | 2 ++ src/net_processing.cpp | 2 +- src/node/interfaces.cpp | 2 +- src/policy/policy.h | 2 -- src/rpc/fees.cpp | 2 +- src/rpc/mempool.cpp | 5 ++--- src/txmempool.cpp | 3 ++- src/txmempool.h | 5 +++++ src/validation.cpp | 12 +++++------- 10 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index c3d54bf0f41..e8c129050e0 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -4,8 +4,13 @@ #ifndef BITCOIN_KERNEL_MEMPOOL_OPTIONS_H #define BITCOIN_KERNEL_MEMPOOL_OPTIONS_H +#include + class CBlockPolicyEstimator; +/** Default for -maxmempool, maximum megabytes of mempool memory usage */ +static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; + namespace kernel { /** * Options struct containing options for constructing a CTxMemPool. Default @@ -19,6 +24,7 @@ struct MemPoolOptions { CBlockPolicyEstimator* estimator{nullptr}; /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; + int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; }; } // namespace kernel diff --git a/src/mempool_args.cpp b/src/mempool_args.cpp index 578463b81ae..aef4e478f3c 100644 --- a/src/mempool_args.cpp +++ b/src/mempool_args.cpp @@ -13,4 +13,6 @@ using kernel::MemPoolOptions; void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts) { mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio); + + if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1'000'000; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e958bef78f1..14cc9c169d6 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4599,7 +4599,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi // transactions to us, regardless of feefilter state. if (pto.IsBlockOnlyConn()) return; - CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000).GetFeePerK(); + CAmount currentFilter = m_mempool.GetMinFee().GetFeePerK(); static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}}; if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 12b40aa2454..14eecdb950d 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -685,7 +685,7 @@ public: CFeeRate mempoolMinFee() override { if (!m_node.mempool) return {}; - return m_node.mempool->GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000); + return m_node.mempool->GetMinFee(); } CFeeRate relayMinFee() override { return ::minRelayTxFee; } CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; } diff --git a/src/policy/policy.h b/src/policy/policy.h index 027b9c6a376..b3993b6a245 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -31,8 +31,6 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{82}; static constexpr unsigned int MAX_P2SH_SIGOPS{15}; /** The maximum number of sigops we're willing to relay/mine in a single tx */ static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5}; -/** Default for -maxmempool, maximum megabytes of mempool memory usage */ -static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or BIP 125 replacement **/ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000}; /** Default for -bytespersigop */ diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp index e6e8324c27c..dd1a6441a08 100644 --- a/src/rpc/fees.cpp +++ b/src/rpc/fees.cpp @@ -89,7 +89,7 @@ static RPCHelpMan estimatesmartfee() FeeCalculation feeCalc; CFeeRate feeRate{fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative)}; if (feeRate != CFeeRate(0)) { - CFeeRate min_mempool_feerate{mempool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000)}; + CFeeRate min_mempool_feerate{mempool.GetMinFee()}; CFeeRate min_relay_feerate{::minRelayTxFee}; feeRate = std::max({feeRate, min_mempool_feerate, min_relay_feerate}); result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK())); diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 07b12bc047f..2cdde141447 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -657,9 +657,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool) ret.pushKV("bytes", (int64_t)pool.GetTotalTxSize()); ret.pushKV("usage", (int64_t)pool.DynamicMemoryUsage()); ret.pushKV("total_fee", ValueFromAmount(pool.GetTotalFee())); - int64_t maxmempool{gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000}; - ret.pushKV("maxmempool", maxmempool); - ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK())); + ret.pushKV("maxmempool", pool.m_max_size_bytes); + ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(), ::minRelayTxFee).GetFeePerK())); ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK())); ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()}); return ret; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 84088aead6d..3e75f2db8d7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -453,7 +453,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, - minerPolicyEstimator{opts.estimator} + minerPolicyEstimator{opts.estimator}, + m_max_size_bytes{opts.max_size_bytes} { _clear(); //lock free clear } diff --git a/src/txmempool.h b/src/txmempool.h index 32319ac1818..d5d573b4f92 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -564,6 +564,8 @@ public: using Options = kernel::MemPoolOptions; + const int64_t m_max_size_bytes; + /** 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 @@ -702,6 +704,9 @@ public: * takes the fee rate to go back down all the way to 0. When the feerate * would otherwise be half of this, it is set to 0 instead. */ + CFeeRate GetMinFee() const { + return GetMinFee(m_max_size_bytes); + } CFeeRate GetMinFee(size_t sizelimit) const; /** Remove transactions from the mempool until its dynamic size is <= sizelimit. diff --git a/src/validation.cpp b/src/validation.cpp index 4252914353a..a14f82106f4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -255,7 +255,7 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip, // Returns the script flags which should be checked for a given block static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const ChainstateManager& chainman); -static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age) +static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, std::chrono::seconds age) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) { AssertLockHeld(::cs_main); @@ -266,7 +266,7 @@ static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, siz } std::vector vNoSpendsRemaining; - pool.TrimToSize(limit, &vNoSpendsRemaining); + pool.TrimToSize(pool.m_max_size_bytes, &vNoSpendsRemaining); for (const COutPoint& removed : vNoSpendsRemaining) coins_cache.Uncache(removed); } @@ -377,7 +377,6 @@ void CChainState::MaybeUpdateMempoolForReorg( LimitMempoolSize( *m_mempool, this->CoinsTip(), - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); } @@ -644,7 +643,7 @@ private: { AssertLockHeld(::cs_main); AssertLockHeld(m_pool.cs); - CAmount mempoolRejectFee = m_pool.GetMinFee(gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000).GetFee(package_size); + CAmount mempoolRejectFee = m_pool.GetMinFee().GetFee(package_size); if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool min fee not met", strprintf("%d < %d", package_fee, mempoolRejectFee)); } @@ -1082,7 +1081,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool // is still within limits and package submission happens atomically. if (!args.m_package_submission && !bypass_limits) { - LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); if (!m_pool.exists(GenTxid::Txid(hash))) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); } @@ -1148,7 +1147,6 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // It may or may not be the case that all the transactions made it into the mempool. Regardless, // make sure we haven't exceeded max mempool size. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, @@ -2292,7 +2290,7 @@ CoinsCacheSizeState CChainState::GetCoinsCacheSizeState() AssertLockHeld(::cs_main); return this->GetCoinsCacheSizeState( m_coinstip_cache_size_bytes, - gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000); + m_mempool ? m_mempool->m_max_size_bytes : 0); } CoinsCacheSizeState CChainState::GetCoinsCacheSizeState( From 386c9472c8764738282e6d163b42e15a8feda7ea Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 27 Jun 2022 15:47:00 -0400 Subject: [PATCH 06/16] mempool: Make GetMinFee() with custom size protected The version of GetMinFee() with a custom size specification is and should only be used by tests. Mark it as protected and use a derived class exposing GetMinFee() as public in tests. --- src/test/mempool_tests.cpp | 8 +++++++- src/txmempool.h | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index bc631220251..8c745b07b9a 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -16,6 +16,12 @@ BOOST_FIXTURE_TEST_SUITE(mempool_tests, TestingSetup) static constexpr auto REMOVAL_REASON_DUMMY = MemPoolRemovalReason::REPLACED; +class MemPoolTest final : public CTxMemPool +{ +public: + using CTxMemPool::GetMinFee; +}; + BOOST_AUTO_TEST_CASE(MempoolRemoveTest) { // Test CTxMemPool::remove functionality @@ -423,7 +429,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { - CTxMemPool& pool = *Assert(m_node.mempool); + auto& pool = static_cast(*Assert(m_node.mempool)); LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; diff --git a/src/txmempool.h b/src/txmempool.h index d5d573b4f92..95fd56879fa 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -453,6 +453,8 @@ protected: bool m_is_loaded GUARDED_BY(cs){false}; + CFeeRate GetMinFee(size_t sizelimit) const; + public: static const int ROLLING_FEE_HALFLIFE = 60 * 60 * 12; // public only for testing @@ -707,7 +709,6 @@ public: CFeeRate GetMinFee() const { return GetMinFee(m_max_size_bytes); } - CFeeRate GetMinFee(size_t sizelimit) const; /** Remove transactions from the mempool until its dynamic size is <= sizelimit. * pvNoSpendsRemaining, if set, will be populated with the list of outpoints From 51c7a41a5eb6fcb60333812c770d80227cf7b64d Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 11:48:29 -0400 Subject: [PATCH 07/16] init: Only determine maxmempool once Now that MemPoolOptions has a correctly-determined max_size member, use that instead of redetermining it to print the log line. --- src/init.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 3ad4dc2667a..efbc9f41d9f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1413,7 +1413,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // cache size calculations CacheSizes cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size()); - int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000; LogPrintf("Cache configuration:\n"); LogPrintf("* Using %.1f MiB for block index database\n", cache_sizes.block_tree_db * (1.0 / 1024 / 1024)); if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { @@ -1424,7 +1423,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) cache_sizes.filter_index * (1.0 / 1024 / 1024), BlockFilterTypeName(filter_type)); } LogPrintf("* Using %.1f MiB for chain state database\n", cache_sizes.coins_db * (1.0 / 1024 / 1024)); - 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), nMempoolSizeMax * (1.0 / 1024 / 1024)); assert(!node.mempool); assert(!node.chainman); @@ -1435,6 +1433,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) }; ApplyArgsManOptions(args, mempool_opts); mempool_opts.check_ratio = std::clamp(mempool_opts.check_ratio, 0, 1'000'000); + 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(mempool_opts); From aa9141cd8185cb7ad532bc16feb9d302b05d9697 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 28 Oct 2021 14:13:04 -0400 Subject: [PATCH 08/16] mempool: Pass in -mempoolexpiry instead of referencing gArgs - Store the mempool expiry (-mempoolexpiry) in CTxMemPool as a std::chrono::seconds member. - Remove the requirement to explicitly specify a mempool expiry for LimitMempoolSize(...), just use the newly-introduced member. - Remove all now-unnecessary instances of: std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)} --- src/kernel/mempool_options.h | 4 ++++ src/mempool_args.cpp | 2 ++ src/txmempool.cpp | 3 ++- src/txmempool.h | 1 + src/validation.cpp | 16 ++++++---------- src/validation.h | 2 -- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index e8c129050e0..35621e1a28e 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -4,12 +4,15 @@ #ifndef BITCOIN_KERNEL_MEMPOOL_OPTIONS_H #define BITCOIN_KERNEL_MEMPOOL_OPTIONS_H +#include #include class CBlockPolicyEstimator; /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; +/** Default for -mempoolexpiry, expiration time for mempool transactions in hours */ +static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY{336}; namespace kernel { /** @@ -25,6 +28,7 @@ struct MemPoolOptions { /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; + std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY}}; }; } // namespace kernel diff --git a/src/mempool_args.cpp b/src/mempool_args.cpp index aef4e478f3c..06d553cf444 100644 --- a/src/mempool_args.cpp +++ b/src/mempool_args.cpp @@ -15,4 +15,6 @@ void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opt mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio); if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1'000'000; + + if (auto hours = argsman.GetIntArg("-mempoolexpiry")) mempool_opts.expiry = std::chrono::hours{*hours}; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 3e75f2db8d7..a2220e68746 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -454,7 +454,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, minerPolicyEstimator{opts.estimator}, - m_max_size_bytes{opts.max_size_bytes} + m_max_size_bytes{opts.max_size_bytes}, + m_expiry{opts.expiry} { _clear(); //lock free clear } diff --git a/src/txmempool.h b/src/txmempool.h index 95fd56879fa..0a91cf31c76 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -567,6 +567,7 @@ public: using Options = kernel::MemPoolOptions; const int64_t m_max_size_bytes; + const std::chrono::seconds m_expiry; /** Create a new CTxMemPool. * Sanity checks will be off by default for performance, because otherwise diff --git a/src/validation.cpp b/src/validation.cpp index a14f82106f4..c1ff603faab 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -255,12 +255,12 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip, // Returns the script flags which should be checked for a given block static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const ChainstateManager& chainman); -static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, std::chrono::seconds age) +static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) { AssertLockHeld(::cs_main); AssertLockHeld(pool.cs); - int expired = pool.Expire(GetTime() - age); + int expired = pool.Expire(GetTime() - pool.m_expiry); if (expired != 0) { LogPrint(BCLog::MEMPOOL, "Expired %i transactions from the memory pool\n", expired); } @@ -374,10 +374,7 @@ void CChainState::MaybeUpdateMempoolForReorg( // We also need to remove any now-immature transactions m_mempool->removeForReorg(m_chain, filter_final_and_mature); // Re-limit mempool size, in case we added any transactions - LimitMempoolSize( - *m_mempool, - this->CoinsTip(), - std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); + LimitMempoolSize(*m_mempool, this->CoinsTip()); } /** @@ -1081,7 +1078,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool // is still within limits and package submission happens atomically. if (!args.m_package_submission && !bypass_limits) { - LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); if (!m_pool.exists(GenTxid::Txid(hash))) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); } @@ -1146,8 +1143,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // It may or may not be the case that all the transactions made it into the mempool. Regardless, // make sure we haven't exceeded max mempool size. - LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), - std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, // but don't report success unless they all made it into the mempool. @@ -4645,7 +4641,7 @@ static const uint64_t MEMPOOL_DUMP_VERSION = 1; bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mockable_fopen_function) { - int64_t nExpiryTimeout = gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60; + int64_t nExpiryTimeout = std::chrono::seconds{pool.m_expiry}.count(); FILE* filestr{mockable_fopen_function(gArgs.GetDataDirNet() / "mempool.dat", "rb")}; CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); if (file.IsNull()) { diff --git a/src/validation.h b/src/validation.h index 3b6cd509c68..0e27e117fa5 100644 --- a/src/validation.h +++ b/src/validation.h @@ -59,8 +59,6 @@ namespace Consensus { struct Params; } // namespace Consensus -/** Default for -mempoolexpiry, expiration time for mempool transactions in hours */ -static const unsigned int DEFAULT_MEMPOOL_EXPIRY = 336; /** Maximum number of dedicated script-checking threads allowed */ static const int MAX_SCRIPTCHECK_THREADS = 15; /** -par default (number of script-checking threads, 0 = auto) */ From 1ecc77321deb61b9f6888e4e10752b9d972fd26e Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 21 Jun 2022 10:37:10 -0400 Subject: [PATCH 09/16] scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit Better to be explicit when it comes to time to avoid unintentional bugs. -BEGIN VERIFY SCRIPT- find_regex="DEFAULT_MEMPOOL_EXPIRY" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@\0_HOURS@g" -END VERIFY SCRIPT- --- src/init.cpp | 2 +- src/kernel/mempool_options.h | 4 ++-- test/functional/mempool_expiry.py | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index efbc9f41d9f..ea0d7640c6d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -417,7 +417,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantx=", strprintf("Keep at most unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-minimumchainwork=", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s, signet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), signetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-par=", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)", -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index 35621e1a28e..c4f34ff9f99 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -12,7 +12,7 @@ class CBlockPolicyEstimator; /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */ -static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY{336}; +static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336}; namespace kernel { /** @@ -28,7 +28,7 @@ struct MemPoolOptions { /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; - std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY}}; + std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}}; }; } // namespace kernel diff --git a/test/functional/mempool_expiry.py b/test/functional/mempool_expiry.py index f301b29c254..47ae0c762b4 100755 --- a/test/functional/mempool_expiry.py +++ b/test/functional/mempool_expiry.py @@ -5,7 +5,7 @@ """Tests that a mempool transaction expires after a given timeout and that its children are removed as well. -Both the default expiry timeout defined by DEFAULT_MEMPOOL_EXPIRY and a user +Both the default expiry timeout defined by DEFAULT_MEMPOOL_EXPIRY_HOURS and a user definable expiry timeout via the '-mempoolexpiry=' command line argument ( is the timeout in hours) are tested. """ @@ -20,7 +20,7 @@ from test_framework.util import ( ) from test_framework.wallet import MiniWallet -DEFAULT_MEMPOOL_EXPIRY = 336 # hours +DEFAULT_MEMPOOL_EXPIRY_HOURS = 336 # hours CUSTOM_MEMPOOL_EXPIRY = 10 # hours @@ -98,8 +98,8 @@ class MempoolExpiryTest(BitcoinTestFramework): def run_test(self): self.log.info('Test default mempool expiry timeout of %d hours.' % - DEFAULT_MEMPOOL_EXPIRY) - self.test_transaction_expiry(DEFAULT_MEMPOOL_EXPIRY) + DEFAULT_MEMPOOL_EXPIRY_HOURS) + self.test_transaction_expiry(DEFAULT_MEMPOOL_EXPIRY_HOURS) self.log.info('Test custom mempool expiry timeout of %d hours.' % CUSTOM_MEMPOOL_EXPIRY) From 716bb5fbd31077bbe99d11a54d6c2c250afc8085 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Mar 2022 21:54:40 -0400 Subject: [PATCH 10/16] scripted-diff: Rename anc/desc size limit vars to indicate SI unit Better to be explicit when it comes to sizes to avoid unintentional bugs. We use MB and KB all over the place. -BEGIN VERIFY SCRIPT- find_regex="DEFAULT_(ANCESTOR|DESCENDANT)_SIZE_LIMIT" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@\0_KVB@g" -END VERIFY SCRIPT- --- src/init.cpp | 6 +++--- src/node/interfaces.cpp | 4 ++-- src/policy/packages.h | 4 ++-- src/policy/policy.h | 4 ++-- src/validation.cpp | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index ea0d7640c6d..9259745214c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -538,9 +538,9 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitancestorcount=", strprintf("Do not accept transactions if number of in-mempool ancestors is or more (default: %u)", DEFAULT_ANCESTOR_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-limitancestorsize=", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-limitancestorsize=", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitdescendantcount=", strprintf("Do not accept transactions if any ancestor would have or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-limitdescendantsize=", strprintf("Do not accept transactions if any ancestor would have more than kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-limitdescendantsize=", strprintf("Do not accept transactions if any ancestor would have more than kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-mocktime=", "Replace actual time with " + UNIX_EPOCH_TIME + " (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); @@ -932,7 +932,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb // mempool limits int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000; - int64_t nMempoolSizeMin = args.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT) * 1000 * 40; + int64_t nMempoolSizeMin = args.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB) * 1000 * 40; if (nMempoolSizeMax < 0 || nMempoolSizeMax < nMempoolSizeMin) return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(nMempoolSizeMin / 1000000.0))); // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 14eecdb950d..a99ed73596d 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -663,9 +663,9 @@ public: CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); CTxMemPool::setEntries ancestors; auto limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); - auto limit_ancestor_size = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000; + auto limit_ancestor_size = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB) * 1000; auto limit_descendant_count = gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT); - auto limit_descendant_size = gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT) * 1000; + auto limit_descendant_size = gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB) * 1000; std::string unused_error_string; LOCK(m_node.mempool->cs); return m_node.mempool->CalculateMemPoolAncestors( diff --git a/src/policy/packages.h b/src/policy/packages.h index 564ff50d291..36c70e9e660 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -25,8 +25,8 @@ static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_ // defaults reflect this constraint. static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT); static_assert(DEFAULT_ANCESTOR_LIMIT >= MAX_PACKAGE_COUNT); -static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT >= MAX_PACKAGE_SIZE); -static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT >= MAX_PACKAGE_SIZE); +static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE); +static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE); /** A "reason" why a package was invalid. It may be that one or more of the included * transactions is invalid or the package itself violates our rules. diff --git a/src/policy/policy.h b/src/policy/policy.h index b3993b6a245..e255708eff4 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -58,11 +58,11 @@ static constexpr unsigned int DEFAULT_MIN_RELAY_TX_FEE{1000}; /** Default for -limitancestorcount, max number of in-mempool ancestors */ static constexpr unsigned int DEFAULT_ANCESTOR_LIMIT{25}; /** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */ -static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT{101}; +static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KVB{101}; /** Default for -limitdescendantcount, max number of in-mempool descendants */ static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25}; /** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */ -static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT{101}; +static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101}; /** * An extra transaction can be added to a package, as long as it only has one * ancestor and is no larger than this. Not really any reason to make this diff --git a/src/validation.cpp b/src/validation.cpp index c1ff603faab..431b89c32be 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -321,7 +321,7 @@ void CChainState::MaybeUpdateMempoolForReorg( // UpdateTransactionsFromBlock finds descendants of any transactions in // the disconnectpool that were added back and cleans up the mempool state. const uint64_t ancestor_count_limit = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); - const uint64_t ancestor_size_limit = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000; + const uint64_t ancestor_size_limit = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB) * 1000; m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_size_limit, ancestor_count_limit); // Predicate to use for filtering transactions in removeForReorg. @@ -426,9 +426,9 @@ class MemPoolAccept public: explicit MemPoolAccept(CTxMemPool& mempool, CChainState& active_chainstate) : m_pool(mempool), m_view(&m_dummy), m_viewmempool(&active_chainstate.CoinsTip(), m_pool), m_active_chainstate(active_chainstate), m_limit_ancestors(gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT)), - m_limit_ancestor_size(gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000), + m_limit_ancestor_size(gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB)*1000), m_limit_descendants(gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT)), - m_limit_descendant_size(gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000) { + m_limit_descendant_size(gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB)*1000) { } // We put the arguments we're handed into a struct, so we can pass them From 9333427014695ac235c96d48791098168dfdc9db Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Mar 2022 22:09:05 -0400 Subject: [PATCH 11/16] mempool: Introduce (still-unused) MemPoolLimits They live as a CTxMemPool member. [META] These limits will be used in subsequent commits to replace calls to gArgs. --- src/Makefile.am | 1 + src/kernel/mempool_limits.h | 26 ++++++++++++++++++++++++++ src/kernel/mempool_options.h | 3 +++ src/mempool_args.cpp | 17 +++++++++++++++++ src/mempool_args.h | 7 +++++++ src/txmempool.cpp | 3 ++- src/txmempool.h | 5 +++++ 7 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 src/kernel/mempool_limits.h diff --git a/src/Makefile.am b/src/Makefile.am index bdb279d1761..734af78ae50 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -173,6 +173,7 @@ BITCOIN_CORE_H = \ kernel/checks.h \ kernel/coinstats.h \ kernel/context.h \ + kernel/mempool_limits.h \ kernel/mempool_options.h \ key.h \ key_io.h \ diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h new file mode 100644 index 00000000000..083e9681e74 --- /dev/null +++ b/src/kernel/mempool_limits.h @@ -0,0 +1,26 @@ +// 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_LIMITS_H +#define BITCOIN_KERNEL_MEMPOOL_LIMITS_H + +#include + +#include + +namespace kernel { +/** + * Options struct containing limit options for 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::Limits. + */ +struct MemPoolLimits { + int64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT}; + int64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000}; + int64_t descendant_count{DEFAULT_DESCENDANT_LIMIT}; + int64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000}; +}; +} // namespace kernel + +#endif // BITCOIN_KERNEL_MEMPOOL_LIMITS_H diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index c4f34ff9f99..a14abb66289 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -4,6 +4,8 @@ #ifndef BITCOIN_KERNEL_MEMPOOL_OPTIONS_H #define BITCOIN_KERNEL_MEMPOOL_OPTIONS_H +#include + #include #include @@ -29,6 +31,7 @@ struct MemPoolOptions { int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}}; + MemPoolLimits limits{}; }; } // namespace kernel diff --git a/src/mempool_args.cpp b/src/mempool_args.cpp index 06d553cf444..e26cbe02753 100644 --- a/src/mempool_args.cpp +++ b/src/mempool_args.cpp @@ -4,12 +4,27 @@ #include +#include #include #include +using kernel::MemPoolLimits; using kernel::MemPoolOptions; +namespace { +void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolLimits& mempool_limits) +{ + mempool_limits.ancestor_count = argsman.GetIntArg("-limitancestorcount", mempool_limits.ancestor_count); + + if (auto vkb = argsman.GetIntArg("-limitancestorsize")) mempool_limits.ancestor_size_vbytes = *vkb * 1'000; + + mempool_limits.descendant_count = argsman.GetIntArg("-limitdescendantcount", mempool_limits.descendant_count); + + if (auto vkb = argsman.GetIntArg("-limitdescendantsize")) mempool_limits.descendant_size_vbytes = *vkb * 1'000; +} +} + void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts) { mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio); @@ -17,4 +32,6 @@ void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opt if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1'000'000; if (auto hours = argsman.GetIntArg("-mempoolexpiry")) mempool_opts.expiry = std::chrono::hours{*hours}; + + ApplyArgsManOptions(argsman, mempool_opts.limits); } diff --git a/src/mempool_args.h b/src/mempool_args.h index 1732bdaac93..9a4abe6618f 100644 --- a/src/mempool_args.h +++ b/src/mempool_args.h @@ -10,6 +10,13 @@ namespace kernel { struct MemPoolOptions; }; +/** + * Overlay the options set in \p argsman on top of corresponding members in \p mempool_opts. + * + * @param[in] argsman The ArgsManager in which to check set options. + * @param[in,out] mempool_opts The MemPoolOptions to modify according to \p argsman. + */ void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts); + #endif // BITCOIN_MEMPOOL_ARGS_H diff --git a/src/txmempool.cpp b/src/txmempool.cpp index a2220e68746..2c2b0631332 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -455,7 +455,8 @@ CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, minerPolicyEstimator{opts.estimator}, m_max_size_bytes{opts.max_size_bytes}, - m_expiry{opts.expiry} + m_expiry{opts.expiry}, + m_limits{opts.limits} { _clear(); //lock free clear } diff --git a/src/txmempool.h b/src/txmempool.h index 0a91cf31c76..4cc62448f99 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -569,6 +570,10 @@ public: const int64_t m_max_size_bytes; const std::chrono::seconds m_expiry; + using Limits = kernel::MemPoolLimits; + + const Limits m_limits; + /** 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 From 38af2bcf358a72b9457d370282e57f4be1c5c849 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 6 Jun 2022 21:19:50 -0400 Subject: [PATCH 12/16] mempoolaccept: Use limits from mempool in constructor --- src/validation.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 431b89c32be..166aaa56fba 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -425,10 +425,10 @@ class MemPoolAccept { public: explicit MemPoolAccept(CTxMemPool& mempool, CChainState& active_chainstate) : m_pool(mempool), m_view(&m_dummy), m_viewmempool(&active_chainstate.CoinsTip(), m_pool), m_active_chainstate(active_chainstate), - m_limit_ancestors(gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT)), - m_limit_ancestor_size(gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB)*1000), - m_limit_descendants(gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT)), - m_limit_descendant_size(gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB)*1000) { + m_limit_ancestors(m_pool.m_limits.ancestor_count), + m_limit_ancestor_size(m_pool.m_limits.ancestor_size_vbytes), + m_limit_descendants(m_pool.m_limits.descendant_count), + m_limit_descendant_size(m_pool.m_limits.descendant_size_vbytes) { } // We put the arguments we're handed into a struct, so we can pass them From 9e93b1030182eff92ef91181e17c7dd498c7e164 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 14:43:59 -0400 Subject: [PATCH 13/16] node/ifaces: Use existing MemPoolLimits --- src/node/interfaces.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index a99ed73596d..3c085ae6fbb 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -653,8 +653,12 @@ public: } void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) override { - limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); - limit_descendant_count = gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT); + const CTxMemPool::Limits default_limits{}; + + const CTxMemPool::Limits& limits{m_node.mempool ? m_node.mempool->m_limits : default_limits}; + + limit_ancestor_count = limits.ancestor_count; + limit_descendant_count = limits.descendant_count; } bool checkChainLimits(const CTransactionRef& tx) override { @@ -662,15 +666,12 @@ public: LockPoints lp; CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); CTxMemPool::setEntries ancestors; - auto limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); - auto limit_ancestor_size = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB) * 1000; - auto limit_descendant_count = gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT); - auto limit_descendant_size = gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB) * 1000; + const CTxMemPool::Limits& limits{m_node.mempool->m_limits}; std::string unused_error_string; LOCK(m_node.mempool->cs); return m_node.mempool->CalculateMemPoolAncestors( - entry, ancestors, limit_ancestor_count, limit_ancestor_size, - limit_descendant_count, limit_descendant_size, unused_error_string); + entry, ancestors, limits.ancestor_count, limits.ancestor_size_vbytes, + limits.descendant_count, limits.descendant_size_vbytes, unused_error_string); } CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override { From 6c5c60c4124293d948735756f84efc85262ea66f Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 14:44:25 -0400 Subject: [PATCH 14/16] mempool: Use m_limit for UpdateTransactionsFromBlock Since: - UpdateTransactionsFromBlock is only called by MaybeUpdateMempoolForReorg, which calls it with the gArgs-determined ancestor limits - UpdateForDescendants is only called by UpdateTransactionsFromBlock with the ancestor limits unchanged We can remove the requirement to specify the ancestor limits for both UpdateTransactionsFromBlock and UpdateForDescendants and just use the values in the m_limits member. Also move some removed comments to MemPoolLimits struct members. The uint64_t cast in UpdateForDescendants is not new behavior, see the diff in CChainState::MaybeUpdateMempoolForReorg for where they were previously. --- src/kernel/mempool_limits.h | 4 ++++ src/txmempool.cpp | 9 ++++----- src/txmempool.h | 14 ++------------ src/validation.cpp | 4 +--- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h index 083e9681e74..e192e7e6cde 100644 --- a/src/kernel/mempool_limits.h +++ b/src/kernel/mempool_limits.h @@ -16,9 +16,13 @@ namespace kernel { * Most of the time, this struct should be referenced as CTxMemPool::Limits. */ struct MemPoolLimits { + //! The maximum allowed number of transactions in a package including the entry and its ancestors. int64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT}; + //! The maximum allowed size in virtual bytes of an entry and its ancestors within a package. int64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000}; + //! The maximum allowed number of transactions in a package including the entry and its descendants. int64_t descendant_count{DEFAULT_DESCENDANT_LIMIT}; + //! The maximum allowed size in virtual bytes of an entry and its descendants within a package. int64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000}; }; } // namespace kernel diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2c2b0631332..25019e98ab5 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -107,8 +107,7 @@ size_t CTxMemPoolEntry::GetTxSize() const } void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, - const std::set& setExclude, std::set& descendants_to_remove, - uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) + const std::set& setExclude, std::set& descendants_to_remove) { CTxMemPoolEntry::Children stageEntries, descendants; stageEntries = updateIt->GetMemPoolChildrenConst(); @@ -148,7 +147,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan // Don't directly remove the transaction here -- doing so would // invalidate iterators in cachedDescendants. Mark it for removal // by inserting into descendants_to_remove. - if (descendant.GetCountWithAncestors() > ancestor_count_limit || descendant.GetSizeWithAncestors() > ancestor_size_limit) { + if (descendant.GetCountWithAncestors() > uint64_t(m_limits.ancestor_count) || descendant.GetSizeWithAncestors() > uint64_t(m_limits.ancestor_size_vbytes)) { descendants_to_remove.insert(descendant.GetTx().GetHash()); } } @@ -156,7 +155,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount)); } -void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashesToUpdate, uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) +void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) { AssertLockHeld(cs); // For each entry in vHashesToUpdate, store the set of in-mempool, but not @@ -199,7 +198,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes } } } // release epoch guard for UpdateForDescendants - UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded, descendants_to_remove, ancestor_size_limit, ancestor_count_limit); + UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded, descendants_to_remove); } for (const auto& txid : descendants_to_remove) { diff --git a/src/txmempool.h b/src/txmempool.h index 4cc62448f99..2e2cbe526a0 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -659,13 +659,8 @@ public: * * @param[in] vHashesToUpdate The set of txids from the * disconnected block that have been accepted back into the mempool. - * @param[in] ancestor_size_limit The maximum allowed size in virtual - * bytes of an entry and its ancestors - * @param[in] ancestor_count_limit The maximum allowed number of - * transactions including the entry and its ancestors. */ - void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate, - uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); + void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); /** Try to calculate all in-mempool ancestors of entry. * (these are all calculated including the tx itself) @@ -840,14 +835,9 @@ private: * @param[out] descendants_to_remove Populated with the txids of entries that * exceed ancestor limits. It's the responsibility of the caller to * removeRecursive them. - * @param[in] ancestor_size_limit the max number of ancestral bytes allowed - * for any descendant - * @param[in] ancestor_count_limit the max number of ancestor transactions - * allowed for any descendant */ void UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, - const std::set& setExclude, std::set& descendants_to_remove, - uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs); + const std::set& setExclude, std::set& descendants_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Update ancestors of hash to add/remove it as a descendant transaction. */ void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Set ancestor state for an entry */ diff --git a/src/validation.cpp b/src/validation.cpp index 166aaa56fba..e9abdb7b172 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -320,9 +320,7 @@ void CChainState::MaybeUpdateMempoolForReorg( // previously-confirmed transactions back to the mempool. // UpdateTransactionsFromBlock finds descendants of any transactions in // the disconnectpool that were added back and cleans up the mempool state. - const uint64_t ancestor_count_limit = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); - const uint64_t ancestor_size_limit = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB) * 1000; - m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_size_limit, ancestor_count_limit); + m_mempool->UpdateTransactionsFromBlock(vHashUpdate); // Predicate to use for filtering transactions in removeForReorg. // Checks whether the transaction is still final and, if it spends a coinbase output, mature. From 9a3d825c30e8e6118d74a4e568744cb9d03f7f5d Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 6 Jun 2022 17:14:39 -0400 Subject: [PATCH 15/16] init: Remove redundant -*mempool*, -limit* queries Now that MemPoolOptions has correctly-determined max_size and limits members, perform sanity checks on that instead of re-determining the options. --- src/init.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9259745214c..ea8d01a7fac 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -930,11 +930,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainparams.GetConsensus().nMinimumChainWork.GetHex()); } - // mempool limits - int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000; - int64_t nMempoolSizeMin = args.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB) * 1000 * 40; - if (nMempoolSizeMax < 0 || nMempoolSizeMax < nMempoolSizeMin) - return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(nMempoolSizeMin / 1000000.0))); // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting. if (args.IsArgSet("-incrementalrelayfee")) { @@ -1433,6 +1428,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) }; ApplyArgsManOptions(args, mempool_opts); 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();) { From d1684beabe5b738c2cc83de83e1aaef11a761b69 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 8 Dec 2021 16:19:31 -0500 Subject: [PATCH 16/16] fees: Pass in a filepath instead of referencing gArgs --- src/Makefile.am | 2 ++ src/init.cpp | 5 +++-- src/policy/fees.cpp | 16 ++++++---------- src/policy/fees.h | 4 +++- src/policy/fees_args.cpp | 12 ++++++++++++ src/policy/fees_args.h | 15 +++++++++++++++ src/test/fuzz/policy_estimator.cpp | 8 +++++++- src/test/fuzz/policy_estimator_io.cpp | 8 +++++++- src/test/util/setup_common.cpp | 3 ++- 9 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 src/policy/fees_args.cpp create mode 100644 src/policy/fees_args.h diff --git a/src/Makefile.am b/src/Makefile.am index 734af78ae50..16c29a16b77 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -206,6 +206,7 @@ BITCOIN_CORE_H = \ outputtype.h \ policy/feerate.h \ policy/fees.h \ + policy/fees_args.h \ policy/packages.h \ policy/policy.h \ policy/rbf.h \ @@ -381,6 +382,7 @@ libbitcoin_node_a_SOURCES = \ node/interface_ui.cpp \ noui.cpp \ policy/fees.cpp \ + policy/fees_args.cpp \ policy/packages.cpp \ policy/rbf.cpp \ policy/settings.cpp \ diff --git a/src/init.cpp b/src/init.cpp index ea8d01a7fac..ee90abcc932 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -40,10 +40,11 @@ #include #include #include -#include #include +#include #include #include +#include #include #include #include @@ -1291,7 +1292,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.fee_estimator); // Don't initialize fee estimation with old data if we don't relay transactions, // as they would never get updated. - if (!ignores_incoming_txs) node.fee_estimator = std::make_unique(); + if (!ignores_incoming_txs) node.fee_estimator = std::make_unique(FeeestPath(args)); // sanitize comments per BIP-0014, format user agent and check total size std::vector uacomments; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index b39632364f1..27a6ab221fe 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -31,8 +31,6 @@ #include #include -static const char* FEE_ESTIMATES_FILENAME = "fee_estimates.dat"; - static constexpr double INF_FEERATE = 1e99; std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) @@ -529,8 +527,8 @@ bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock) } } -CBlockPolicyEstimator::CBlockPolicyEstimator() - : nBestSeenHeight(0), firstRecordedHeight(0), historicalFirst(0), historicalBest(0), trackedTxs(0), untrackedTxs(0) +CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath) + : m_estimation_filepath{estimation_filepath}, nBestSeenHeight{0}, firstRecordedHeight{0}, historicalFirst{0}, historicalBest{0}, trackedTxs{0}, untrackedTxs{0} { static_assert(MIN_BUCKET_FEERATE > 0, "Min feerate must be nonzero"); size_t bucketIndex = 0; @@ -548,10 +546,9 @@ CBlockPolicyEstimator::CBlockPolicyEstimator() longStats = std::unique_ptr(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE)); // If the fee estimation file is present, read recorded estimations - fs::path est_filepath = gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME; - CAutoFile est_file(fsbridge::fopen(est_filepath, "rb"), SER_DISK, CLIENT_VERSION); + CAutoFile est_file(fsbridge::fopen(m_estimation_filepath, "rb"), SER_DISK, CLIENT_VERSION); if (est_file.IsNull() || !Read(est_file)) { - LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", fs::PathToString(est_filepath)); + LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath)); } } @@ -907,10 +904,9 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation void CBlockPolicyEstimator::Flush() { FlushUnconfirmed(); - fs::path est_filepath = gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME; - CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION); + CAutoFile est_file(fsbridge::fopen(m_estimation_filepath, "wb"), SER_DISK, CLIENT_VERSION); if (est_file.IsNull() || !Write(est_file)) { - LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(est_filepath)); + LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath)); } } diff --git a/src/policy/fees.h b/src/policy/fees.h index dea1e1d31bc..9ee5c2938a2 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -6,6 +6,7 @@ #define BITCOIN_POLICY_FEES_H #include +#include #include #include #include @@ -179,9 +180,10 @@ private: */ static constexpr double FEE_SPACING = 1.05; + const fs::path m_estimation_filepath; public: /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */ - CBlockPolicyEstimator(); + CBlockPolicyEstimator(const fs::path& estimation_filepath); ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ diff --git a/src/policy/fees_args.cpp b/src/policy/fees_args.cpp new file mode 100644 index 00000000000..a3531153b5b --- /dev/null +++ b/src/policy/fees_args.cpp @@ -0,0 +1,12 @@ +#include + +#include + +namespace { +const char* FEE_ESTIMATES_FILENAME = "fee_estimates.dat"; +} // namespace + +fs::path FeeestPath(const ArgsManager& argsman) +{ + return argsman.GetDataDirNet() / FEE_ESTIMATES_FILENAME; +} diff --git a/src/policy/fees_args.h b/src/policy/fees_args.h new file mode 100644 index 00000000000..6b65ce0aa9f --- /dev/null +++ b/src/policy/fees_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_POLICY_FEES_ARGS_H +#define BITCOIN_POLICY_FEES_ARGS_H + +#include + +class ArgsManager; + +/** @return The fee estimates data file path. */ +fs::path FeeestPath(const ArgsManager& argsman); + +#endif // BITCOIN_POLICY_FEES_ARGS_H diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index e4d95f72a07..58c19a91cb7 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -15,15 +16,20 @@ #include #include +namespace { +const BasicTestingSetup* g_setup; +} // namespace + void initialize_policy_estimator() { static const auto testing_setup = MakeNoLogFileContext<>(); + g_setup = testing_setup.get(); } FUZZ_TARGET_INIT(policy_estimator, initialize_policy_estimator) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - CBlockPolicyEstimator block_policy_estimator; + CBlockPolicyEstimator block_policy_estimator{FeeestPath(*g_setup->m_node.args)}; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { CallOneOf( fuzzed_data_provider, diff --git a/src/test/fuzz/policy_estimator_io.cpp b/src/test/fuzz/policy_estimator_io.cpp index 9021d95954f..77402c260a8 100644 --- a/src/test/fuzz/policy_estimator_io.cpp +++ b/src/test/fuzz/policy_estimator_io.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -11,9 +12,14 @@ #include #include +namespace { +const BasicTestingSetup* g_setup; +} // namespace + void initialize_policy_estimator_io() { static const auto testing_setup = MakeNoLogFileContext<>(); + g_setup = testing_setup.get(); } FUZZ_TARGET_INIT(policy_estimator_io, initialize_policy_estimator_io) @@ -22,7 +28,7 @@ FUZZ_TARGET_INIT(policy_estimator_io, initialize_policy_estimator_io) FuzzedAutoFileProvider fuzzed_auto_file_provider = ConsumeAutoFile(fuzzed_data_provider); CAutoFile fuzzed_auto_file = fuzzed_auto_file_provider.open(); // Re-using block_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator object. - static CBlockPolicyEstimator block_policy_estimator; + static CBlockPolicyEstimator block_policy_estimator{FeeestPath(*g_setup->m_node.args)}; if (block_policy_estimator.Read(fuzzed_auto_file)) { block_policy_estimator.Write(fuzzed_auto_file); } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 60ddda1c0a1..0c9e880d675 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -177,7 +178,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); }); GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler); - m_node.fee_estimator = std::make_unique(); + m_node.fee_estimator = std::make_unique(FeeestPath(*m_node.args)); m_node.mempool = std::make_unique(MemPoolOptionsForTest(m_node)); m_cache_sizes = CalculateCacheSizes(m_args);