MOVEONLY: BIP125 Rule 2 to policy/rbf

3 files changed, 49 insertions, 32 deletions

diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index 3ff4c1f9c2..f6b3bc783a 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -75,3 +75,40 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
     return std::nullopt;
 }
 
+std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
+                                               const CTxMemPool& m_pool,
+                                               const CTxMemPool::setEntries& setIterConflicting)
+{
+    AssertLockHeld(m_pool.cs);
+    std::set<uint256> setConflictsParents;
+    for (const auto& mi : setIterConflicting) {
+        for (const CTxIn &txin : mi->GetTx().vin)
+        {
+            setConflictsParents.insert(txin.prevout.hash);
+        }
+    }
+
+    for (unsigned int j = 0; j < tx.vin.size(); j++)
+    {
+        // We don't want to accept replacements that require low
+        // feerate junk to be mined first. Ideally we'd keep track of
+        // the ancestor feerates and make the decision based on that,
+        // but for now requiring all new inputs to be confirmed works.
+        //
+        // Note that if you relax this to make RBF a little more useful,
+        // this may break the CalculateMempoolAncestors RBF relaxation,
+        // above. See the comment above the first CalculateMempoolAncestors
+        // call for more info.
+        if (!setConflictsParents.count(tx.vin[j].prevout.hash))
+        {
+            // Rather than check the UTXO set - potentially expensive -
+            // it's cheaper to just check if the new input refers to a
+            // tx that's in the mempool.
+            if (m_pool.exists(tx.vin[j].prevout.hash)) {
+                return strprintf("replacement %s adds unconfirmed input, idx %d",
+                                 tx.GetHash().ToString(), j);
+            }
+        }
+    }
+    return std::nullopt;
+}
diff --git a/src/policy/rbf.h b/src/policy/rbf.h
index 2a41ca8892..0f9a3d9856 100644
--- a/src/policy/rbf.h
+++ b/src/policy/rbf.h
@@ -49,4 +49,11 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMem
                                                   const CTxMemPool::setEntries& setIterConflicting,
                                                   CTxMemPool::setEntries& allConflicting)
                                                   EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
+
+/** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input
+ * was included in one of the original transactions."
+ * @returns error message if Rule #2 is broken, otherwise std::nullopt. */
+std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& m_pool,
+                                               const CTxMemPool::setEntries& setIterConflicting)
+                                               EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
 #endif // BITCOIN_POLICY_RBF_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 6f28c42db3..20970bceb8 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -821,6 +821,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
                                  "too many potential replacements", *err_string);
         }
+        // Enforce Rule #2.
+        if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) {
+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
+                                 "replacement-adds-unconfirmed", *err_string);
+        }
 
         // Check if it's economically rational to mine this transaction rather
         // than the ones it replaces.
@@ -829,38 +834,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
             nConflictingSize += it->GetTxSize();
         }
 
-        std::set<uint256> setConflictsParents;
-        for (const auto& mi : setIterConflicting) {
-            for (const CTxIn &txin : mi->GetTx().vin)
-            {
-                setConflictsParents.insert(txin.prevout.hash);
-            }
-        }
-
-        for (unsigned int j = 0; j < tx.vin.size(); j++)
-        {
-            // We don't want to accept replacements that require low
-            // feerate junk to be mined first. Ideally we'd keep track of
-            // the ancestor feerates and make the decision based on that,
-            // but for now requiring all new inputs to be confirmed works.
-            //
-            // Note that if you relax this to make RBF a little more useful,
-            // this may break the CalculateMempoolAncestors RBF relaxation,
-            // above. See the comment above the first CalculateMempoolAncestors
-            // call for more info.
-            if (!setConflictsParents.count(tx.vin[j].prevout.hash))
-            {
-                // Rather than check the UTXO set - potentially expensive -
-                // it's cheaper to just check if the new input refers to a
-                // tx that's in the mempool.
-                if (m_pool.exists(tx.vin[j].prevout.hash)) {
-                    return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed",
-                            strprintf("replacement %s adds unconfirmed input, idx %d",
-                                hash.ToString(), j));
-                }
-            }
-        }
-
         // The replacement must pay greater fees than the transactions it
         // replaces - if we did the bandwidth used by those conflicting
         // transactions would not be paid for.