From 4dd374c50e8a52e66afc6f134bca8b251c53afa9 Mon Sep 17 00:00:00 2001 From: rustaceanrob Date: Fri, 22 May 2026 11:09:46 +0100 Subject: [PATCH] mempool: Remove time as a key of entries The discussion on `LimitMempoolSize` has indicated that it is possible for a high-fee package to get evicted from the mempool if the parent times out due to the `Expire` logic. This is a incentive compatibility bug, and it would surprise me if miners have not already removed this behavior. ref: https://github.com/bitcoin/bitcoin/issues/33510 --- src/init.cpp | 5 ++++- src/kernel/mempool_options.h | 5 ----- src/kernel/mempool_removal_reason.cpp | 1 - src/kernel/mempool_removal_reason.h | 1 - src/node/mempool_args.cpp | 3 --- src/node/mempool_persist.cpp | 7 ++----- src/test/fuzz/package_eval.cpp | 1 - src/test/fuzz/tx_pool.cpp | 7 ------- src/txmempool.cpp | 17 ---------------- src/txmempool.h | 28 ++++----------------------- src/validation.cpp | 5 ----- src/validationinterface.h | 1 - 12 files changed, 10 insertions(+), 71 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 2f1367dd4807..8a7d01433874 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -465,7 +465,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from an external file on startup. Obfuscated blocks are not supported.", 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("-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("-mempoolexpiry=", strprintf("Deprecated. Time-based mempool expiry has been removed. This option has no effect."), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-minimumchainwork=", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet3: %s, testnet4: %s, signet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnet4ChainParams->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 (0 = auto, up to %d, <0 = leave that many cores free, default: %d)", MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -849,6 +849,9 @@ bool AppInitParameterInteraction(const ArgsManager& args) if (args.IsArgSet("-limitdescendantsize")) { InitWarning(_("Option '-limitdescendantsize' is given but descendant size limits have been replaced with cluster size limits (see -limitclustersize). This option has no effect.")); } + if (args.IsArgSet("-mempoolexpiry")) { + InitWarning(_("Option '-mempoolexpiry' is set but time-based mempool expiry has been removed. This option has no effect.")); + } // Error if network-specific options (-addnode, -connect, etc) are // specified in default section of config file, but not overridden diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index 392c837fbbf9..5f4978a8c98c 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -8,8 +8,6 @@ #include #include -#include - #include #include @@ -19,8 +17,6 @@ class ValidationSignals; static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -maxmempool when blocksonly is set */ static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5}; -/** Default for -mempoolexpiry, expiration time for mempool transactions in hours */ -static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336}; /** Whether to fall back to legacy V1 serialization when writing mempool.dat */ static constexpr bool DEFAULT_PERSIST_V1_DAT{false}; /** Default for -acceptnonstdtxn */ @@ -38,7 +34,6 @@ 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_HOURS}}; CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE}; /** A fee rate smaller than this is considered zero fee (for relaying, mining and transaction creation) */ CFeeRate min_relay_feerate{DEFAULT_MIN_RELAY_TX_FEE}; diff --git a/src/kernel/mempool_removal_reason.cpp b/src/kernel/mempool_removal_reason.cpp index df27590c7ab6..a421b5329c22 100644 --- a/src/kernel/mempool_removal_reason.cpp +++ b/src/kernel/mempool_removal_reason.cpp @@ -10,7 +10,6 @@ std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept { switch (r) { - case MemPoolRemovalReason::EXPIRY: return "expiry"; case MemPoolRemovalReason::SIZELIMIT: return "sizelimit"; case MemPoolRemovalReason::REORG: return "reorg"; case MemPoolRemovalReason::BLOCK: return "block"; diff --git a/src/kernel/mempool_removal_reason.h b/src/kernel/mempool_removal_reason.h index 53c2ff1c31c0..b16a617965be 100644 --- a/src/kernel/mempool_removal_reason.h +++ b/src/kernel/mempool_removal_reason.h @@ -11,7 +11,6 @@ * this is passed to the notification signal. */ enum class MemPoolRemovalReason { - EXPIRY, //!< Expired from mempool SIZELIMIT, //!< Removed in size limiting REORG, //!< Removed for reorganization BLOCK, //!< Removed for block diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp index d5ff50be96bf..f7c8ea63de9a 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -19,7 +19,6 @@ #include #include -#include #include using common::AmountErrMsg; @@ -54,8 +53,6 @@ util::Result ApplyArgsManOptions(const ArgsManager& argsman, const CChainP mempool_opts.max_size_bytes = *mb * 1'000'000; } - if (auto hours = argsman.GetIntArg("-mempoolexpiry")) mempool_opts.expiry = std::chrono::hours{*hours}; - // incremental relay fee sets the minimum feerate increase necessary for replacement in the mempool // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting. if (const auto arg{argsman.GetArg("-incrementalrelayfee")}) { diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp index dcb05267edda..af1b201d2744 100644 --- a/src/node/mempool_persist.cpp +++ b/src/node/mempool_persist.cpp @@ -51,7 +51,6 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active } int64_t count = 0; - int64_t expired = 0; int64_t failed = 0; int64_t already_there = 0; int64_t unbroadcast = 0; @@ -100,7 +99,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active if (amountdelta && opts.apply_fee_delta_priority) { pool.PrioritiseTransaction(tx->GetHash(), amountdelta); } - if (nTime > TicksSinceEpoch(now - pool.m_opts.expiry)) { + { LOCK(cs_main); const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false); if (accepted.m_result_type == MempoolAcceptResult::ResultType::VALID) { @@ -116,8 +115,6 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active ++failed; } } - } else { - ++expired; } if (active_chainstate.m_chainman.m_interrupt) return false; @@ -146,7 +143,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active return false; } - LogInfo("Imported mempool transactions from file: %i succeeded, %i failed, %i expired, %i already there, %i waiting for initial broadcast\n", count, failed, expired, already_there, unbroadcast); + LogInfo("Imported mempool transactions from file: %i succeeded, %i failed, %i already there, %i waiting for initial broadcast\n", count, failed, already_there, unbroadcast); return true; } diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 1cc2caa396e9..ea07d7fc7ae4 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -124,7 +124,6 @@ std::unique_ptr MakeMempool(FuzzedDataProvider& fuzzed_data_provider mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 50); mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 50); mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 200) * 1'000'000; - mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange(0, 999)}; // Only interested in 2 cases: sigop cost 0 or when single legacy sigop cost is >> 1KvB nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(0, 1) * 10'000; diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index f70dd710b356..a3a69b82d452 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -86,8 +86,6 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr ToString(fuzzed_data_provider.ConsumeIntegralInRange(1, 250))); args.ForceSetArg("-maxmempool", ToString(fuzzed_data_provider.ConsumeIntegralInRange(0, 200))); - args.ForceSetArg("-mempoolexpiry", - ToString(fuzzed_data_provider.ConsumeIntegralInRange(0, 999))); } void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Chainstate& chainstate) @@ -129,11 +127,6 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha LOCK2(::cs_main, tx_pool.cs); tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange(0U, tx_pool.DynamicMemoryUsage() * 2)); } - if (fuzzed_data_provider.ConsumeBool()) { - // Try expiry - LOCK2(::cs_main, tx_pool.cs); - tx_pool.Expire(GetMockTime() - std::chrono::seconds(fuzzed_data_provider.ConsumeIntegral())); - } WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1)); g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue(); } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index a22cd2b199d7..6f7ee15ee025 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -808,23 +808,6 @@ bool CTxMemPool::CheckPolicyLimits(const CTransactionRef& tx) return changeset->CheckMemPoolPolicyLimits(); } -int CTxMemPool::Expire(std::chrono::seconds time) -{ - AssertLockHeld(cs); - Assume(!m_have_changeset); - indexed_transaction_set::index::type::iterator it = mapTx.get().begin(); - setEntries toremove; - while (it != mapTx.get().end() && it->GetTime() < time) { - toremove.insert(mapTx.project<0>(it)); - it++; - } - setEntries stage; - for (txiter removeit : toremove) { - CalculateDescendants(removeit, stage); - } - RemoveStaged(stage, MemPoolRemovalReason::EXPIRY); - return stage.size(); -} CFeeRate CTxMemPool::GetMinFee(size_t sizelimit) const { LOCK(cs); diff --git a/src/txmempool.h b/src/txmempool.h index ae59057ca62b..cb563f967596 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -92,17 +92,7 @@ struct mempoolentry_wtxid } }; -class CompareTxMemPoolEntryByEntryTime -{ -public: - bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const - { - return a.GetTime() < b.GetTime(); - } -}; - // Multi_index tag names -struct entry_time {}; struct index_by_wtxid {}; /** @@ -160,8 +150,8 @@ struct TxMempoolInfo * interfaces to the rest of the codebase, such as: * - to validation for replace-by-fee calculations and cluster size limits * when evaluating unconfirmed transactions - * - to validation for evicting transactions due to expiry or the mempool size - * limit being hit + * - to validation for evicting transactions due to the mempool size limit being + * hit * - to validation for updating the mempool to be consistent with the best * chain after a new block is connected or after a reorg. * - to net_processing for ordering transactions that are to-be-announced to @@ -172,10 +162,9 @@ struct TxMempoolInfo * functions.) * * Within CTxMemPool, the mempool entries are stored in a boost::multi_index - * mapTx, which sorts the mempool on 3 criteria: + * mapTx, which sorts the mempool on 2 criteria: * - transaction hash (txid) * - witness-transaction hash (wtxid) - * - time in mempool * * We also maintain a map from COutPoint to the (in-mempool) transaction that * spends it (mapNextTx). This allows us to recover from a reorg and find @@ -221,12 +210,6 @@ class CTxMemPool boost::multi_index::tag, mempoolentry_wtxid, SaltedWtxidHasher - >, - // sorted by entry time - boost::multi_index::ordered_non_unique< - boost::multi_index::tag, - boost::multi_index::identity, - CompareTxMemPoolEntryByEntryTime > > >; @@ -457,9 +440,6 @@ class CTxMemPool */ void TrimToSize(size_t sizelimit, std::vector* pvNoSpendsRemaining = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); - /** Expire all transaction (and their dependencies) in the mempool older than time. Return the number of removed transactions. */ - int Expire(std::chrono::seconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - /** * Calculate the ancestor and cluster count for the given transaction. * The counts include the transaction itself. @@ -593,7 +573,7 @@ class CTxMemPool * This class is used for all mempool additions and associated removals (eg * due to rbf). Removals that don't need to be evaluated for acceptance, * such as removing transactions that appear in a block, or due to reorg, - * or removals related to mempool limiting or expiry do not need to use + * or removals related to mempool limiting do not need to use * this. * * Callers can interleave calls to StageAddition()/StageRemoval(), and diff --git a/src/validation.cpp b/src/validation.cpp index 056b3baa5713..ee71f15c63ac 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -260,11 +260,6 @@ static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache) { AssertLockHeld(::cs_main); AssertLockHeld(pool.cs); - int expired = pool.Expire(GetTime() - pool.m_opts.expiry); - if (expired != 0) { - LogDebug(BCLog::MEMPOOL, "Expired %i transactions from the memory pool\n", expired); - } - std::vector vNoSpendsRemaining; pool.TrimToSize(pool.m_opts.max_size_bytes, &vNoSpendsRemaining); for (const COutPoint& removed : vNoSpendsRemaining) diff --git a/src/validationinterface.h b/src/validationinterface.h index 09808cbcbf4f..49ba5cbbbf62 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -77,7 +77,6 @@ class CValidationInterface { * This notification fires for transactions that are removed from the * mempool for the following reasons: * - * - EXPIRY (expired from mempool after -mempoolexpiry hours) * - SIZELIMIT (removed in size limiting if the mempool exceeds -maxmempool megabytes) * - REORG (removed during a reorg) * - CONFLICT (removed because it conflicts with in-block transaction)