refactor: extract removeProTxReferences and cover replacement eviction

The earlier fix inlined a copy of removeProTxSpentCollateralConflicts' inner loop into removeProTxConflicts, minus the diagnostic log on the should-never-happen branch. Hoist that loop into a named CTxMemPool::removeProTxReferences helper and call it from both sites, so the two paths that drop TXs naming a vanished MN cannot drift apart.

Also add a removeForBlock assertion to the new test. existsProviderTxConflict only gates our own acceptance and cannot stop an attacker from mining the replacement ProRegTx themselves; the eviction hunk in removeProTxConflicts is what keeps the orphaned update from stalling our block assembly afterwards, and it previously had no coverage. Verified as a negative control: the new assertion fails (1 != 0) with that hunk disabled.

Author: PastaPastaPasta

src/test/evo_deterministicmns_tests.cpp

@@ -1391,6 +1391,14 @@ void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup)
         testPool.addUnchecked(entry.FromTx(tx_up_serv));
         BOOST_CHECK_EQUAL(testPool.size(), 1U);
         BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_reg_replace)));
+
+        // existsProviderTxConflict only gates our own acceptance; it cannot stop a miner from
+        // confirming the replacement. Once that block arrives, removeForBlock must evict the
+        // now-unmineable update, otherwise it lingers and stalls our own block assembly.
+        std::vector<CTransactionRef> connected{MakeTransactionRef(CMutableTransaction()),
+                                               MakeTransactionRef(tx_reg_replace)};
+        testPool.removeForBlock(connected, tip_height() + 1);
+        BOOST_CHECK_EQUAL(testPool.size(), 0U);
     }
 }
 

src/txmempool.cpp

@@ -965,41 +965,43 @@ void CTxMemPool::removeProTxCollateralConflicts(const CTransaction &tx, const CO
     }
 }
 
+void CTxMemPool::removeProTxReferences(const uint256& proTxHash)
+{
+    // Can't use equal_range here as every call to removeRecursive might invalidate iterators
+    AssertLockHeld(cs);
+    while (true) {
+        auto it = mapProTxRefs.find(proTxHash);
+        if (it == mapProTxRefs.end()) {
+            break;
+        }
+        auto conflictIt = mapTx.find(it->second);
+        if (conflictIt != mapTx.end()) {
+            removeRecursive(conflictIt->GetTx(), MemPoolRemovalReason::CONFLICT);
+        } else {
+            // Should not happen as we track referencing TXs in addUnchecked/removeUnchecked.
+            // But lets be on the safe side and not run into an endless loop...
+            LogPrint(BCLog::MEMPOOL, "%s: ERROR: found invalid TX ref in mapProTxRefs, proTxHash=%s, txHash=%s\n", __func__, proTxHash.ToString(), it->second.ToString());
+            mapProTxRefs.erase(it);
+        }
+    }
+}
+
 void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx)
 {
     auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire));
 
     // Remove TXs that refer to a MN for which the collateral was spent
-    auto removeSpentCollateralConflict = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) {
-        // Can't use equal_range here as every call to removeRecursive might invalidate iterators
-        AssertLockHeld(cs);
-        while (true) {
-            auto it = mapProTxRefs.find(proTxHash);
-            if (it == mapProTxRefs.end()) {
-                break;
-            }
-            auto conflictIt = mapTx.find(it->second);
-            if (conflictIt != mapTx.end()) {
-                removeRecursive(conflictIt->GetTx(), MemPoolRemovalReason::CONFLICT);
-            } else {
-                // Should not happen as we track referencing TXs in addUnchecked/removeUnchecked.
-                // But lets be on the safe side and not run into an endless loop...
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: found invalid TX ref in mapProTxRefs, proTxHash=%s, txHash=%s\n", __func__, proTxHash.ToString(), it->second.ToString());
-                mapProTxRefs.erase(it);
-            }
-        }
-    };
     auto mnList = dmnman->GetListAtChainTip();
     for (const auto& in : tx.vin) {
         auto collateralIt = mapProTxCollaterals.find(in.prevout);
         if (collateralIt != mapProTxCollaterals.end()) {
             // These are not yet mined ProRegTxs
-            removeSpentCollateralConflict(collateralIt->second);
+            removeProTxReferences(collateralIt->second);
         }
         auto dmn = mnList.GetMNByCollateral(in.prevout);
         if (dmn) {
             // These are updates referring to a mined ProRegTx
-            removeSpentCollateralConflict(dmn->proTxHash);
+            removeProTxReferences(dmn->proTxHash);
         }
     }
 }
@@ -1050,23 +1052,12 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
         }
         if (!proTx.collateralOutpoint.hash.IsNull()) {
             removeProTxCollateralConflicts(tx, proTx.collateralOutpoint);
-            // Replacement of a live MN via external-collateral reuse: drop any
-            // mempool updates that still target the proTxHash being replaced.
-            // removeProTxSpentCollateralConflicts only covers spends, not reuse.
-            if (auto dmn = m_dmnman.load(std::memory_order_acquire)->GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) {
-                // Can't use equal_range + erase loop: removeRecursive may invalidate iterators.
-                while (true) {
-                    auto it = mapProTxRefs.find(dmn->proTxHash);
-                    if (it == mapProTxRefs.end()) {
-                        break;
-                    }
-                    auto conflictIt = mapTx.find(it->second);
-                    if (conflictIt != mapTx.end()) {
-                        removeRecursive(conflictIt->GetTx(), MemPoolRemovalReason::CONFLICT);
-                    } else {
-                        mapProTxRefs.erase(it);
-                    }
-                }
+            // A ProRegTx reusing an external collateral replaces the MN that collateral
+            // currently backs, so that MN ceases to exist. Drop any mempool update that
+            // still targets its proTxHash; such an update can never be mined afterwards.
+            // removeProTxSpentCollateralConflicts only covers collateral *spends*, not reuse.
+            if (auto dmn = Assert(m_dmnman.load(std::memory_order_acquire))->GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) {
+                removeProTxReferences(dmn->proTxHash);
             }
         } else {
             removeProTxCollateralConflicts(tx, COutPoint(tx_hash, proTx.collateralOutpoint.n));

src/txmempool.h

@@ -655,6 +655,10 @@ class CTxMemPool
     void removeProTxPubKeyConflicts(const CTransaction &tx, const CBLSLazyPublicKey &pubKey) EXCLUSIVE_LOCKS_REQUIRED(cs);
     void removeProTxPlatformNodeIDConflicts(const CTransaction &tx, const uint160 &platformNodeID) EXCLUSIVE_LOCKS_REQUIRED(cs);
     void removeProTxCollateralConflicts(const CTransaction &tx, const COutPoint &collateralOutpoint) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    /** Remove every mempool TX that refers to proTxHash. Used when the MN it names ceases to exist
+     *  (collateral spent, or collateral reused by a replacement ProRegTx), since such TXs can never
+     *  be mined afterwards and would abort block assembly with "bad-protx-hash". */
+    void removeProTxReferences(const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs);
     void removeProTxSpentCollateralConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
     void removeProTxKeyChangedConflicts(const CTransaction &tx, const uint256& proTxHash, const uint256& newKeyHash) EXCLUSIVE_LOCKS_REQUIRED(cs);
     void removeProTxConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs);