MOVEONLY: check that fees > direct conflicts to policy/rbf

3 files changed, 43 insertions, 24 deletions

diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index 95b74123c9..5d1db6b58d 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -129,3 +129,35 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
     }
     return std::nullopt;
 }
+
+std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting,
+                                                 CFeeRate newFeeRate,
+                                                 const uint256& hash)
+{
+    for (const auto& mi : setIterConflicting) {
+        // Don't allow the replacement to reduce the feerate of the
+        // mempool.
+        //
+        // We usually don't want to accept replacements with lower
+        // feerates than what they replaced as that would lower the
+        // feerate of the next block. Requiring that the feerate always
+        // be increased is also an easy-to-reason about way to prevent
+        // DoS attacks via replacements.
+        //
+        // We only consider the feerates of transactions being directly
+        // replaced, not their indirect descendants. While that does
+        // mean high feerate children are ignored when deciding whether
+        // or not to replace, we do require the replacement to pay more
+        // overall fees too, mitigating most cases.
+        CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize());
+        if (newFeeRate <= oldFeeRate)
+        {
+            return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
+                             hash.ToString(),
+                             newFeeRate.ToString(),
+                             oldFeeRate.ToString());
+        }
+    }
+    return std::nullopt;
+}
+
diff --git a/src/policy/rbf.h b/src/policy/rbf.h
index 6c4e218959..2c548152b5 100644
--- a/src/policy/rbf.h
+++ b/src/policy/rbf.h
@@ -69,4 +69,13 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTx
 std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors,
                                                    const std::set<uint256>& setConflicts,
                                                    const uint256& txid);
+
+/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each
+ * of the transactions in setIterConflicting.
+ * @param[in]   setIterConflicting  The set of mempool entries.
+ * @returns error message if fees insufficient, otherwise std::nullopt.
+ */
+std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting,
+                                                 CFeeRate newFeeRate, const uint256& hash);
+
 #endif // BITCOIN_POLICY_RBF_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 710fe2d873..b0403bffd2 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -782,30 +782,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     if (fReplacementTransaction)
     {
         CFeeRate newFeeRate(nModifiedFees, nSize);
-        for (const auto& mi : setIterConflicting) {
-            // Don't allow the replacement to reduce the feerate of the
-            // mempool.
-            //
-            // We usually don't want to accept replacements with lower
-            // feerates than what they replaced as that would lower the
-            // feerate of the next block. Requiring that the feerate always
-            // be increased is also an easy-to-reason about way to prevent
-            // DoS attacks via replacements.
-            //
-            // We only consider the feerates of transactions being directly
-            // replaced, not their indirect descendants. While that does
-            // mean high feerate children are ignored when deciding whether
-            // or not to replace, we do require the replacement to pay more
-            // overall fees too, mitigating most cases.
-            CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize());
-            if (newFeeRate <= oldFeeRate)
-            {
-                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
-                        strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
-                            hash.ToString(),
-                            newFeeRate.ToString(),
-                            oldFeeRate.ToString()));
-            }
+        if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) {
+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
         }
 
         // Calculate all conflicting entries and enforce Rule #5.