From 7c34dd8934c4b2f18a2df17fe52c52e7f34792c3 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 23 Aug 2026 12:01:12 -0500 Subject: [PATCH] fix(mempool): exempt a MN's own ProRegTx from collateral-reuse conflicts The collateral-reuse conflict checks added in dash#7489 compare only map presence, not identity. During a reorg, dmnman's tip list still holds the disconnected chain's list until SynchronousUpdatedBlockTip fires after ActivateBestChainStep, so a ProRegTx of a masternode that list contains resolves via GetMNByCollateral() to the masternode it itself created. That misfires in two places: - MaybeUpdateMempoolForReorg resubmits the disconnected ProRegTx. With an update for that MN pending in the mempool, it was rejected as protx-dup and dropped, even though registration and update are mineable together in that order. - ConnectTip on the new branch re-mines the same ProRegTx and removeForBlock evicted every pending update of the masternode, although they stay valid once the registration is connected. A ProRegTx never replaces the masternode it creates. Skip the replacement handling when the resolved MN's proTxHash equals the ProRegTx's own hash, in admission and in block eviction, and have the three update branches share a collateralReusedInMempool() helper that requires the pending ProRegTx to be a different transaction, mirroring the it->second != proTxHash form the neighbouring checks already use. Co-Authored-By: Claude Fable 5 Co-Authored-By: Claude Opus 5.5 (1M context) --- src/test/evo_deterministicmns_tests.cpp | 40 ++++++++++++++++++++++--- src/txmempool.cpp | 38 ++++++++++++++--------- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 9cc1f4bf6dac..80a93ecf25b0 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -1644,8 +1644,11 @@ void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup) auto tx_reg_replace = CreateProRegTxExternalCollateral(chainman, utxos, /*port=*/3, collateralOutpoint, scriptPayout2, ownerKey2, operatorKey2, collateralKey, setup.coinbaseKey); - // Same-block-style update for the MN that the replacement would delete. + // Same-block-style updates for the MN that the replacement would delete. auto tx_up_serv = CreateProUpServTx(chainman, utxos, proTxHash, operatorKey, /*port=*/2, CScript(), setup.coinbaseKey); + auto tx_up_reg = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey.GetPublicKey(), + ownerKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey); + auto tx_up_rev = CreateProUpRevTx(chainman, utxos, proTxHash, operatorKey, setup.coinbaseKey); // Prove the in-block hazard itself: replacement first, then update of the deleted proTxHash. { @@ -1688,15 +1691,44 @@ void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup) MakeTransactionRef(tx_reg_replace)}; testPool.removeForBlock(connected, tip_height() + 1); BOOST_CHECK_EQUAL(testPool.size(), 0U); + + // ProUpReg and ProUpRev for the replaced MN conflict with a pending replacement too. + testPool.addUnchecked(entry.FromTx(tx_reg_replace)); + BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_up_reg))); + BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_up_rev))); + testPool.removeRecursive(CTransaction(tx_reg_replace), MemPoolRemovalReason::MANUAL); + BOOST_CHECK_EQUAL(testPool.size(), 0U); + + // The MN's own ProRegTx is not a replacement. While a reorg is being processed the + // tip list can still contain the MN whose registration is being resubmitted; if + // either direction were treated as a conflict, the resubmitted registration (or its + // updates) would be dropped even though the pair is mineable together. + testPool.addUnchecked(entry.FromTx(tx_up_serv)); + BOOST_CHECK(!testPool.existsProviderTxConflict(CTransaction(tx_reg))); + testPool.removeRecursive(CTransaction(tx_up_serv), MemPoolRemovalReason::MANUAL); + + testPool.addUnchecked(entry.FromTx(tx_reg)); + BOOST_CHECK(!testPool.existsProviderTxConflict(CTransaction(tx_up_serv))); + BOOST_CHECK(!testPool.existsProviderTxConflict(CTransaction(tx_up_reg))); + BOOST_CHECK(!testPool.existsProviderTxConflict(CTransaction(tx_up_rev))); + testPool.removeRecursive(CTransaction(tx_reg), MemPoolRemovalReason::MANUAL); + BOOST_CHECK_EQUAL(testPool.size(), 0U); + + // Likewise, a block connecting the MN's own ProRegTx while the tip list still contains + // the MN (the new branch of a reorg re-mining it) must not evict the MN's updates. + testPool.addUnchecked(entry.FromTx(tx_up_serv)); + std::vector reconnected{MakeTransactionRef(CMutableTransaction()), + MakeTransactionRef(tx_reg)}; + testPool.removeForBlock(reconnected, tip_height() + 1); + BOOST_CHECK(testPool.exists(tx_up_serv.GetHash())); + testPool.removeRecursive(CTransaction(tx_up_serv), MemPoolRemovalReason::MANUAL); + BOOST_CHECK_EQUAL(testPool.size(), 0U); } // The replacement is not the only confirmed transaction that can leave updates of the MN // unmineable: so does a ProUpReg changing the operator key, and a spend of the collateral. - auto tx_up_reg = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey.GetPublicKey(), - ownerKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey); auto tx_up_reg_new_key = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey2.GetPublicKey(), ownerKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey); - auto tx_up_rev = CreateProUpRevTx(chainman, utxos, proTxHash, operatorKey, setup.coinbaseKey); CMutableTransaction tx_spend_other; tx_spend_other.vin.emplace_back(COutPoint(tx_collateral.GetHash(), collateralOutpoint.n + 1)); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 43109a39228e..7e9ec7281ace 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1247,7 +1247,10 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx) // 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 = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) { + // The MN's own ProRegTx replaces nothing: during a reorg the tip list still holds + // the disconnected chain's MN list when the new branch re-mines the registration. + if (auto dmn = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint); + dmn && dmn->proTxHash != tx_hash) { removeProTxReferences(dmn->proTxHash); } } else { @@ -1675,6 +1678,16 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { return mapProTxPubKeyIDs.count(CKeyID{id}) || mapProTxPlatformNodeIDs.count(id); }; + // A pending ProRegTx claiming a live MN's collateral replaces (deletes) that MN when + // mined, so an update targeting the MN could never be mined afterwards. The MN's own + // ProRegTx (same hash, e.g. resubmitted while a reorg is being processed) replaces + // nothing and is not a conflict. + auto collateralReusedInMempool = [&](const CDeterministicMN& dmn) EXCLUSIVE_LOCKS_REQUIRED(cs) { + AssertLockHeld(cs); + auto it = mapProTxCollaterals.find(dmn.collateralOutpoint); + return it != mapProTxCollaterals.end() && it->second != dmn.proTxHash; + }; + const uint256 tx_hash{tx.GetHash()}; if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { const auto opt_proTx = GetTxPayload(tx); @@ -1717,10 +1730,12 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { // A replacement ProRegTx deletes the live MN backed by this collateral. Any // in-mempool update that still targets that MN's proTxHash would then fail // BuildNewListFromBlock with bad-protx-hash if both were mined in one block. - if (auto dmn = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) { - if (mapProTxRefs.find(dmn->proTxHash) != mapProTxRefs.end()) { - return true; - } + // Exempt the MN's own registration: while a reorg is being processed the tip + // list can still contain the MN whose ProRegTx is being resubmitted, and + // re-registering the same MN replaces nothing. + if (auto dmn = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint); + dmn && dmn->proTxHash != tx_hash && mapProTxRefs.count(dmn->proTxHash)) { + return true; } } return false; @@ -1745,11 +1760,8 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { return true; } } - // Conflict with a replacement ProRegTx that reuses this MN's external collateral. - if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(opt_proTx->proTxHash)) { - if (mapProTxCollaterals.count(dmn->collateralOutpoint)) { - return true; - } + if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(opt_proTx->proTxHash); dmn && collateralReusedInMempool(*dmn)) { + return true; } } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { const auto opt_proTx = GetTxPayload(tx); @@ -1765,8 +1777,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString()); return true; // i.e. failed to find validated ProTx == conflict } - // Conflict with a replacement ProRegTx that reuses this MN's external collateral. - if (mapProTxCollaterals.count(dmn->collateralOutpoint)) { + if (collateralReusedInMempool(*dmn)) { return true; } // only allow one operator key change in the mempool @@ -1791,8 +1802,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString()); return true; // i.e. failed to find validated ProTx == conflict } - // Conflict with a replacement ProRegTx that reuses this MN's external collateral. - if (mapProTxCollaterals.count(dmn->collateralOutpoint)) { + if (collateralReusedInMempool(*dmn)) { return true; } // only allow one operator key change in the mempool