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