Skip to content

fix: exempt a MN's own ProRegTx from collateral-reuse mempool conflicts #7638

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

Open
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+48 −13
Open

fix: exempt a MN's own ProRegTx from collateral-reuse mempool conflicts #7638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 28 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 @@ -1521,6 +1521,34 @@ 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.
CKey votingKey;
votingKey.MakeNewKey(true);
CBLSSecretKey operatorKey3;
operatorKey3.MakeNewKey();
auto tx_up_reg = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey3.GetPublicKey(),
votingKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey);
auto tx_up_rev = CreateProUpRevTx(chainman, utxos, proTxHash, operatorKey, setup.coinbaseKey);

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)));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

Expand Down
33 changes: 20 additions & 13 deletions src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,16 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
return false;
};

// 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<CProRegTx>(tx);
Expand Down Expand Up @@ -1462,10 +1472,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)) {
Comment on lines +1478 to +1479

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 23, 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.

P2 Badge Exempt same-key registrar updates from the self-registration check

When the pending update is a ProUpRegTx that changes only the voting key or payout script while retaining the operator key, the disconnected MN's own ProRegTx still never reaches this new self-exemption: the earlier mapProTxBlsPubKeyHashes.count(proTx.pubKeyOperator.GetHash()) check returns true, and existsProviderTxCrossSchemeConflict() also probes a ProRegTx without its own proTxHash. Consequently, this realistic reorg case is still rejected as protx-dup and the registration is dropped; the negative tests cover service/revoke updates but omit a same-key registrar update.

AGENTS.md reference: AGENTS.md:L193-L211

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Aug 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct observation, but it is a different, pre-existing check: mapProTxBlsPubKeyHashes.count() (and the cross-scheme probe) reject any ProRegTx whose operator key is already claimed in the mempool, and have done so since before #7489. This PR deliberately only fixes the collateral-reuse checks #7489 introduced. Exempting the key-duplication checks needs different semantics — their map values are the claiming transaction's txid, not the masternode's hash, so a correct exemption must establish that the entry belongs to an update of the very masternode the ProRegTx re-creates (and the cross-scheme probe needs the same treatment). Same low-severity, self-healing class as the case fixed here; happy to address it in a follow-up.

🤖 Posted autonomously by Claude on behalf of pasta.

Comment on lines +1478 to +1479

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 23, 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.

P2 Badge Exempt self-owned service properties before returning

When the pending transaction is a ProUpServTx that retains the MN's existing service address—for example, an update that only changes the operator payout—the incoming disconnected ProRegTx never reaches this hash-based exemption because mapProTxAddresses.count(entry) returns first; Evo updates similarly retain a platformNodeID and hit that earlier check. CheckProUpServTx explicitly permits properties already owned by the same proTxHash, so these pairs are mineable, but during the documented reorg sequence the registration is still rejected as protx-dup and dropped. The new test avoids both paths by changing the port and using a regular MN; the exemption must also identify same-MN service/platform claims.

AGENTS.md reference: AGENTS.md:L198-L211

Useful? React with 👍 / 👎.

return true;
}
}
return false;
Expand All @@ -1487,11 +1499,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<CProUpRegTx>(tx);
Expand All @@ -1507,8 +1516,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
Expand All @@ -1533,8 +1541,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
Expand Down
Loading