From fc02f77ca604f0221171bfde3059b34f5d0fb1cd Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sun, 24 Oct 2021 02:26:19 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/util/system.cpp b/src/util/system.cpp index 6e2638a5d6..e83ad11e80 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 07d7a533aa..04c66341d3 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 -- cgit v1.2.3 From ccbaf546a68d6cda8ed3efd0598c0e4121b366bb Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 12:12:04 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index f20c55dcb1..dfbcff4b1d 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 751a03f01c..e958bef78f 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 1905a4df29..12b40aa245 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 cd46652efc..027b9c6a37 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 1873bc1587..e6e8324c27 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 97ec95a166..07b12bc047 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 b775c85912..4252914353 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( -- cgit v1.2.3 From 0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 24 Jun 2022 12:30:14 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index 8dcaa609b5..d4ec6ecfb1 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)); -- cgit v1.2.3 From f1941e8bfd2eecc478c7660434b1ebf6a64095a0 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 18 Mar 2022 13:51:37 -0400 Subject: 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 | 23 ++++++++++++++++++++--- src/test/util/setup_common.h | 3 +++ src/txmempool.cpp | 5 +++-- src/txmempool.h | 9 +++++---- 12 files changed, 127 insertions(+), 16 deletions(-) create mode 100644 src/kernel/mempool_options.h create mode 100644 src/mempool_args.cpp create mode 100644 src/mempool_args.h (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index fa716af619..bdb279d176 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -173,11 +173,13 @@ BITCOIN_CORE_H = \ kernel/checks.h \ kernel/coinstats.h \ kernel/context.h \ + kernel/mempool_options.h \ key.h \ key_io.h \ logging.h \ logging/timer.h \ mapport.h \ + mempool_args.h \ memusage.h \ merkleblock.h \ net.h \ @@ -361,6 +363,7 @@ libbitcoin_node_a_SOURCES = \ kernel/coinstats.cpp \ kernel/context.cpp \ mapport.cpp \ + mempool_args.cpp \ net.cpp \ netgroup.cpp \ net_processing.cpp \ diff --git a/src/init.cpp b/src/init.cpp index dfbcff4b1d..3ad4dc2667 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -30,6 +30,7 @@ #include #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 0000000000..c3d54bf0f4 --- /dev/null +++ b/src/kernel/mempool_options.h @@ -0,0 +1,25 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +#ifndef BITCOIN_KERNEL_MEMPOOL_OPTIONS_H +#define BITCOIN_KERNEL_MEMPOOL_OPTIONS_H + +class CBlockPolicyEstimator; + +namespace kernel { +/** + * Options struct containing options for constructing a CTxMemPool. Default + * constructor populates the struct with sane default values which can be + * modified. + * + * Most of the time, this struct should be referenced as CTxMemPool::Options. + */ +struct MemPoolOptions { + /* Used to estimate appropriate transaction fees. */ + CBlockPolicyEstimator* estimator{nullptr}; + /* The ratio used to determine how often sanity checks will run. */ + int check_ratio{0}; +}; +} // namespace kernel + +#endif // BITCOIN_KERNEL_MEMPOOL_OPTIONS_H diff --git a/src/mempool_args.cpp b/src/mempool_args.cpp new file mode 100644 index 0000000000..578463b81a --- /dev/null +++ b/src/mempool_args.cpp @@ -0,0 +1,16 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#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 0000000000..1732bdaac9 --- /dev/null +++ b/src/mempool_args.h @@ -0,0 +1,15 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_MEMPOOL_ARGS_H +#define BITCOIN_MEMPOOL_ARGS_H + +class ArgsManager; +namespace kernel { +struct MemPoolOptions; +}; + +void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts); + +#endif // BITCOIN_MEMPOOL_ARGS_H diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index d4ec6ecfb1..4801635791 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #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 4f40608c4f..2d88ee295b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -3,6 +3,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#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 c2aaf486c5..9532610f8d 100644 --- a/src/test/fuzz/validation_load_mempool.cpp +++ b/src/test/fuzz/validation_load_mempool.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#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 d9fff85bf5..60ddda1c0a 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::fPruneMode; +using node::fReindex; using node::LoadChainstate; +using node::NodeContext; using node::RegenerateCommitments; using node::VerifyLoadedChainstate; -using node::fPruneMode; -using node::fReindex; const std::function 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 37407bcb92..ed2c5db7e6 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -90,6 +90,9 @@ struct BasicTestingSetup { ArgsManager m_args; }; + +CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node); + /** Testing setup that performs all steps up until right before * ChainstateManager gets initialized. Meant for testing ChainstateManager * initialization behaviour. diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 65c8b4ea60..84088aead6 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -451,8 +451,9 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, assert(int(nSigOpCostWithAncestors) >= 0); } -CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio) - : m_check_ratio(check_ratio), minerPolicyEstimator(estimator) +CTxMemPool::CTxMemPool(const Options& opts) + : m_check_ratio{opts.check_ratio}, + minerPolicyEstimator{opts.estimator} { _clear(); //lock free clear } diff --git a/src/txmempool.h b/src/txmempool.h index f5d5abc62e..32319ac181 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -14,6 +14,8 @@ #include #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 -- cgit v1.2.3 From 82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 28 Oct 2021 13:46:19 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index c3d54bf0f4..e8c129050e 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 578463b81a..aef4e478f3 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 e958bef78f..14cc9c169d 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 12b40aa245..14eecdb950 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 027b9c6a37..b3993b6a24 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 e6e8324c27..dd1a6441a0 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 07b12bc047..2cdde14144 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 84088aead6..3e75f2db8d 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 32319ac181..d5d573b4f9 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 4252914353..a14f82106f 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( -- cgit v1.2.3 From 386c9472c8764738282e6d163b42e15a8feda7ea Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 27 Jun 2022 15:47:00 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index bc63122025..8c745b07b9 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 d5d573b4f9..95fd56879f 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 -- cgit v1.2.3 From 51c7a41a5eb6fcb60333812c770d80227cf7b64d Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 11:48:29 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index 3ad4dc2667..efbc9f41d9 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); -- cgit v1.2.3 From aa9141cd8185cb7ad532bc16feb9d302b05d9697 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 28 Oct 2021 14:13:04 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index e8c129050e..35621e1a28 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 aef4e478f3..06d553cf44 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 3e75f2db8d..a2220e6874 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 95fd56879f..0a91cf31c7 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 a14f82106f..c1ff603faa 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 3b6cd509c6..0e27e117fa 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) */ -- cgit v1.2.3 From 1ecc77321deb61b9f6888e4e10752b9d972fd26e Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 21 Jun 2022 10:37:10 -0400 Subject: 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 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index efbc9f41d9..ea0d7640c6 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 35621e1a28..c4f34ff9f9 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 -- cgit v1.2.3 From 716bb5fbd31077bbe99d11a54d6c2c250afc8085 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Mar 2022 21:54:40 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index ea0d7640c6..9259745214 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 14eecdb950..a99ed73596 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 564ff50d29..36c70e9e66 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 b3993b6a24..e255708eff 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 c1ff603faa..431b89c32b 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 -- cgit v1.2.3 From 9333427014695ac235c96d48791098168dfdc9db Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Mar 2022 22:09:05 -0400 Subject: 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 (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index bdb279d176..734af78ae5 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 0000000000..083e9681e7 --- /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 c4f34ff9f9..a14abb6628 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 06d553cf44..e26cbe0275 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 1732bdaac9..9a4abe6618 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 a2220e6874..2c2b063133 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 0a91cf31c7..4cc62448f9 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 -- cgit v1.2.3 From 38af2bcf358a72b9457d370282e57f4be1c5c849 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 6 Jun 2022 21:19:50 -0400 Subject: mempoolaccept: Use limits from mempool in constructor --- src/validation.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 431b89c32b..166aaa56fb 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 -- cgit v1.2.3 From 9e93b1030182eff92ef91181e17c7dd498c7e164 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 14:43:59 -0400 Subject: node/ifaces: Use existing MemPoolLimits --- src/node/interfaces.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index a99ed73596..3c085ae6fb 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 { -- cgit v1.2.3 From 6c5c60c4124293d948735756f84efc85262ea66f Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 18 May 2022 14:44:25 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h index 083e9681e7..e192e7e6cd 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 2c2b063133..25019e98ab 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 4cc62448f9..2e2cbe526a 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 166aaa56fb..e9abdb7b17 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. -- cgit v1.2.3 From 9a3d825c30e8e6118d74a4e568744cb9d03f7f5d Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 6 Jun 2022 17:14:39 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index 9259745214..ea8d01a7fa 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();) { -- cgit v1.2.3 From d1684beabe5b738c2cc83de83e1aaef11a761b69 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 8 Dec 2021 16:19:31 -0500 Subject: 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 (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index 734af78ae5..16c29a16b7 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 ea8d01a7fa..ee90abcc93 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 b39632364f..27a6ab221f 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 dea1e1d31b..9ee5c2938a 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 0000000000..a3531153b5 --- /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 0000000000..6b65ce0aa9 --- /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 e4d95f72a0..58c19a91cb 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 9021d95954..77402c260a 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 60ddda1c0a..0c9e880d67 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); -- cgit v1.2.3