Treat SPK conflicts the same as RBF-optin TxIn conflicts (except never DoS ban)

Author: luke-jr

src/policy/rbf.cpp

@@ -119,12 +119,17 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
 }
 
 std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
-                                                   const std::set<Txid>& direct_conflicts,
-                                                   const uint256& txid)
+                                                   const std::map<Txid, bool>& direct_conflicts,
+                                                   const uint256& txid, bool* const out_violates_policy)
 {
     for (CTxMemPool::txiter ancestorIt : ancestors) {
         const Txid& hashAncestor = ancestorIt->GetTx().GetHash();
-        if (direct_conflicts.count(hashAncestor)) {
+        const auto& conflictit = direct_conflicts.find(hashAncestor);
+        if (conflictit != direct_conflicts.end()) {
+            if (!conflictit->second /* mere SPK conflict, NOT invalid */) {
+                if (out_violates_policy) *out_violates_policy = true;
+                continue;
+            }
             return strprintf("%s spends conflicting transaction %s",
                              txid.ToString(),
                              hashAncestor.ToString());

src/policy/rbf.h

@@ -88,11 +88,12 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTx
  * @param[in]   direct_conflicts    Set of txids corresponding to the mempool conflicts
  *                                  (candidates to be replaced).
  * @param[in]   txid                Transaction ID, included in the error message if violation occurs.
- * @returns error message if the sets intersect, std::nullopt if they are disjoint.
+ * @param[out]  out_violates_policy Assigned to true if there are any policy-only conflicts.
+ * @returns error message if the sets intersect (consensus-only conflicts), std::nullopt if they are disjoint or only intersect on policy matters.
  */
 std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
-                                                   const std::set<Txid>& direct_conflicts,
-                                                   const uint256& txid);
+                                                   const std::map<Txid, bool>& direct_conflicts,
+                                                   const uint256& txid, bool* out_violates_policy);
 
 /** Check that the feerate of the replacement transaction(s) is higher than the feerate of each
  * of the transactions in iters_conflicting.

src/test/rbf_tests.cpp

@@ -216,14 +216,22 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
     BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4_high->GetModifiedFee(), entry4_high->GetTxSize()), unused_txid).has_value());
 
     // Tests for EntriesAndTxidsDisjoint
-    BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {tx1->GetHash()}, unused_txid) == std::nullopt);
-    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx3->GetHash()}, unused_txid) == std::nullopt);
-    BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx2->GetHash()}, unused_txid).has_value());
-    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx1->GetHash()}, unused_txid).has_value());
-    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value());
+    bool violates_policy{false};
+    BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {{tx1->GetHash(), true}}, unused_txid, &violates_policy) == std::nullopt);
+    BOOST_CHECK(!violates_policy);
+    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {{tx3->GetHash(), true}}, unused_txid, &violates_policy) == std::nullopt);
+    BOOST_CHECK(!violates_policy);
+    BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {{tx2->GetHash(), true}}, unused_txid, &violates_policy).has_value());
+    BOOST_CHECK(!violates_policy);
+    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {{tx1->GetHash(), true}}, unused_txid, &violates_policy).has_value());
+    BOOST_CHECK(!violates_policy);
+    BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {{tx2->GetHash(), true}}, unused_txid, &violates_policy).has_value());
+    BOOST_CHECK(!violates_policy);
     // EntriesAndTxidsDisjoint does not calculate descendants of iters_conflicting; it uses whatever
     // the caller passed in. As such, no error is returned even though entry2_normal is a descendant of tx1.
-    BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx1->GetHash()}, unused_txid) == std::nullopt);
+    BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {{tx1->GetHash(), true}}, unused_txid, &violates_policy) == std::nullopt);
+    BOOST_CHECK(!violates_policy);
+    // TODO: Add tests for policy-only conflicts
 
     // Tests for PaysForRBF
     const CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};

src/validation.cpp

@@ -630,7 +630,8 @@ class MemPoolAccept
         explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {}
         /** Txids of mempool transactions that this transaction directly conflicts with or may
          * replace via sibling eviction. */
-        std::set<Txid> m_conflicts;
+        /** .second=true is a consensus conflict, and .second=false is a policy conflict. */
+        std::map<Txid, bool> m_conflicts_incl_policy;
         /** Iterators to mempool entries that this transaction directly conflicts with or may
          * replace via sibling eviction. */
         CTxMemPool::setEntries m_iters_conflicting;
@@ -852,7 +853,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
                 // Transaction conflicts with a mempool tx, but we're not allowing replacements.
                 return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed");
             }
-            if (!ws.m_conflicts.count(ptxConflicting->GetHash()))
+            if (!ws.m_conflicts_incl_policy.count(ptxConflicting->GetHash()))
             {
                 // Transactions that don't explicitly signal replaceability are
                 // *not* replaceable with the current logic, even if one of their
@@ -878,16 +879,22 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
                     return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
                 }
 
-                ws.m_conflicts.insert(ptxConflicting->GetHash());
+                ws.m_conflicts_incl_policy.emplace(ptxConflicting->GetHash(), true);
             }
         }
     }
 
     if (spk_reuse_mode != SRM_ALLOW) {
         for (const CTxOut& txout : tx.vout) {
             uint160 hashSPK = ScriptHashkey(txout.scriptPubKey);
-            if (m_pool.mapUsedSPK.find(hashSPK) != m_pool.mapUsedSPK.end()) {
-                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-spk-reused");
+            const auto& SPKUsedIn = m_pool.mapUsedSPK.find(hashSPK);
+            if (SPKUsedIn != m_pool.mapUsedSPK.end()) {
+                if (SPKUsedIn->second.first) {
+                    ws.m_conflicts_incl_policy.emplace(SPKUsedIn->second.first->GetHash(), false);
+                }
+                if (SPKUsedIn->second.second) {
+                    ws.m_conflicts_incl_policy.emplace(SPKUsedIn->second.second->GetHash(), false);
+                }
             }
             if (mapSPK.find(hashSPK) != mapSPK.end()) {
                 return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-spk-reused-twinoutputs");
@@ -962,7 +969,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
             const auto& SPKit = m_pool.mapUsedSPK.find(hashSPK);
             if (SPKit != m_pool.mapUsedSPK.end()) {
                 if (SPKit->second.second /* Spent */) {
-                    return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-spk-reused-spend");
+                    ws.m_conflicts_incl_policy.emplace(SPKit->second.second->GetHash(), false);
                 }
             }
             mapSPK[hashSPK] = MemPool_SPK_State(mapSPK[hashSPK] | MSS_SPENT);
@@ -1024,14 +1031,18 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // feerate later.
     if (!args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state, args.m_ignore_rejects)) return false;
 
-    ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts);
+    std::set<Txid> conflicts_as_a_set;
+    std::transform(ws.m_conflicts_incl_policy.begin(), ws.m_conflicts_incl_policy.end(),
+                    std::inserter(conflicts_as_a_set, conflicts_as_a_set.end()),
+                    [](const std::pair<Txid, bool>& pair){ return pair.first; });
+    ws.m_iters_conflicting = m_pool.GetIterSet(conflicts_as_a_set);
 
     // Note that these modifications are only applicable to single transaction scenarios;
     // 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.
-    if (ws.m_conflicts.size() == 1 && args.m_allow_carveouts) {
+    if (ws.m_conflicts_incl_policy.size() == 1 && args.m_allow_carveouts) {
         // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we
         // would meet the chain limits after the conflicts have been removed. However, there isn't a practical
         // way to do this short of calculating the ancestor and descendant sets with an overlay cache of
@@ -1115,15 +1126,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // check using the full ancestor set here because it's more convenient to use what we have
     // already calculated.
     if (m_pool.m_opts.truc_policy == TRUCPolicy::Enforce) {
-    if (const auto err{SingleTRUCChecks(ws.m_ptx, "truc-", reason, ignore_rejects, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
+    if (const auto err{SingleTRUCChecks(ws.m_ptx, "truc-", reason, ignore_rejects, ws.m_ancestors, conflicts_as_a_set, ws.m_vsize)}) {
         // Single transaction contexts only.
         if (args.m_allow_sibling_eviction && err->second != nullptr) {
             // We should only be considering where replacement is considered valid as well.
             Assume(args.m_allow_replacement);
 
             // Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
             // included in RBF checks.
-            ws.m_conflicts.insert(err->second->GetHash());
+            ws.m_conflicts_incl_policy.emplace(err->second->GetHash(), false);
+            conflicts_as_a_set.insert(err->second->GetHash());
             // Adding the sibling to m_iters_conflicting here means that it doesn't count towards
             // RBF Carve Out above. This is correct, since removing to-be-replaced transactions from
             // the descendant count is done separately in SingleTRUCChecks for TRUC transactions.
@@ -1142,14 +1154,18 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // that we have the set of all ancestors we can detect this
     // pathological case by making sure ws.m_conflicts and ws.m_ancestors don't
     // intersect.
-    if (const auto err_string{EntriesAndTxidsDisjoint(ws.m_ancestors, ws.m_conflicts, hash)}) {
+    bool has_policy_conflict{false};
+    if (const auto err_string{EntriesAndTxidsDisjoint(ws.m_ancestors, ws.m_conflicts_incl_policy, hash, &has_policy_conflict)}) {
         // We classify this as a consensus error because a transaction depending on something it
         // conflicts with would be inconsistent.
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string);
     }
+    if (has_policy_conflict) {
+        return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-spk-reused-chained");
+    }
 
     // We want to detect conflicts in any tx in a package to trigger package RBF logic
-    m_subpackage.m_rbf |= !ws.m_conflicts.empty();
+    m_subpackage.m_rbf |= !ws.m_conflicts_incl_policy.empty();
     return true;
 }