From 7d7f7a1189432b1b6245ba25df572229870567cb Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 13 Sep 2023 15:33:32 +0100 Subject: [PATCH 01/10] [policy] check for duplicate txids in package Duplicates of normal transactions would be found by looking for conflicting inputs, but this doesn't catch identical empty transactions. These wouldn't be valid but exiting early is good and AcceptPackage's result sanity checks assume non-duplicate transactions. --- src/policy/packages.cpp | 7 +++++++ src/test/txpackage_tests.cpp | 11 +++++++++++ test/functional/rpc_packages.py | 8 ++++---- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 6e70a94088a..a901ef8f38c 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -37,6 +37,13 @@ bool CheckPackage(const Package& txns, PackageValidationState& state) std::unordered_set later_txids; std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), [](const auto& tx) { return tx->GetHash(); }); + + // Package must not contain any duplicate transactions, which is checked by txid. This also + // includes transactions with duplicate wtxids and same-txid-different-witness transactions. + if (later_txids.size() != txns.size()) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-contains-duplicates"); + } + for (const auto& tx : txns) { for (const auto& input : tx->vin) { if (later_txids.find(input.prevout.hash) != later_txids.end()) { diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index c08d2748a62..01f0a836a3f 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -66,6 +66,17 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) BOOST_CHECK(!CheckPackage(package_too_large, state_too_large)); BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large"); + + // Packages can't contain transactions with the same txid. + Package package_duplicate_txids_empty; + for (auto i{0}; i < 3; ++i) { + CMutableTransaction empty_tx; + package_duplicate_txids_empty.emplace_back(MakeTransactionRef(empty_tx)); + } + PackageValidationState state_duplicates; + BOOST_CHECK(!CheckPackage(package_duplicate_txids_empty, state_duplicates)); + BOOST_CHECK_EQUAL(state_duplicates.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(state_duplicates.GetRejectReason(), "package-contains-duplicates"); } BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index ae1a498e28a..9c4960aa1ea 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -212,8 +212,8 @@ class RPCPackagesTest(BitcoinTestFramework): coin = self.wallet.get_utxo() # tx1 and tx2 share the same inputs - tx1 = self.wallet.create_self_transfer(utxo_to_spend=coin) - tx2 = self.wallet.create_self_transfer(utxo_to_spend=coin) + tx1 = self.wallet.create_self_transfer(utxo_to_spend=coin, fee_rate=DEFAULT_FEE) + tx2 = self.wallet.create_self_transfer(utxo_to_spend=coin, fee_rate=2*DEFAULT_FEE) # Ensure tx1 and tx2 are valid by themselves assert node.testmempoolaccept([tx1["hex"]])[0]["allowed"] @@ -222,8 +222,8 @@ class RPCPackagesTest(BitcoinTestFramework): self.log.info("Test duplicate transactions in the same package") testres = node.testmempoolaccept([tx1["hex"], tx1["hex"]]) assert_equal(testres, [ - {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"}, - {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"} + {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "package-contains-duplicates"}, + {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "package-contains-duplicates"} ]) self.log.info("Test conflicting transactions in the same package") From 3f01a3dab1c4ee37fd4093b6a0a3b622f53e231d Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 23 Aug 2023 15:35:26 +0100 Subject: [PATCH 02/10] [CCoinsViewMemPool] track non-base coins and allow Reset Temporary coins should not be available in separate subpackage submissions. Any mempool coins that are cached in m_view should be removed whenever mempool contents change, as they may be spent or no longer exist. --- src/txmempool.cpp | 7 +++++++ src/txmempool.h | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 79b2b4ec94a..70985dec00c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -982,6 +982,7 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const { if (ptx) { if (outpoint.n < ptx->vout.size()) { coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false); + m_non_base_coins.emplace(outpoint); return true; } else { return false; @@ -994,8 +995,14 @@ void CCoinsViewMemPool::PackageAddTransaction(const CTransactionRef& tx) { for (unsigned int n = 0; n < tx->vout.size(); ++n) { m_temp_added.emplace(COutPoint(tx->GetHash(), n), Coin(tx->vout[n], MEMPOOL_HEIGHT, false)); + m_non_base_coins.emplace(COutPoint(tx->GetHash(), n)); } } +void CCoinsViewMemPool::Reset() +{ + m_temp_added.clear(); + m_non_base_coins.clear(); +} size_t CTxMemPool::DynamicMemoryUsage() const { LOCK(cs); diff --git a/src/txmempool.h b/src/txmempool.h index a1867eb895a..4e68b711a2a 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -839,15 +839,27 @@ class CCoinsViewMemPool : public CCoinsViewBacked * validation, since we can access transaction outputs without submitting them to mempool. */ std::unordered_map m_temp_added; + + /** + * Set of all coins that have been fetched from mempool or created using PackageAddTransaction + * (not base). Used to track the origin of a coin, see GetNonBaseCoins(). + */ + mutable std::unordered_set m_non_base_coins; protected: const CTxMemPool& mempool; public: CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); + /** GetCoin, returning whether it exists and is not spent. Also updates m_non_base_coins if the + * coin is not fetched from base. */ bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; /** Add the coins created by this transaction. These coins are only temporarily stored in * 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; } + /** Clear m_temp_added and m_non_base_coins. */ + void Reset(); }; /** From 03b87c11ca0705e1d6147b90da33ce555f9f41c8 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Dec 2022 18:00:04 +0000 Subject: [PATCH 03/10] [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (1) Call AcceptSingleTransaction when there is only 1 transaction in the subpackage. This avoids calling PackageMempoolChecks() which enforces rules that don't need to be applied for a single transaction, i.e. disabling CPFP carve out. There is a slight change in the error type returned, as shown in the txpackage_tests change. When a transaction is the last one left in the package and its fee is too low, this returns a PCKG_TX instead of PCKG_POLICY. This interface is clearer; "package-fee-too-low" for 1 transaction would be a bit misleading. (2) Clean up m_view and m_viewmempool so that coins created in this sub-package evaluation are not available for other sub-package evaluations. The contents of the mempool may change, so coins that are available now might not be later. --- src/test/txpackage_tests.cpp | 6 ++- src/validation.cpp | 85 +++++++++++++++++++++++++++++++----- 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 01f0a836a3f..118a963c75c 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -821,18 +821,20 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) expected_pool_size += 1; BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); - // The child would have been validated on its own and failed, then submitted as a "package" of 1. + // The child would have been validated on its own and failed. BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_TX); BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "transaction failed"); auto it_parent = submit_rich_parent.m_tx_results.find(tx_parent_rich->GetWitnessHash()); + auto it_child = submit_rich_parent.m_tx_results.find(tx_child_poor->GetWitnessHash()); BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end()); + BOOST_CHECK(it_child != submit_rich_parent.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(high_parent_fee, GetVirtualTransactionSize(*tx_parent_rich))); - auto it_child = submit_rich_parent.m_tx_results.find(tx_child_poor->GetWitnessHash()); BOOST_CHECK(it_child != submit_rich_parent.m_tx_results.end()); BOOST_CHECK_EQUAL(it_child->second.m_result_type, MempoolAcceptResult::ResultType::INVALID); BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MEMPOOL_POLICY); diff --git a/src/validation.cpp b/src/validation.cpp index c45c847471a..639b263463b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -554,6 +554,19 @@ public: */ PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** + * Submission of a subpackage. + * If subpackage size == 1, calls AcceptSingleTransaction() with adjusted ATMPArgs to avoid + * package policy restrictions like no CPFP carve out (PackageMempoolChecks) and disabled RBF + * (m_allow_replacement), and creates a PackageMempoolAcceptResult wrapping the result. + * + * If subpackage size > 1, calls AcceptMultipleTransactions() with the provided ATMPArgs. + * + * Also cleans up all non-chainstate coins from m_view at the end. + */ + PackageMempoolAcceptResult AcceptSubPackage(const std::vector& subpackage, ATMPArgs& args) + EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + /** * Package (more specific than just multiple transactions) acceptance. Package must be a child * with all of its unconfirmed parents, and topologically sorted. @@ -1326,6 +1339,54 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, std::move(results)); } +PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector& subpackage, ATMPArgs& args) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(m_pool.cs); + auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) { + if (subpackage.size() > 1) { + return AcceptMultipleTransactions(subpackage, args); + } + const auto& tx = subpackage.front(); + ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + const auto single_res = AcceptSingleTransaction(tx, single_args); + PackageValidationState package_state_wrapped; + if (single_res.m_result_type != MempoolAcceptResult::ResultType::VALID) { + package_state_wrapped.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + } + return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}}); + }(); + // Clean up m_view and m_viewmempool so that other subpackage evaluations don't have access to + // coins they shouldn't. Keep some coins in order to minimize re-fetching coins from the UTXO set. + // + // There are 3 kinds of coins in m_view: + // (1) Temporary coins from the transactions in subpackage, constructed by m_viewmempool. + // (2) Mempool coins from transactions in the mempool, constructed by m_viewmempool. + // (3) Confirmed coins fetched from our current UTXO set. + // + // (1) Temporary coins need to be removed, regardless of whether the transaction was submitted. + // If the transaction was submitted to the mempool, m_viewmempool will be able to fetch them from + // there. If it wasn't submitted to mempool, it is incorrect to keep them - future calls may try + // to spend those coins that don't actually exist. + // (2) Mempool coins also need to be removed. If the mempool contents have changed as a result + // of submitting or replacing transactions, coins previously fetched from mempool may now be + // spent or nonexistent. Those coins need to be deleted from m_view. + // (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. + // 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()) { + // In addition to resetting m_viewmempool, we also need to manually delete these coins from + // m_view because it caches copies of the coins it fetched from m_viewmempool previously. + m_view.Uncache(outpoint); + } + // This deletes the temporary and mempool coins. + m_viewmempool.Reset(); + return result; +} + PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) { AssertLockHeld(cs_main); @@ -1384,15 +1445,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, LOCK(m_pool.cs); // Stores final results that won't change std::map results_final; - // Node operators are free to set their mempool policies however they please, nodes may receive - // transactions in different orders, and malicious counterparties may try to take advantage of - // policy differences to pin or delay propagation of transactions. As such, it's possible for - // some package transaction(s) to already be in the mempool, and we don't want to reject the - // entire package in that case (as that could be a censorship vector). De-duplicate the - // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with - // the new transactions. This ensures we don't double-count transaction counts and sizes when - // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. - ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); // Results from individual validation. "Nonfinal" because if a transaction fails by itself but // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not // reflected in this map). If a transaction fails more than once, we want to return the first @@ -1408,6 +1460,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // we know is that the inputs aren't available. if (m_pool.exists(GenTxid::Wtxid(wtxid))) { // Exact transaction already exists in the mempool. + // Node operators are free to set their mempool policies however they please, nodes may receive + // transactions in different orders, and malicious counterparties may try to take advantage of + // policy differences to pin or delay propagation of transactions. As such, it's possible for + // some package transaction(s) to already be in the mempool, and we don't want to reject the + // entire package in that case (as that could be a censorship vector). De-duplicate the + // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with + // the new transactions. This ensures we don't double-count transaction counts and sizes when + // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); @@ -1426,7 +1486,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } else { // Transaction does not already exist in the mempool. // Try submitting the transaction on its own. - const auto single_res = AcceptSingleTransaction(tx, single_args); + const auto single_package_res = AcceptSubPackage({tx}, args); + const auto& single_res = single_package_res.m_tx_results.at(wtxid); if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { // The transaction succeeded on its own and is now in the mempool. Don't include it // in package validation, because its fees should only be "used" once. @@ -1464,7 +1525,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } // Validate the (deduplicated) transactions as a package. Note that submission_result has its // own PackageValidationState; package_state_quit_early is unused past this point. - auto submission_result = AcceptMultipleTransactions(txns_package_eval, args); + auto submission_result = AcceptSubPackage(txns_package_eval, args); // Include already-in-mempool transaction results in the final result. for (const auto& [wtxid, mempoolaccept_res] : results_final) { Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); @@ -1472,7 +1533,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) { // Package validation failed because one or more transactions failed. Provide a result for - // each transaction; if AcceptMultipleTransactions() didn't return a result for a tx, + // each transaction; if a transaction doesn't have an entry in submission_result, // include the previous individual failure reason. submission_result.m_tx_results.insert(individual_results_nonfinal.cbegin(), individual_results_nonfinal.cend()); From 8ad7ad33929ee846a55a43c55732be0cb8973060 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 11:47:56 +0100 Subject: [PATCH 04/10] [validation] make PackageMempoolAcceptResult members mutable After the PackageMempoolAcceptResult is returned from AcceptMultipleTransactions, leave room for results to change due to LimitMempool() eviction. --- src/validation.cpp | 8 ++++---- src/validation.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 639b263463b..e835728a53c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -655,7 +655,7 @@ private: // The package may end up partially-submitted after size limiting; returns true if all // transactions are successfully added to the mempool, false otherwise. bool SubmitPackage(const ATMPArgs& args, std::vector& workspaces, PackageValidationState& package_state, - std::map& results) + std::map& results) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. @@ -1132,7 +1132,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& workspaces, PackageValidationState& package_state, - std::map& results) + std::map& results) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); @@ -1262,7 +1262,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: workspaces.reserve(txns.size()); std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces), [](const auto& tx) { return Workspace(tx); }); - std::map results; + std::map results; LOCK(m_pool.cs); @@ -1444,7 +1444,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, LOCK(m_pool.cs); // Stores final results that won't change - std::map results_final; + std::map results_final; // Results from individual validation. "Nonfinal" because if a transaction fails by itself but // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not // reflected in this map). If a transaction fails more than once, we want to return the first diff --git a/src/validation.h b/src/validation.h index d7ad86a5e85..aacc6933005 100644 --- a/src/validation.h +++ b/src/validation.h @@ -211,21 +211,21 @@ private: */ struct PackageMempoolAcceptResult { - const PackageValidationState m_state; + PackageValidationState m_state; /** * Map from wtxid to finished MempoolAcceptResults. The client is responsible * for keeping track of the transaction objects themselves. If a result is not * present, it means validation was unfinished for that transaction. If there * was a package-wide error (see result in m_state), m_tx_results will be empty. */ - std::map m_tx_results; + std::map m_tx_results; explicit PackageMempoolAcceptResult(PackageValidationState state, - std::map&& results) + std::map&& results) : m_state{state}, m_tx_results(std::move(results)) {} explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, - std::map&& results) + std::map&& results) : m_state{state}, m_tx_results(std::move(results)) {} /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */ From 9698b81828ff98820fa49c83ca364063233374c6 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 11:54:02 +0100 Subject: [PATCH 05/10] [refactor] back-fill results in AcceptPackage Instead of populating the last PackageMempoolAcceptResult with stuff from results_final and individual_results_nonfinal, fill results_final and create a PackageMempoolAcceptResult using that one. A future commit will add LimitMempoolSize() which may change the status of each of these transactions from "already in mempool" or "submitted to mempool" to "no longer in mempool". We will change those transactions' results here. A future commit also gets rid of the last AcceptSubPackage outside of the loop. It makes more sense to use results_final as the place where all results end up. --- src/validation.cpp | 54 +++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e835728a53c..3cd815da264 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1443,12 +1443,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, m_view.SetBackend(m_dummy); LOCK(m_pool.cs); - // Stores final results that won't change + // Stores results from which we will create the returned PackageMempoolAcceptResult. + // A result may be changed if a mempool transaction is evicted later due to LimitMempoolSize(). std::map results_final; - // Results from individual validation. "Nonfinal" because if a transaction fails by itself but - // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not - // reflected in this map). If a transaction fails more than once, we want to return the first - // result, when it was considered on its own. So changes will only be from invalid -> valid. + // Results from individual validation which will be returned if no other result is available for + // this transaction. "Nonfinal" because if a transaction fails by itself but succeeds later + // (i.e. when evaluated with a fee-bumping child), the result in this map may be discarded. std::map individual_results_nonfinal; bool quit_early{false}; std::vector txns_package_eval; @@ -1514,32 +1514,28 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } } - // Quit early because package validation won't change the result or the entire package has - // already been submitted. - if (quit_early || txns_package_eval.empty()) { - for (const auto& [wtxid, mempoolaccept_res] : individual_results_nonfinal) { - Assume(results_final.emplace(wtxid, mempoolaccept_res).second); - Assume(mempoolaccept_res.m_result_type == MempoolAcceptResult::ResultType::INVALID); + auto multi_submission_result = quit_early || txns_package_eval.empty() ? PackageMempoolAcceptResult(package_state_quit_early, {}) : + AcceptSubPackage(txns_package_eval, args); + PackageValidationState& package_state_final = multi_submission_result.m_state; + + for (const auto& tx : package) { + const auto& wtxid = tx->GetWitnessHash(); + if (multi_submission_result.m_tx_results.count(wtxid) > 0) { + // We shouldn't have re-submitted if the tx result was already in results_final. + Assume(results_final.count(wtxid) == 0); + results_final.emplace(wtxid, multi_submission_result.m_tx_results.at(wtxid)); + } else if (const auto it{results_final.find(wtxid)}; it != results_final.end()) { + // Already-in-mempool transaction. + Assume(it->second.m_result_type != MempoolAcceptResult::ResultType::INVALID); + Assume(individual_results_nonfinal.count(wtxid) == 0); + } else if (const auto it{individual_results_nonfinal.find(wtxid)}; it != individual_results_nonfinal.end()) { + Assume(it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + // Interesting result from previous processing. + results_final.emplace(wtxid, it->second); } - return PackageMempoolAcceptResult(package_state_quit_early, std::move(results_final)); } - // Validate the (deduplicated) transactions as a package. Note that submission_result has its - // own PackageValidationState; package_state_quit_early is unused past this point. - auto submission_result = AcceptSubPackage(txns_package_eval, args); - // Include already-in-mempool transaction results in the final result. - for (const auto& [wtxid, mempoolaccept_res] : results_final) { - Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); - Assume(mempoolaccept_res.m_result_type != MempoolAcceptResult::ResultType::INVALID); - } - if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) { - // Package validation failed because one or more transactions failed. Provide a result for - // each transaction; if a transaction doesn't have an entry in submission_result, - // include the previous individual failure reason. - submission_result.m_tx_results.insert(individual_results_nonfinal.cbegin(), - individual_results_nonfinal.cend()); - Assume(submission_result.m_tx_results.size() == package.size()); - } - return submission_result; + Assume(results_final.size() == package.size()); + return PackageMempoolAcceptResult(package_state_final, std::move(results_final)); } } // anon namespace From d227b7234cd4cfd7c593ffcf8e2f24573d1ebea5 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 25 Aug 2023 11:11:36 +0100 Subject: [PATCH 06/10] [validation] return correct result when already-in-mempool tx gets evicted Bug fix: a transaction may be in the mempool when package evaluation begins (so it is added to results_final with MEMPOOL_ENTRY or DIFFERENT_WITNESS), but get evicted due to another transaction submission. --- src/validation.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3cd815da264..70aa51b1028 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1525,9 +1525,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, Assume(results_final.count(wtxid) == 0); results_final.emplace(wtxid, multi_submission_result.m_tx_results.at(wtxid)); } else if (const auto it{results_final.find(wtxid)}; it != results_final.end()) { - // Already-in-mempool transaction. + // Already-in-mempool transaction. Check to see if it's still there, as it could have + // been evicted when LimitMempoolSize() was called. Assume(it->second.m_result_type != MempoolAcceptResult::ResultType::INVALID); Assume(individual_results_nonfinal.count(wtxid) == 0); + // Query by txid to include the same-txid-different-witness ones. + if (!m_pool.exists(GenTxid::Txid(tx->GetHash()))) { + package_state_final.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + TxValidationState mempool_full_state; + mempool_full_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); + // Replace the previous result. + results_final.erase(wtxid); + results_final.emplace(wtxid, MempoolAcceptResult::Failure(mempool_full_state)); + } } else if (const auto it{individual_results_nonfinal.find(wtxid)}; it != individual_results_nonfinal.end()) { Assume(it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); // Interesting result from previous processing. From 3ea71feb11c261f002ed918f91f3434fd8a23589 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 12:11:21 +0100 Subject: [PATCH 07/10] [validation] don't LimitMempoolSize in any subpackage submissions Don't do any mempool evictions until package validation is done, preventing the mempool minimum feerate from changing. Whether we submit transactions separately or as a package depends on whether they meet the mempool minimum feerate threshold, so it's best that the value not change while we are evaluating a package. This avoids a situation where we have a CPFP package in which the parents meet the mempool minimum feerate and are submitted by themselves, but they are evicted before we have submitted the child. --- src/validation.cpp | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 70aa51b1028..b8168c2e4c2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -455,7 +455,7 @@ public: * any transaction spending the same inputs as a transaction in the mempool is considered * a conflict. */ const bool m_allow_replacement; - /** When true, the mempool will not be trimmed when individual transactions are submitted in + /** 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. */ @@ -516,7 +516,7 @@ public: /* m_coins_to_uncache */ package_args.m_coins_to_uncache, /* m_test_accept */ package_args.m_test_accept, /* m_allow_replacement */ true, - /* m_package_submission */ false, + /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() /* m_package_feerates */ false, // only 1 transaction }; } @@ -652,8 +652,7 @@ private: // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. - // The package may end up partially-submitted after size limiting; returns true if all - // transactions are successfully added to the mempool, false otherwise. + // Does not call LimitMempoolSize(), so mempool max_size_bytes may be temporarily exceeded. bool SubmitPackage(const ATMPArgs& args, std::vector& workspaces, PackageValidationState& package_state, std::map& results) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); @@ -1187,32 +1186,21 @@ 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::vector all_package_wtxids; all_package_wtxids.reserve(workspaces.size()); std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); - // 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. + + // Add successful results. The returned results may change later if LimitMempoolSize() evicts them. for (Workspace& ws : workspaces) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : std::vector({ws.m_ptx->GetWitnessHash()}); - if (m_pool.exists(GenTxid::Wtxid(ws.m_ptx->GetWitnessHash()))) { - results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, - ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); - GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); - } else { - all_submitted = false; - ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); - package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - } + results.emplace(ws.m_ptx->GetWitnessHash(), + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); + GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); } return all_submitted; } @@ -1518,12 +1506,26 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, AcceptSubPackage(txns_package_eval, args); PackageValidationState& package_state_final = multi_submission_result.m_state; + // Make sure we haven't exceeded max mempool size. + // Package transactions that were submitted to mempool or already in mempool may be evicted. + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); if (multi_submission_result.m_tx_results.count(wtxid) > 0) { // We shouldn't have re-submitted if the tx result was already in results_final. Assume(results_final.count(wtxid) == 0); - results_final.emplace(wtxid, multi_submission_result.m_tx_results.at(wtxid)); + // If it was submitted, check to see if the tx is still in the mempool. It could have + // been evicted due to LimitMempoolSize() above. + const auto& txresult = multi_submission_result.m_tx_results.at(wtxid); + if (txresult.m_result_type == MempoolAcceptResult::ResultType::VALID && !m_pool.exists(GenTxid::Wtxid(wtxid))) { + package_state_final.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + TxValidationState mempool_full_state; + mempool_full_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); + results_final.emplace(wtxid, MempoolAcceptResult::Failure(mempool_full_state)); + } else { + results_final.emplace(wtxid, txresult); + } } else if (const auto it{results_final.find(wtxid)}; it != results_final.end()) { // Already-in-mempool transaction. Check to see if it's still there, as it could have // been evicted when LimitMempoolSize() was called. From d08696120e3647b4c2cd0ae8d6e57dea12418b7c Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Aug 2023 10:10:06 +0100 Subject: [PATCH 08/10] [test framework] add ability to spend only confirmed utxos Useful to ensure that the topologies of packages/transactions are as expected, preventing bugs caused by having unexpected mempool ancestors. --- test/functional/test_framework/wallet.py | 25 ++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index 4d751943536..035a482f4cb 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -208,7 +208,7 @@ class MiniWallet: assert_equal(self._mode, MiniWalletMode.ADDRESS_OP_TRUE) return self._address - def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True) -> dict: + def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True, confirmed_only=False) -> dict: """ Returns a utxo and marks it as spent (pops it from the internal list) @@ -224,19 +224,23 @@ class MiniWallet: utxo_filter = reversed(mature_coins) # By default the largest utxo if vout is not None: utxo_filter = filter(lambda utxo: vout == utxo['vout'], utxo_filter) + if confirmed_only: + utxo_filter = filter(lambda utxo: utxo['confirmations'] > 0, utxo_filter) index = self._utxos.index(next(utxo_filter)) if mark_as_spent: return self._utxos.pop(index) else: return self._utxos[index] - def get_utxos(self, *, include_immature_coinbase=False, mark_as_spent=True): + def get_utxos(self, *, include_immature_coinbase=False, mark_as_spent=True, confirmed_only=False): """Returns the list of all utxos and optionally mark them as spent""" if not include_immature_coinbase: blocks_height = self._test_node.getblockchaininfo()['blocks'] utxo_filter = filter(lambda utxo: not utxo['coinbase'] or COINBASE_MATURITY - 1 <= blocks_height - utxo['height'], self._utxos) else: utxo_filter = self._utxos + if confirmed_only: + utxo_filter = filter(lambda utxo: utxo['confirmations'] > 0, utxo_filter) utxos = deepcopy(list(utxo_filter)) if mark_as_spent: self._utxos = [] @@ -286,14 +290,15 @@ class MiniWallet: locktime=0, sequence=0, fee_per_output=1000, - target_weight=0 + target_weight=0, + confirmed_only=False ): """ Create and return a transaction that spends the given UTXOs and creates a certain number of outputs with equal amounts. The output amounts can be set by amount_per_output or automatically calculated with a fee_per_output. """ - utxos_to_spend = utxos_to_spend or [self.get_utxo()] + utxos_to_spend = utxos_to_spend or [self.get_utxo(confirmed_only=confirmed_only)] sequence = [sequence] * len(utxos_to_spend) if type(sequence) is int else sequence assert_equal(len(utxos_to_spend), len(sequence)) @@ -333,9 +338,17 @@ class MiniWallet: "tx": tx, } - def create_self_transfer(self, *, fee_rate=Decimal("0.003"), fee=Decimal("0"), utxo_to_spend=None, locktime=0, sequence=0, target_weight=0): + def create_self_transfer(self, *, + fee_rate=Decimal("0.003"), + fee=Decimal("0"), + utxo_to_spend=None, + locktime=0, + sequence=0, + target_weight=0, + confirmed_only=False + ): """Create and return a tx with the specified fee. If fee is 0, use fee_rate, where the resulting fee may be exact or at most one satoshi higher than needed.""" - utxo_to_spend = utxo_to_spend or self.get_utxo() + utxo_to_spend = utxo_to_spend or self.get_utxo(confirmed_only=confirmed_only) assert fee_rate >= 0 assert fee >= 0 # calculate fee From a67f460c3fd1c7eb8070623666d887eefccff0d6 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Aug 2023 10:30:57 +0100 Subject: [PATCH 09/10] [refactor] split setup in mempool_limit test We want to be able to re-use fill_mempool so that none of the tests affect each other. Change the logs from info to debug because they are otherwise repeated many times in the test output. --- test/functional/mempool_limit.py | 44 +++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index f3f4b42ad06..9cf1e9b16e4 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -34,29 +34,27 @@ class MempoolLimitTest(BitcoinTestFramework): ]] self.supports_cli = False - def run_test(self): + def fill_mempool(self): + """Fill mempool until eviction.""" + self.log.info("Fill the mempool until eviction is triggered and the mempoolminfee rises") txouts = gen_return_txouts() node = self.nodes[0] - miniwallet = MiniWallet(node) + miniwallet = self.wallet relayfee = node.getnetworkinfo()['relayfee'] - self.log.info('Check that mempoolminfee is minrelaytxfee') - assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) - assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - tx_batch_size = 1 num_of_batches = 75 # Generate UTXOs to flood the mempool # 1 to create a tx initially that will be evicted from the mempool later - # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO + # 75 transactions each with a fee rate higher than the previous one # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee # And 2 more for the package cpfp test - self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1 + 2) + self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size)) # Mine 99 blocks so that the UTXOs are allowed to be spent self.generate(node, COINBASE_MATURITY - 1) - self.log.info('Create a mempool tx that will be evicted') + self.log.debug("Create a mempool tx that will be evicted") tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] # Increase the tx fee rate to give the subsequent transactions a higher priority in the mempool @@ -64,21 +62,37 @@ class MempoolLimitTest(BitcoinTestFramework): # by 130 should result in a fee that corresponds to 2x of that fee rate base_fee = relayfee * 130 - self.log.info("Fill up the mempool with txs with higher fee rate") - for batch_of_txid in range(num_of_batches): - fee = (batch_of_txid + 1) * base_fee - create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts) + self.log.debug("Fill up the mempool with txs with higher fee rate") + with node.assert_debug_log(["rolling minimum fee bumped"]): + for batch_of_txid in range(num_of_batches): + fee = (batch_of_txid + 1) * base_fee + create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts) - self.log.info('The tx should be evicted by now') + self.log.debug("The tx should be evicted by now") # The number of transactions created should be greater than the ones present in the mempool assert_greater_than(tx_batch_size * num_of_batches, len(node.getrawmempool())) # Initial tx created should not be present in the mempool anymore as it had a lower fee rate assert tx_to_be_evicted_id not in node.getrawmempool() - self.log.info('Check that mempoolminfee is larger than minrelaytxfee') + self.log.debug("Check that mempoolminfee is larger than minrelaytxfee") assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + def run_test(self): + node = self.nodes[0] + self.wallet = MiniWallet(node) + miniwallet = self.wallet + + # Generate coins needed to create transactions in the subtests (excluding coins used in fill_mempool). + self.generate(miniwallet, 10) + + relayfee = node.getnetworkinfo()['relayfee'] + self.log.info('Check that mempoolminfee is minrelaytxfee') + assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) + assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + + self.fill_mempool() + # Deliberately try to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool self.log.info('Create a mempool tx that will not pass mempoolminfee') assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee) From 32c1dd1ad65af0ad4d36a56d2ca32a8481237e68 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 14 Aug 2023 10:53:39 +0100 Subject: [PATCH 10/10] [test] mempool coins disappearing mid-package evaluation Test for scenario(s) outlined in PR 28251. Test what happens when a package transaction spends a mempool coin which is fetched and then disappears mid-package evaluation due to eviction or replacement. --- test/functional/mempool_limit.py | 164 ++++++++++++++++++++++++++++++- 1 file changed, 163 insertions(+), 1 deletion(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 9cf1e9b16e4..0abebbec025 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -78,13 +78,172 @@ class MempoolLimitTest(BitcoinTestFramework): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + 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')) + + self.fill_mempool() + current_info = node.getmempoolinfo() + mempoolmin_feerate = current_info["mempoolminfee"] + + package_hex = [] + # UTXOs to be spent by the ultimate child transaction + parent_utxos = [] + + evicted_weight = 8000 + # 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=(mempoolmin_feerate / 1000) * (evicted_weight // 4) + Decimal('0.000001'), + target_weight=evicted_weight, + 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_weight = 100000 + num_big_parents = 3 + assert_greater_than(parent_weight * num_big_parents, current_info["maxmempool"] - current_info["bytes"]) + parent_fee = (100 * mempoolmin_feerate / 1000) * (parent_weight // 4) + + big_parent_txids = [] + for i in range(num_big_parents): + parent = self.wallet.create_self_transfer(fee=parent_fee, target_weight=parent_weight, 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_raises_rpc_error(-26, "mempool full", node.submitpackage, package_hex) + + # 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") + + 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')) + + self.fill_mempool() + 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. + double_spent_utxo = self.wallet.get_utxo(confirmed_only=True) + replaced_tx = self.wallet.send_self_transfer( + from_node=node, + utxo_to_spend=double_spent_utxo, + fee_rate=mempoolmin_feerate, + confirmed_only=True + ) + # Already in mempool when package is submitted. + 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 + # 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"], + fee_rate=mempoolmin_feerate - Decimal('0.00001'), + confirmed_only=True) + + self.wallet.rescan_utxos() + + # Parent that replaces the parent of cpfp_parent. + replacement_tx = self.wallet.create_self_transfer( + utxo_to_spend=double_spent_utxo, + fee_rate=10*mempoolmin_feerate, + confirmed_only=True + ) + parent_utxos = [cpfp_parent["new_utxo"], replacement_tx["new_utxo"]] + + # Create a child spending everything, CPFPing the low-feerate parent. + approx_child_vsize = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos)["tx"].get_vsize() + cpfp_fee = (2 * mempoolmin_feerate / 1000) * (cpfp_parent["tx"].get_vsize() + approx_child_vsize) - cpfp_parent["fee"] + child = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=int(cpfp_fee * COIN)) + # It's very important that the cpfp_parent is before replacement_tx so that its input (from + # replaced_tx) is first looked up *before* replacement_tx is submitted. + package_hex = [cpfp_parent["hex"], replacement_tx["hex"], child["hex"]] + + # Package should be submitted, temporarily exceeding maxmempool, and then evicted. + assert_raises_rpc_error(-26, "bad-txns-inputs-missingorspent", node.submitpackage, package_hex) + + # Maximum size must never be exceeded. + assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) + + resulting_mempool_txids = node.getrawmempool() + # The replacement should be successful. + assert replacement_tx["txid"] in resulting_mempool_txids + # The replaced tx and all of its descendants must not be in mempool. + assert replaced_tx["txid"] not in resulting_mempool_txids + assert cpfp_parent["txid"] not in resulting_mempool_txids + assert child["txid"] not in resulting_mempool_txids + + def run_test(self): node = self.nodes[0] self.wallet = MiniWallet(node) miniwallet = self.wallet # Generate coins needed to create transactions in the subtests (excluding coins used in fill_mempool). - self.generate(miniwallet, 10) + self.generate(miniwallet, 20) relayfee = node.getnetworkinfo()['relayfee'] self.log.info('Check that mempoolminfee is minrelaytxfee') @@ -163,6 +322,9 @@ class MempoolLimitTest(BitcoinTestFramework): self.stop_node(0) self.nodes[0].assert_start_raises_init_error(["-maxmempool=4"], "Error: -maxmempool must be at least 5 MB") + self.test_mid_package_replacement() + self.test_mid_package_eviction() + if __name__ == '__main__': MempoolLimitTest().main()