Skip to content

fix: conflict ProRegTx collateral reuse with in-mempool masternode updates #7489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 3 commits into from
Aug 23, 2026
Merged

fix: conflict ProRegTx collateral reuse with in-mempool masternode updates #7489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d4f579
test: prove ProReg replacement conflicts with same-MN updates
PastaPastaPasta Jul 25, 2026
9b3ca40
fix: conflict ProReg collateral reuse with mempool MN updates
PastaPastaPasta Jul 25, 2026
95341f5
refactor: extract removeProTxReferences and cover replacement eviction
PastaPastaPasta Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions src/test/evo_deterministicmns_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,112 @@ 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<CBlock>(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<CBlock>(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);
Comment on lines +1481 to +1482

@thepastaclaw thepastaclaw Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Cover the ProUpRegTx and ProUpRevTx admission branches

The test constructs only a ProUpServTx, so it exercises the service-update check at src/txmempool.cpp:1490-1495. The PR implements separate incoming-update checks for ProUpRegTx at lines 1510-1513 and ProUpRevTx at lines 1536-1539; the reverse direction also relies on their separate mapProTxRefs indexing branches in addUncheckedProTx(). Extend the test with registrar and revoke updates and verify both orderings for each type: a pending replacement rejects the update, and a pending update rejects the replacement ProRegTx.

source: ['codex']


// 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)));

// 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);
}
}

void FuncVerifyDB(TestChainSetup& setup)
{
auto& chainman = *Assert(setup.m_node.chainman.get());
Expand Down Expand Up @@ -3144,6 +3250,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)
{
Expand Down
73 changes: 52 additions & 21 deletions src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -1027,6 +1029,13 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
}
if (!proTx.collateralOutpoint.hash.IsNull()) {
removeProTxCollateralConflicts(tx, proTx.collateralOutpoint);
// 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)) {
removeProTxReferences(dmn->proTxHash);
}
} else {
removeProTxCollateralConflicts(tx, COutPoint(tx_hash, proTx.collateralOutpoint.n));
}
Expand Down Expand Up @@ -1450,6 +1459,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) {
Expand All @@ -1470,6 +1487,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<CProUpRegTx>(tx);
if (!opt_proTx) {
Expand All @@ -1484,6 +1507,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)) {
Expand All @@ -1506,6 +1533,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)) {
Expand Down
4 changes: 4 additions & 0 deletions src/txmempool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading