diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 1bf90c7e9193b5..0bc5c3bf393ae9 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -216,32 +216,18 @@ std::optional CheckConflictTopology(const CTxMemPool::setEntries& d std::optional CheckMinerScores(CAmount replacement_fees, int64_t replacement_vsize, - const CTxMemPool::setEntries& direct_conflicts, const CTxMemPool::setEntries& original_transactions) { // Note that this assumes no in-mempool ancestors const CFeeRate replacement_miner_score(replacement_fees, replacement_vsize); - for (const auto& entry : direct_conflicts) { - const bool conflict_is_v3{entry->GetSharedTx()->nVersion == 3}; - CFeeRate original_score(entry->GetModifiedFee(), entry->GetTxSize()); - // If the original transaction is v3, we can calculate the exact miner score and avoid overestimating. - if (conflict_is_v3) { - original_score = std::min(original_score, CFeeRate(entry->GetModFeesWithAncestors(), entry->GetSizeWithAncestors())); - } - if (replacement_miner_score < original_score) { - return strprintf("replacement miner score lower than %s miner score of direct conflict; %s < %s", - conflict_is_v3 ? "calculated" : "estimated", - replacement_miner_score.ToString(), - original_score.ToString()); - } - } for (const auto& entry : original_transactions) { - const CFeeRate original_ancestor_feerate(entry->GetModFeesWithAncestors(), entry->GetSizeWithAncestors()); - if (replacement_miner_score < original_ancestor_feerate) { - return strprintf("replacement miner score lower than ancestor feerate of original tx; %s < %s", + CFeeRate original_miner_score(entry->GetModFeesWithAncestors(), entry->GetSizeWithAncestors()); + original_miner_score = std::min(original_miner_score, CFeeRate(entry->GetModifiedFee(), entry->GetTxSize())); + if (replacement_miner_score < original_miner_score) { + return strprintf("replacement miner score lower than miner score of original tx; %s < %s", replacement_miner_score.ToString(), - original_ancestor_feerate.ToString()); + original_miner_score.ToString()); } } return std::nullopt; diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 826ae1c1289692..7f0b3492454a3b 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -112,14 +112,12 @@ std::optional PaysForRBF(CAmount original_fees, std::optional CheckConflictTopology(const CTxMemPool::setEntries& direct_conflicts); /** Attempt to ensure replacement transactions are more incentive compatible to mine than the - * transactions they are replacing. Currently, this function requires that feerate - * of the replacement transaction(s) be higher than the individual feerates of - * all directly conflicting transactions and the ancestor feerates of all original transactions. - * If the direct conflict is v3, we calculate the miner score instead. + * transactions they are replacing. This is computing exact miner scores assuming + * that each original transaction was a part of a cluster up to size 2, i.e. + * passed CheckConflictTopology(). * */ std::optional CheckMinerScores(CAmount replacement_fees, int64_t replacement_vsize, - const CTxMemPool::setEntries& direct_conflicts, const CTxMemPool::setEntries& original_transactions); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index 46e36282e134e0..17d0aa35efb731 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -231,34 +231,30 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) // Tests for CheckMinerScores + /* FIXME these tests are stale; rework them */ + // These tests use modified fees (including prioritisation), not base fees. BOOST_CHECK(CheckMinerScores(entry5_low->GetFee() + entry6_low_prioritised->GetFee() + 1, entry5_low->GetTxSize() + entry6_low_prioritised->GetTxSize(), - {entry5_low}, set_56_low).has_value()); BOOST_CHECK(CheckMinerScores(entry5_low->GetModifiedFee() + entry6_low_prioritised->GetModifiedFee() + 1, entry5_low->GetTxSize() + entry6_low_prioritised->GetTxSize(), - {entry5_low}, set_56_low) == std::nullopt); // High-feerate ancestors don't help raise the replacement's miner score. BOOST_CHECK(CheckMinerScores(entry1_normal->GetFee() - 1, entry1_normal->GetTxSize(), - set_12_normal, set_12_normal).has_value()); // Replacement must be higher than the individual feerate of direct conflicts. // Note entry4_high's individual feerate is higher than its ancestor feerate BOOST_CHECK(CheckMinerScores(entry4_high->GetFee() - 1, entry4_high->GetTxSize(), - {entry4_high}, - {entry4_high}).has_value()); + {entry4_high}) == std::nullopt); BOOST_CHECK(CheckMinerScores(entry4_high->GetFee() - 1, entry4_high->GetTxSize(), - {entry3_low}, set_34_cpfp) == std::nullopt); - } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 648a1b11698b63..8ef5ae9fbfa35d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1119,8 +1119,14 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: parent paying for child anti-DoS", ""); } + // Check if direct conflicts are all members of a up to size 2 cluster + if (const auto err_string{CheckConflictTopology(direct_conflict_iters)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: unable to compute mining score", *err_string); + } + // Check if it's economically rational to mine this package rather than the ones it replaces. - if (const auto err_string{CheckMinerScores(m_total_modified_fees, m_total_vsize, direct_conflict_iters, + if (const auto err_string{CheckMinerScores(m_total_modified_fees, m_total_vsize, m_all_conflicts)}) { return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: insufficient feerate", *err_string);