diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/policy/v3_policy.cpp | 6 | ||||
| -rw-r--r-- | src/test/fuzz/package_eval.cpp | 2 | ||||
| -rw-r--r-- | src/test/txpackage_tests.cpp | 144 | ||||
| -rw-r--r-- | src/test/util/txmempool.cpp | 28 | ||||
| -rw-r--r-- | src/validation.cpp | 151 |
5 files changed, 300 insertions, 31 deletions
diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp index bcf0b2b236..6bd043b8e3 100644 --- a/src/policy/v3_policy.cpp +++ b/src/policy/v3_policy.cpp @@ -91,7 +91,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v const auto parent_info = [&] { if (mempool_ancestors.size() > 0) { auto& mempool_parent = *mempool_ancestors.begin(); - Assume(mempool_parent->GetCountWithDescendants() == 1); return ParentInfo{mempool_parent->GetTx().GetHash(), mempool_parent->GetTx().GetWitnessHash(), mempool_parent->GetTx().version, @@ -135,10 +134,7 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v } } - // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks - // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions, - // any tx having a mempool ancestor would mean the package exceeds ancestor limits. - if (!Assume(!parent_info.m_has_mempool_descendant)) { + if (parent_info.m_has_mempool_descendant) { return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); } diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 122543ebad..53aedf23ea 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -314,7 +314,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) // just use result_package.m_state here. This makes the expect_valid check meaningless, but // we can still verify that the contents of m_tx_results are consistent with m_state. const bool expect_valid{result_package.m_state.IsValid()}; - Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr)); + Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, &tx_pool)); } else { // This is empty if it fails early checks, or "full" if transactions are looked at deeper Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty()); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index d5f3412aed..478121cc6f 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -6,6 +6,7 @@ #include <key_io.h> #include <policy/packages.h> #include <policy/policy.h> +#include <policy/rbf.h> #include <primitives/transaction.h> #include <script/script.h> #include <serialize.h> @@ -938,4 +939,147 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); } } + +BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup) +{ + mineBlocks(5); + LOCK(::cs_main); + size_t expected_pool_size = m_node.mempool->size(); + CKey child_key{GenerateRandomKey()}; + CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey())); + CKey grandchild_key{GenerateRandomKey()}; + CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey())); + + const CAmount coinbase_value{50 * COIN}; + // Test that de-duplication works. This is not actually package rbf. + { + // 1 parent paying 200sat, 1 child paying 300sat + Package package1; + // 1 parent paying 200sat, 1 child paying 500sat + Package package2; + // Package1 and package2 have the same parent. The children conflict. + auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_spk, + /*output_amount=*/coinbase_value - low_fee_amt, /*submit=*/false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + package1.push_back(tx_parent); + package2.push_back(tx_parent); + + CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 300, false)); + package1.push_back(tx_child_1); + CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 500, false)); + package2.push_back(tx_child_2); + + LOCK(m_node.mempool->cs); + const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false, std::nullopt); + if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_1.value()); + } + + // Check precise ResultTypes and mempool size. We know it_parent_1 and it_child_1 exist from above call + auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + expected_pool_size += 2; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false, std::nullopt); + if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_2.value()); + } + + // Check precise ResultTypes and mempool size. We know it_parent_2 and it_child_2 exist from above call + auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); + BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // child1 has been replaced + BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash()))); + } + + // Test package rbf. + { + CTransactionRef tx_parent_1 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false)); + CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_1, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 400, /*submit=*/false)); + + CTransactionRef tx_parent_2 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 800, /*submit=*/false)); + CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_2, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 800 - 200, /*submit=*/false)); + + CTransactionRef tx_parent_3 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 199, /*submit=*/false)); + CTransactionRef tx_child_3 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_3, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 199 - 1300, /*submit=*/false)); + + // In all packages, the parents conflict with each other + BOOST_CHECK(tx_parent_1->GetHash() != tx_parent_2->GetHash() && tx_parent_2->GetHash() != tx_parent_3->GetHash()); + + // 1 parent paying 200sat, 1 child paying 200sat. + Package package1{tx_parent_1, tx_child_1}; + // 1 parent paying 800sat, 1 child paying 200sat. + Package package2{tx_parent_2, tx_child_2}; + // 1 parent paying 199sat, 1 child paying 1300sat. + Package package3{tx_parent_3, tx_child_3}; + + const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_1.value()); + } + auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash()); + auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + expected_pool_size += 2; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // This replacement is actually not package rbf; the parent carries enough fees + // to replace the entire package on its own. + const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt); + if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_2.value()); + } + auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash()); + auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules + const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt); + if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_3.value()); + } + auto it_parent_3 = submit3.m_tx_results.find(tx_parent_3->GetWitnessHash()); + auto it_child_3 = submit3.m_tx_results.find(tx_child_3->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + + // package3 was considered as a package to replace both package2 transactions + BOOST_CHECK(it_parent_3->second.m_replaced_transactions.size() == 2); + BOOST_CHECK(it_child_3->second.m_replaced_transactions.empty()); + + std::vector<Wtxid> expected_package3_wtxids({tx_parent_3->GetWitnessHash(), tx_child_3->GetWitnessHash()}); + const auto package3_total_vsize{GetVirtualTransactionSize(*tx_parent_3) + GetVirtualTransactionSize(*tx_child_3)}; + BOOST_CHECK(it_parent_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids); + BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids); + BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); + BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + } + +} BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 249ce9503c..94d50bba50 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -7,6 +7,7 @@ #include <chainparams.h> #include <node/context.h> #include <node/mempool_args.h> +#include <policy/rbf.h> #include <policy/v3_policy.h> #include <txmempool.h> #include <util/check.h> @@ -68,6 +69,28 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString()); } + // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here) + if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) { + return strprintf("tx %s result replaced too many transactions", + wtxid.ToString()); + } + + // Replacements can't happen for subpackages larger than 2 + if (!atmp_result.m_replaced_transactions.empty() && + atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) { + return strprintf("tx %s was part of a too-large package RBF subpackage", + wtxid.ToString()); + } + + if (!atmp_result.m_replaced_transactions.empty() && mempool) { + LOCK(mempool->cs); + // If replacements occurred and it used 2 transactions, this is a package RBF and should result in a cluster of size 2 + if (atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() == 2) { + const auto cluster = mempool->GatherClusters({tx->GetHash()}); + if (cluster.size() != 2) return strprintf("tx %s has too many ancestors or descendants for a package rbf", wtxid.ToString()); + } + } + // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY}; if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) { @@ -108,6 +131,11 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, return strprintf("wtxid %s should not be in mempool", wtxid.ToString()); } } + for (const auto& tx_ref : atmp_result.m_replaced_transactions) { + if (mempool->exists(GenTxid::Txid(tx_ref->GetHash()))) { + return strprintf("tx %s should not be in mempool as it was replaced", tx_ref->GetWitnessHash().ToString()); + } + } } } return std::nullopt; diff --git a/src/validation.cpp b/src/validation.cpp index cf9bfe0e2b..c34d60f137 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -525,7 +525,7 @@ public: /* m_bypass_limits */ false, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, - /* m_allow_replacement */ false, + /* m_allow_replacement */ true, /* m_allow_sibling_eviction */ false, /* m_package_submission */ true, /* m_package_feerates */ true, @@ -603,8 +603,8 @@ public: /** * 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. + * package policy restrictions like no CPFP carve out (PackageMempoolChecks) + * and creates a PackageMempoolAcceptResult wrapping the result. * * If subpackage size > 1, calls AcceptMultipleTransactions() with the provided ATMPArgs. * @@ -667,12 +667,13 @@ private: // only tests that are fast should be done here (to avoid CPU DoS). bool PreChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); - // Run checks for mempool replace-by-fee. + // Run checks for mempool replace-by-fee, only used in AcceptSingleTransaction. bool ReplacementChecks(Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Enforce package mempool ancestor/descendant limits (distinct from individual - // ancestor/descendant limits done in PreChecks). + // ancestor/descendant limits done in PreChecks) and run Package RBF checks. bool PackageMempoolChecks(const std::vector<CTransactionRef>& txns, + std::vector<Workspace>& workspaces, int64_t total_vsize, PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); @@ -950,7 +951,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Note that these modifications are only applicable to single transaction scenarios; - // carve-outs and package RBF are disabled for multi-transaction evaluations. + // carve-outs are disabled for multi-transaction evaluations. CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits; // Calculate in-mempool ancestors, up to a limit. @@ -1089,10 +1090,9 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // descendant transaction of a direct conflict to pay a higher feerate than the transaction that // might replace them, under these rules. if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { - // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not - // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. - // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + // This fee-related failure is TX_RECONSIDERABLE because validating in a package may change + // the result. + return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } @@ -1117,16 +1117,15 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) } if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, m_pool.m_opts.incremental_relay_feerate, hash)}) { - // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not - // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. - // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + // Result may change in a package context + return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } return true; } bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns, + std::vector<Workspace>& workspaces, const int64_t total_vsize, PackageValidationState& package_state) { @@ -1137,12 +1136,88 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));})); + assert(txns.size() == workspaces.size()); + auto result = m_pool.CheckPackageLimits(txns, total_vsize); if (!result) { // This is a package-wide error, separate from an individual transaction error. return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", util::ErrorString(result).original); } - return true; + + // No conflicts means we're finished. Further checks are all RBF-only. + if (!m_subpackage.m_rbf) return true; + + // We're in package RBF context; replacement proposal must be size 2 + if (workspaces.size() != 2 || !Assume(IsChildWithParents(txns))) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child"); + } + + // If the package has in-mempool ancestors, we won't consider a package RBF + // since it would result in a cluster larger than 2. + // N.B. To relax this constraint we will need to revisit how CCoinsViewMemPool::PackageAddTransaction + // is being used inside AcceptMultipleTransactions to track available inputs while processing a package. + for (const auto& ws : workspaces) { + if (!ws.m_ancestors.empty()) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: new transaction cannot have mempool ancestors"); + } + } + + // Aggregate all conflicts into one set. + CTxMemPool::setEntries direct_conflict_iters; + for (Workspace& ws : workspaces) { + // Aggregate all conflicts into one set. + direct_conflict_iters.merge(ws.m_iters_conflicting); + } + + const auto& parent_ws = workspaces[0]; + const auto& child_ws = workspaces[1]; + + // Don't consider replacements that would cause us to remove a large number of mempool entries. + // This limit is not increased in a package RBF. Use the aggregate number of transactions. + if (const auto err_string{GetEntriesForConflicts(*child_ws.m_ptx, m_pool, direct_conflict_iters, + m_subpackage.m_all_conflicts)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: too many potential replacements", *err_string); + } + + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { + m_subpackage.m_conflicting_fees += it->GetModifiedFee(); + m_subpackage.m_conflicting_size += it->GetTxSize(); + } + + // Use the child as the transaction for attributing errors to. + const Txid& child_hash = child_ws.m_ptx->GetHash(); + if (const auto err_string{PaysForRBF(/*original_fees=*/m_subpackage.m_conflicting_fees, + /*replacement_fees=*/m_subpackage.m_total_modified_fees, + /*replacement_vsize=*/m_subpackage.m_total_vsize, + m_pool.m_opts.incremental_relay_feerate, child_hash)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: insufficient anti-DoS fees", *err_string); + } + + // Ensure this two transaction package is a "chunk" on its own; we don't want the child + // to be only paying anti-DoS fees + const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize); + const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize); + if (package_feerate <= parent_feerate) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: package feerate is less than parent feerate", + strprintf("package feerate %s <= parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString())); + } + + // Check if it's economically rational to mine this package rather than the ones it replaces. + // This takes the place of ReplacementChecks()'s PaysMoreThanConflicts() in the package RBF setting. + if (const auto err_tup{ImprovesFeerateDiagram(m_pool, direct_conflict_iters, m_subpackage.m_all_conflicts, m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: " + err_tup.value().second, ""); + } + + LogPrint(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s)\n", + txns.front()->GetHash().ToString(), txns.front()->GetWitnessHash().ToString(), + txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString()); + + + return true; } bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) @@ -1216,16 +1291,19 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) const bool bypass_limits = args.m_bypass_limits; std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry; + if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n", + LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n", it->GetTx().GetHash().ToString(), it->GetTx().GetWitnessHash().ToString(), + it->GetFee(), + it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - FormatMoney(ws.m_modified_fees - m_subpackage.m_conflicting_fees), - (int)entry->GetTxSize() - (int)m_subpackage.m_conflicting_size); + entry->GetFee(), + entry->GetTxSize()); TRACE7(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), @@ -1319,6 +1397,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); + if (!m_subpackage.m_replaced_transactions.empty()) { + LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with %u new one(s) for %s additional fees, %d delta bytes\n", + m_subpackage.m_replaced_transactions.size(), workspaces.size(), + m_subpackage.m_total_modified_fees - m_subpackage.m_conflicting_fees, + m_subpackage.m_total_vsize - static_cast<int>(m_subpackage.m_conflicting_size)); + } + // 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 : @@ -1362,7 +1447,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } - if (m_subpackage.m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (m_subpackage.m_rbf && !ReplacementChecks(ws)) { + if (ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { + // Failed for incentives-based fee reasons. Provide the effective feerate and which tx was included. + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), single_wtxid); + } + return MempoolAcceptResult::Failure(ws.m_state); + } // Perform the inexpensive checks first and avoid hashing and signature verification unless // those checks pass, to mitigate CPU exhaustion denial-of-service attacks. @@ -1394,6 +1485,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } + if (!m_subpackage.m_replaced_transactions.empty()) { + LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new transaction for %s additional fees, %d delta bytes\n", + m_subpackage.m_replaced_transactions.size(), + ws.m_modified_fees - m_subpackage.m_conflicting_fees, + ws.m_vsize - static_cast<int>(m_subpackage.m_conflicting_size)); + } + return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); } @@ -1435,11 +1533,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } // Make the coins created by this transaction available for subsequent transactions in the - // package to spend. Since we already checked conflicts in the package and we don't allow - // replacements, we don't need to track the coins spent. Note that this logic will need to be - // updated if package replace-by-fee is allowed in the future. - assert(!args.m_allow_replacement); - assert(!m_subpackage.m_rbf); + // package to spend. If there are no conflicts within the package, no transaction can spend a coin + // needed by another transaction in the package. We also need to make sure that no package + // tx replaces (or replaces the ancestor of) the parent of another package tx. As long as we + // check these two things, we don't need to track the coins spent. + // If a package tx conflicts with a mempool tx, PackageMempoolChecks() ensures later that any package RBF attempt + // has *no* in-mempool ancestors, so we don't have to worry about subsequent transactions in + // same package spending the same in-mempool outpoints. This needs to be revisited for general + // package RBF. m_viewmempool.PackageAddTransaction(ws.m_ptx); } @@ -1480,7 +1581,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. - if (txns.size() > 1 && !PackageMempoolChecks(txns, m_subpackage.m_total_vsize, package_state)) { + if (txns.size() > 1 && !PackageMempoolChecks(txns, workspaces, m_subpackage.m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } |