From 4d4f5791acad52b100a2c1395f202fa8efd509fe Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 14:32:16 -0500 Subject: [PATCH 1/3] test: prove ProReg replacement conflicts with same-MN updates A ProRegTx that reuses a confirmed external collateral deletes the live MN mid-block. The same-block update for that proTxHash then fails BuildNewListFromBlock with bad-protx-hash and aborts CreateNewBlock. Assert the in-block hazard and that existsProviderTxConflict must reject either ordering in the mempool. --- src/test/evo_deterministicmns_tests.cpp | 104 ++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 6e9339bd0c7a..4efed6924a3d 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -1418,6 +1418,104 @@ void FuncTestMempoolDualProregtx(TestChainSetup& setup) BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_reg2))); } +// A ProRegTx that reuses a confirmed external collateral replaces the live MN at block +// connect time. An update (ProUpServ/ProUpReg/ProUpRev) for the replaced proTxHash is still +// mempool-valid against the pre-block tip list, so fee-ordered packaging can put the replacement +// ProRegTx before the update and make BuildNewListFromBlock return bad-protx-hash, aborting +// CreateNewBlock/getblocktemplate. The mempool must treat them as conflicts. +void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup) +{ + auto& chainman = *Assert(setup.m_node.chainman.get()); + auto& dmnman = *Assert(setup.m_node.dmnman); + auto tip_index = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }; + auto tip_height = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Height()); }; + auto sync_dmn_tip = [&] { dmnman.UpdatedBlockTip(tip_index()); }; + + const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey()); + int nHeight = tip_height(); + auto utxos = BuildSimpleUtxoMap(setup.m_coinbase_txns); + + CKey ownerKey; + CKey payoutKey; + CKey collateralKey; + CBLSSecretKey operatorKey; + ownerKey.MakeNewKey(true); + payoutKey.MakeNewKey(true); + collateralKey.MakeNewKey(true); + operatorKey.MakeNewKey(); + + auto scriptPayout = GetScriptForDestination(PKHash(payoutKey.GetPubKey())); + auto scriptCollateral = GetScriptForDestination(PKHash(collateralKey.GetPubKey())); + + // Fund and mine an external collateral, then register MN X against it. + auto tx_collateral = CreateSpendTx(chainman, utxos, scriptCollateral, dmn_types::Regular.collat_amount, setup.coinbaseKey); + auto block = std::make_shared(setup.CreateBlock({tx_collateral}, coinbase_pk, chainman.ActiveChainstate())); + BOOST_REQUIRE(chainman.ProcessNewBlock(block, true, nullptr)); + sync_dmn_tip(); + BOOST_CHECK_EQUAL(tip_height(), nHeight + 1); + + const auto collateralOutpoint = GetCollateralOutpoint(tx_collateral); + auto tx_reg = CreateProRegTxExternalCollateral(chainman, utxos, /*port=*/1, collateralOutpoint, scriptPayout, + ownerKey, operatorKey, collateralKey, setup.coinbaseKey); + const uint256 proTxHash = tx_reg.GetHash(); + block = std::make_shared(setup.CreateBlock({tx_reg}, coinbase_pk, chainman.ActiveChainstate())); + BOOST_REQUIRE(chainman.ProcessNewBlock(block, true, nullptr)); + sync_dmn_tip(); + BOOST_CHECK_EQUAL(tip_height(), nHeight + 2); + BOOST_REQUIRE(dmnman.GetListAtChainTip().HasMN(proTxHash)); + BOOST_REQUIRE(dmnman.GetListAtChainTip().GetMNByCollateral(collateralOutpoint) != nullptr); + BOOST_CHECK_EQUAL(dmnman.GetListAtChainTip().GetMNByCollateral(collateralOutpoint)->proTxHash.ToString(), + proTxHash.ToString()); + + // Replacement ProRegTx reusing the same external collateral with fresh keys/address. + CKey ownerKey2; + CKey payoutKey2; + CBLSSecretKey operatorKey2; + ownerKey2.MakeNewKey(true); + payoutKey2.MakeNewKey(true); + operatorKey2.MakeNewKey(); + auto scriptPayout2 = GetScriptForDestination(PKHash(payoutKey2.GetPubKey())); + 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. + auto tx_up_serv = CreateProUpServTx(chainman, utxos, proTxHash, operatorKey, /*port=*/2, CScript(), setup.coinbaseKey); + + // Prove the in-block hazard itself: replacement first, then update of the deleted proTxHash. + { + LOCK(cs_main); + CBlock hazard_block; + hazard_block.vtx.emplace_back(MakeTransactionRef(CMutableTransaction())); // dummy coinbase slot + hazard_block.vtx.emplace_back(MakeTransactionRef(tx_reg_replace)); + hazard_block.vtx.emplace_back(MakeTransactionRef(tx_up_serv)); + + BlockValidationState state; + CDeterministicMNList mn_list; + BOOST_CHECK(!chainman.ActiveChainstate().ChainHelper().special_tx->BuildNewListFromBlock( + hazard_block, tip_index(), chainman.ActiveChainstate().CoinsTip(), /*debugLogs=*/false, state, mn_list)); + BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-hash"); + } + + // Mempool acceptance must refuse the second of these once one is present. + CTxMemPool testPool{MemPoolOptionsForTest(setup.m_node)}; + TestMemPoolEntryHelper entry; + + { + LOCK2(cs_main, testPool.cs); + // Replacement already in mempool => update for the MN it replaces is a conflict. + testPool.addUnchecked(entry.FromTx(tx_reg_replace)); + BOOST_CHECK_EQUAL(testPool.size(), 1U); + BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_up_serv))); + testPool.removeRecursive(CTransaction(tx_reg_replace), MemPoolRemovalReason::MANUAL); + BOOST_CHECK_EQUAL(testPool.size(), 0U); + + // Update already in mempool => replacement ProRegTx reusing that MN's collateral conflicts. + testPool.addUnchecked(entry.FromTx(tx_up_serv)); + BOOST_CHECK_EQUAL(testPool.size(), 1U); + BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_reg_replace))); + } +} + void FuncVerifyDB(TestChainSetup& setup) { auto& chainman = *Assert(setup.m_node.chainman.get()); @@ -3144,6 +3242,12 @@ BOOST_AUTO_TEST_CASE(test_mempool_dual_proregtx_basic) FuncTestMempoolDualProregtx(setup); } +BOOST_AUTO_TEST_CASE(test_mempool_proreg_replacement_update_conflict) +{ + TestChainV19Setup setup; + FuncTestMempoolProRegReplacementUpdateConflict(setup); +} + //This one can be started only with legacy scheme, since inside undo block will switch it back to legacy resulting into an inconsistency BOOST_AUTO_TEST_CASE(verify_db_legacy) { From 9b3ca40aa7eb157b88d98546e05ef23c204f3380 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 14:33:24 -0500 Subject: [PATCH 2/3] fix: conflict ProReg collateral reuse with mempool MN updates existsProviderTxConflict now links a replacement ProRegTx that reuses a live external collateral to any in-mempool ProUpServ/ProUpReg/ProUpRev for the MN being replaced, so both cannot coexist and CreateNewBlock cannot package the bad-protx-hash ordering. removeProTxConflicts also drops those updates when such a replacement is mined. Consensus block acceptance is unchanged; only mempool packaging/eviction is tightened. --- src/txmempool.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 001d4de0500a..1e32d6b3adc0 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1027,6 +1027,24 @@ 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.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); + } + } + } } else { removeProTxCollateralConflicts(tx, COutPoint(tx_hash, proTx.collateralOutpoint.n)); } @@ -1450,6 +1468,14 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { // there is another tx that spends the collateral return true; } + // 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; + } + } } return false; } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { @@ -1470,6 +1496,12 @@ 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; + } + } } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { const auto opt_proTx = GetTxPayload(tx); if (!opt_proTx) { @@ -1484,6 +1516,10 @@ 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)) { + return true; + } // only allow one operator key change in the mempool if (dmn->pdmnState->pubKeyOperator != proTx.pubKeyOperator) { if (hasKeyChangeInMempool(proTx.proTxHash)) { @@ -1506,6 +1542,10 @@ 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)) { + return true; + } // only allow one operator key change in the mempool if (dmn->pdmnState->pubKeyOperator.Get() != CBLSPublicKey()) { if (hasKeyChangeInMempool(proTx.proTxHash)) { From 95341f5a5c38e9f92290c6c09c0f07f8b5057c7e Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 20:18:18 -0500 Subject: [PATCH 3/3] 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. --- src/test/evo_deterministicmns_tests.cpp | 8 +++ src/txmempool.cpp | 65 +++++++++++-------------- src/txmempool.h | 4 ++ 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 4efed6924a3d..e76b07e6059c 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -1513,6 +1513,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 connected{MakeTransactionRef(CMutableTransaction()), + MakeTransactionRef(tx_reg_replace)}; + testPool.removeForBlock(connected, tip_height() + 1); + BOOST_CHECK_EQUAL(testPool.size(), 0U); } } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 1e32d6b3adc0..a7754ea05052 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -937,39 +937,41 @@ 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) { // 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 = m_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); } } } @@ -1027,23 +1029,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. + // 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 = m_dmnman.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); - } - } + removeProTxReferences(dmn->proTxHash); } } else { removeProTxCollateralConflicts(tx, COutPoint(tx_hash, proTx.collateralOutpoint.n)); diff --git a/src/txmempool.h b/src/txmempool.h index c0dcd0f1a360..429a4d2a44ed 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -650,6 +650,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);