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(); }; diff --git a/src/validation.cpp b/src/validation.cpp index b62691ca4c..09e04ff0dd 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, @@ -725,8 +723,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; @@ -1470,6 +1487,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"); @@ -1647,7 +1668,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()) { @@ -1831,6 +1853,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) { diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 7c89dd98a7..c9a200cfd9 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") @@ -283,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, @@ -297,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"], @@ -433,7 +339,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()