From 7d7f7a1189432b1b6245ba25df572229870567cb Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 13 Sep 2023 15:33:32 +0100 Subject: [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 +++++++++++ 2 files changed, 18 insertions(+) (limited to 'src') diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 6e70a94088..a901ef8f38 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 c08d2748a6..01f0a836a3 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) -- cgit v1.2.3 From 3f01a3dab1c4ee37fd4093b6a0a3b622f53e231d Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 23 Aug 2023 15:35:26 +0100 Subject: [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(+) (limited to 'src') diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 79b2b4ec94..70985dec00 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 a1867eb895..4e68b711a2 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(); }; /** -- cgit v1.2.3 From 03b87c11ca0705e1d6147b90da33ce555f9f41c8 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Dec 2022 18:00:04 +0000 Subject: [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(-) (limited to 'src') diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 01f0a836a3..118a963c75 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 c45c847471..639b263463 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()); -- cgit v1.2.3 From 8ad7ad33929ee846a55a43c55732be0cb8973060 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 11:47:56 +0100 Subject: [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(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 639b263463..e835728a53 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 d7ad86a5e8..aacc693300 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 */ -- cgit v1.2.3 From 9698b81828ff98820fa49c83ca364063233374c6 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 11:54:02 +0100 Subject: [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 | 58 +++++++++++++++++++++++++----------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index e835728a53..3cd815da26 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); - } - 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; + 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); + } + } + Assume(results_final.size() == package.size()); + return PackageMempoolAcceptResult(package_state_final, std::move(results_final)); } } // anon namespace -- cgit v1.2.3 From d227b7234cd4cfd7c593ffcf8e2f24573d1ebea5 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 25 Aug 2023 11:11:36 +0100 Subject: [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(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 3cd815da26..70aa51b102 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. -- cgit v1.2.3 From 3ea71feb11c261f002ed918f91f3434fd8a23589 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 10 Aug 2023 12:11:21 +0100 Subject: [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(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 70aa51b102..b8168c2e4c 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. -- cgit v1.2.3