Refactor CheckMinerScores to only check proper mining scores

src/policy/rbf.cpp

@@ -216,32 +216,18 @@ std::optional<std::string> CheckConflictTopology(const CTxMemPool::setEntries& d
 
 std::optional<std::string> 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;

src/policy/rbf.h

@@ -112,14 +112,12 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
 std::optional<std::string> 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<std::string> CheckMinerScores(CAmount replacement_fees,
                                             int64_t replacement_vsize,
-                                            const CTxMemPool::setEntries& direct_conflicts,
                                             const CTxMemPool::setEntries& original_transactions);
 
 #endif // BITCOIN_POLICY_RBF_H

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

src/validation.cpp

@@ -1119,8 +1119,14 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& 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);