From ba02c30b8a635db88100916a1bb9c40fb0b47ab6 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Jul 2025 14:43:06 -0400 Subject: [PATCH 1/6] [doc] always CleanupTemporaryCoins after a mempool trim --- src/validation.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 7992b85712..59b7c2fe98 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1470,6 +1470,10 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef // Limit the mempool, if appropriate. if (!args.m_package_submission && !args.m_bypass_limits) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + // If mempool contents change, then the m_view cache is dirty. Given this isn't a package + // submission, we won't be using the cache anymore, but clear it anyway for clarity. + CleanupTemporaryCoins(); + if (!m_pool.exists(ws.m_hash)) { // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package. ws.m_state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool full"); @@ -1831,6 +1835,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Make sure we haven't exceeded max mempool size. // Package transactions that were submitted to mempool or already in mempool may be evicted. + // If mempool contents change, then the m_view cache is dirty. It has already been cleared above. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); for (const auto& tx : package) { From 98ba2b1db2eb81e08b550ba0d5069a9289f30e13 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Jul 2025 12:38:12 -0400 Subject: [PATCH 2/6] [doc] MemPoolAccept coins views --- src/validation.cpp | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 59b7c2fe98..0ac04cfa5f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -725,8 +725,27 @@ private: private: CTxMemPool& m_pool; + + /** Holds a cached view of available coins from the UTXO set, mempool, and artificial temporary coins (to enable package validation). + * The view doesn't track whether a coin previously existed but has now been spent. We detect conflicts in other ways: + * - conflicts within a transaction are checked in CheckTransaction (bad-txns-inputs-duplicate) + * - conflicts within a package are checked in IsWellFormedPackage (conflict-in-package) + * - conflicts with an existing mempool transaction are found in CTxMemPool::GetConflictTx and replacements are allowed + * The temporary coins should persist between individual transaction checks so that package validation is possible, + * but must be cleaned up when we finish validating a subpackage, whether accepted or rejected. The cache must also + * be cleared when mempool contents change (when a changeset is applied or when the mempool trims itself) because it + * can return cached coins that no longer exist in the backend. Use CleanupTemporaryCoins() anytime you are finished + * with a SubPackageState or call LimitMempoolSize(). + */ CCoinsViewCache m_view; + + // These are the two possible backends for m_view. + /** When m_view is connected to m_viewmempool as its backend, it can pull coins from the mempool and from the UTXO + * set. This is also where temporary coins are stored. */ CCoinsViewMemPool m_viewmempool; + /** When m_view is connected to m_dummy, it can no longer look up coins from the mempool or UTXO set (meaning no disk + * operations happen), but can still return coins it accessed previously. Useful for keeping track of which coins + * were pulled from disk. */ CCoinsView m_dummy; Chainstate& m_active_chainstate; @@ -1651,7 +1670,8 @@ void MemPoolAccept::CleanupTemporaryCoins() // (3) Confirmed coins don't need to be removed. The chainstate has not changed (we are // holding cs_main and no blocks have been processed) so the confirmed tx cannot disappear like // a mempool tx can. The coin may now be spent after we submitted a tx to mempool, but - // we have already checked that the package does not have 2 transactions spending the same coin. + // we have already checked that the package does not have 2 transactions spending the same coin + // and we check whether a mempool transaction spends conflicting coins (CTxMemPool::GetConflictTx). // Keeping them in m_view is an optimization to not re-fetch confirmed coins if we later look up // inputs for this transaction again. for (const auto& outpoint : m_viewmempool.GetNonBaseCoins()) { From d8140f5f050076bfa129169c1223cab94b243a6e Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Jul 2025 12:40:48 -0400 Subject: [PATCH 3/6] don't make a copy of m_non_base_coins --- src/txmempool.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/txmempool.h b/src/txmempool.h index 687c474f8e..2c77aef0de 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -940,7 +940,7 @@ public: * m_temp_added and cannot be flushed to the back end. Only used for package validation. */ void PackageAddTransaction(const CTransactionRef& tx); /** Get all coins in m_non_base_coins. */ - std::unordered_set GetNonBaseCoins() const { return m_non_base_coins; } + const std::unordered_set& GetNonBaseCoins() const { return m_non_base_coins; } /** Clear m_temp_added and m_non_base_coins. */ void Reset(); }; From c3cd7fcb2cd9f9911f8251a6f29120f65c63ea37 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Jul 2025 14:22:17 -0400 Subject: [PATCH 4/6] [doc] remove references to now-nonexistent Finalize() function --- src/validation.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0ac04cfa5f..f2f6098e21 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -469,10 +469,8 @@ public: const bool m_allow_replacement; /** When true, allow sibling eviction. This only occurs in single transaction package settings. */ const bool m_allow_sibling_eviction; - /** When true, the mempool will not be trimmed when any transactions are submitted in - * Finalize(). Instead, limits should be enforced at the end to ensure the package is not - * partially submitted. - */ + /** Used to skip the LimitMempoolSize() call within AcceptSingleTransaction(). This should be used when multiple + * AcceptSubPackage calls are expected and the mempool will be trimmed at the end of AcceptPackage(). */ const bool m_package_submission; /** When true, use package feerates instead of individual transaction feerates for fee-based * policies such as mempool min fee and min relay fee. @@ -548,7 +546,7 @@ public: /* m_test_accept */ package_args.m_test_accept, /* m_allow_replacement */ true, /* m_allow_sibling_eviction */ true, - /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() + /* m_package_submission */ true, // trim at the end of AcceptPackage() /* m_package_feerates */ false, // only 1 transaction /* m_client_maxfeerate */ package_args.m_client_maxfeerate, /* m_allow_carveouts */ false, From f3a613aa5bb780f5d85821fa00742f5a99df81b5 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Jul 2025 14:35:09 -0400 Subject: [PATCH 5/6] [cleanup] delete brittle test_mid_package_eviction This test was introduced in #28251 to ensure that the mempool is not trimmed in the middle of a package evaluation and the m_view cache is updated when evictions and replacements happen so coins are no longer visible in subsequent package transactions. These two things have coverage in other tests as well, and are pretty unlikely to happen. This test is also brittle: it requires evaluation of the parents in a particular order, and creates a transaction that itself is not enough to trigger eviction but will be pushed out immediately by the package spending from it. While the current magic number 2000 works, we do not have a way to query remaining space in the mempool if mempool data structures change, and it can differ across platforms. --- test/functional/mempool_limit.py | 93 -------------------------------- 1 file changed, 93 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 7c89dd98a7..fe922f97c8 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -177,98 +177,6 @@ class MempoolLimitTest(BitcoinTestFramework): evicted_feerate_btc_per_kvb = entry["fees"]["modified"] / (Decimal(entry["vsize"]) / 1000) assert_greater_than(evicted_feerate_btc_per_kvb, max_parent_feerate) - def test_mid_package_eviction(self): - node = self.nodes[0] - self.log.info("Check a package where each parent passes the current mempoolminfee but would cause eviction before package submission terminates") - - self.restart_node(0, extra_args=self.extra_args[0]) - - # Restarting the node resets mempool minimum feerate - assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) - assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - - fill_mempool(self, node) - current_info = node.getmempoolinfo() - mempoolmin_feerate = current_info["mempoolminfee"] - - package_hex = [] - # UTXOs to be spent by the ultimate child transaction - parent_utxos = [] - - evicted_vsize = 2000 - # Mempool transaction which is evicted due to being at the "bottom" of the mempool when the - # mempool overflows and evicts by descendant score. It's important that the eviction doesn't - # happen in the middle of package evaluation, as it can invalidate the coins cache. - mempool_evicted_tx = self.wallet.send_self_transfer( - from_node=node, - fee_rate=mempoolmin_feerate, - target_vsize=evicted_vsize, - confirmed_only=True - ) - # Already in mempool when package is submitted. - assert mempool_evicted_tx["txid"] in node.getrawmempool() - - # This parent spends the above mempool transaction that exists when its inputs are first - # looked up, but disappears later. It is rejected for being too low fee (but eligible for - # reconsideration), and its inputs are cached. When the mempool transaction is evicted, its - # coin is no longer available, but the cache could still contains the tx. - cpfp_parent = self.wallet.create_self_transfer( - utxo_to_spend=mempool_evicted_tx["new_utxo"], - fee_rate=mempoolmin_feerate - Decimal('0.00001'), - confirmed_only=True) - package_hex.append(cpfp_parent["hex"]) - parent_utxos.append(cpfp_parent["new_utxo"]) - assert_equal(node.testmempoolaccept([cpfp_parent["hex"]])[0]["reject-reason"], "mempool min fee not met") - - self.wallet.rescan_utxos() - - # Series of parents that don't need CPFP and are submitted individually. Each one is large and - # high feerate, which means they should trigger eviction but not be evicted. - parent_vsize = 25000 - num_big_parents = 3 - # Need to be large enough to trigger eviction - # (note that the mempool usage of a tx is about three times its vsize) - assert_greater_than(parent_vsize * num_big_parents * 3, current_info["maxmempool"] - current_info["bytes"]) - parent_feerate = 100 * mempoolmin_feerate - - big_parent_txids = [] - for i in range(num_big_parents): - parent = self.wallet.create_self_transfer(fee_rate=parent_feerate, target_vsize=parent_vsize, confirmed_only=True) - parent_utxos.append(parent["new_utxo"]) - package_hex.append(parent["hex"]) - big_parent_txids.append(parent["txid"]) - # There is room for each of these transactions independently - assert node.testmempoolaccept([parent["hex"]])[0]["allowed"] - - # Create a child spending everything, bumping cpfp_parent just above mempool minimum - # feerate. It's important not to bump too much as otherwise mempool_evicted_tx would not be - # evicted, making this test much less meaningful. - approx_child_vsize = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos)["tx"].get_vsize() - cpfp_fee = (mempoolmin_feerate / 1000) * (cpfp_parent["tx"].get_vsize() + approx_child_vsize) - cpfp_parent["fee"] - # Specific number of satoshis to fit within a small window. The parent_cpfp + child package needs to be - # - When there is mid-package eviction, high enough feerate to meet the new mempoolminfee - # - When there is no mid-package eviction, low enough feerate to be evicted immediately after submission. - magic_satoshis = 1200 - cpfp_satoshis = int(cpfp_fee * COIN) + magic_satoshis - - child = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=cpfp_satoshis) - package_hex.append(child["hex"]) - - # Package should be submitted, temporarily exceeding maxmempool, and then evicted. - with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]): - assert_equal(node.submitpackage(package_hex)["package_msg"], "transaction failed") - - # Maximum size must never be exceeded. - assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) - - # Evicted transaction and its descendants must not be in mempool. - resulting_mempool_txids = node.getrawmempool() - assert mempool_evicted_tx["txid"] not in resulting_mempool_txids - assert cpfp_parent["txid"] not in resulting_mempool_txids - assert child["txid"] not in resulting_mempool_txids - for txid in big_parent_txids: - assert txid in resulting_mempool_txids - def test_mid_package_replacement(self): node = self.nodes[0] self.log.info("Check a package where an early tx depends on a later-replaced mempool tx") @@ -433,7 +341,6 @@ class MempoolLimitTest(BitcoinTestFramework): self.test_mid_package_eviction_success() self.test_mid_package_replacement() - self.test_mid_package_eviction() self.test_rbf_carveout_disallowed() From b6d4688f77df9e31fd64e2be300f55bb8e944bd0 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 16 Jul 2025 13:27:23 -0400 Subject: [PATCH 6/6] [doc] reword comments in test_mid_package_replacement The comment about eviction seems to be erroneously copy-pasted. Reword another comment for clarity. --- test/functional/mempool_limit.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index fe922f97c8..c9a200cfd9 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -191,9 +191,7 @@ class MempoolLimitTest(BitcoinTestFramework): current_info = node.getmempoolinfo() mempoolmin_feerate = current_info["mempoolminfee"] - # Mempool transaction which is evicted due to being at the "bottom" of the mempool when the - # mempool overflows and evicts by descendant score. It's important that the eviction doesn't - # happen in the middle of package evaluation, as it can invalidate the coins cache. + # Mempool transaction is replaced by a package transaction. double_spent_utxo = self.wallet.get_utxo(confirmed_only=True) replaced_tx = self.wallet.send_self_transfer( from_node=node, @@ -205,8 +203,8 @@ class MempoolLimitTest(BitcoinTestFramework): assert replaced_tx["txid"] in node.getrawmempool() # This parent spends the above mempool transaction that exists when its inputs are first - # looked up, but disappears later. It is rejected for being too low fee (but eligible for - # reconsideration), and its inputs are cached. When the mempool transaction is evicted, its + # looked up, but will disappear if the replacement occurs. It is rejected for being too low fee (but eligible for + # reconsideration), and its inputs are cached. When the mempool transaction is replaced, its # coin is no longer available, but the cache could still contain the tx. cpfp_parent = self.wallet.create_self_transfer( utxo_to_spend=replaced_tx["new_utxo"],