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());