fix: conflict ProRegTx collateral reuse with in-mempool masternode updates - #7489
fix: conflict ProRegTx collateral reuse with in-mempool masternode updates#7489PastaPastaPasta wants to merge 3 commits into
Conversation
e31b6f8 to
1ad32c4
Compare
PastaPastaPasta
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe mempool now centralizes removal of transactions that reference an invalid ProTx. External-collateral ProRegTx replacements remove updates targeting the displaced masternode. Conflict checks cover provider service, registrar, and revoke transactions in both conflict directions. A V19 deterministic masternode regression test verifies insertion-order handling, mempool list rebuilding, and stale-update eviction after the replacement is mined. Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This change prevents conflicting masternode registration and update transactions from coexisting or remaining stale after block connection. No actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Test
participant CTxMemPool
participant ProRegTx
participant ProviderUpdate
Test->>CTxMemPool: submit ProviderUpdate
Test->>CTxMemPool: submit ProRegTx replacement
CTxMemPool->>ProRegTx: check external collateral references
CTxMemPool->>ProviderUpdate: detect displaced ProTx reference
CTxMemPool-->>Test: reject conflict or remove stale update
Test->>CTxMemPool: mine replacement
CTxMemPool-->>Test: evict obsolete update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
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.
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.
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.
|
🔍 Review in progress — actively reviewing now (commit 3256c90) |
1ad32c4 to
3256c90
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ad32c44ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (auto dmn = dmnman->GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) { | ||
| if (mapProTxRefs.find(dmn->proTxHash) != mapProTxRefs.end()) { |
There was a problem hiding this comment.
When a regtest submitpackage contains the replacement ProRegTx as a parent and its update as the child, and a raised mempool minimum causes both to be evaluated using package feerates, AcceptMultipleTransactions() runs every PreChecks() before adding any package member to mapProTxCollaterals or mapProTxRefs. These checks therefore see only the pre-existing mempool, after which SubmitPackage() inserts both transactions without rechecking; because the dependency forces the replacement before the update, block assembly can still hit bad-protx-hash. Add a staged/package-wide provider-conflict check or recheck as each member is finalized.
AGENTS.md reference: AGENTS.md:L163-L163
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good structural observation — AcceptMultipleTransactions() does run every PreChecks() (including existsProviderTxConflict()) against the pre-package mempool before SubmitPackage() inserts anything, so intra-package provider conflicts are not seen. Two clarifications on scope:
-
This blind spot is pre-existing and generic to the whole provider-conflict layer, not introduced here. Every check in
existsProviderTxConflict()has it: a package containing two ProRegTxs claiming the same collateral, the same owner key, the same operator key, or the same address bypasses those checks identically today. The checks added in this PR inherit the same property; they don't widen it. -
The package path is not reachable outside regtest.
submitpackagehard-fails outside-regtest(src/rpc/mempool.cpp), and the only otherProcessNewPackage()caller istestmempoolaccept, which istest_accept-only and never submits. There is no p2p package relay in this codebase.
So the fix for this belongs in a package-wide provider-conflict pass (e.g. re-running existsProviderTxConflict() per member as each is finalized in SubmitPackage(), or a staged check that includes prior package members), covering all conflict classes at once — a separate change from this mempool-policy fix. Keeping it out of this PR; noting it as a known follow-up before package relay is ever exposed beyond regtest.
🤖 Posted autonomously by Claude on behalf of pasta.
|
@coderabbitai review\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta. |
|
✅ Action performedReview finished.
|
2 participants
Issue being fixed or feature implemented
A ProRegTx that reuses an external collateral replaces the masternode that collateral
currently backs — the old masternode ceases to exist and its
proTxHashbecomes unknown.The mempool does not model that. It tracks collateral spends
(
removeProTxSpentCollateralConflicts), but reuse is not a spend, so:masternode being replaced can sit in the mempool at the same time.
BuildNewListFromBlock()deletes the masternode whenit applies the ProRegTx, then fails
bad-protx-hashon the update.The same hazard exists when the replacement ProRegTx arrives in a block rather than through
the mempool: the stale update is left behind and can never be mined.
What was done?
Three commits.
1. Mempool admission (
existsProviderTxConflict) — the two directions are nowsymmetric:
references the incumbent masternode's
proTxHash.masternode's collateral outpoint.
This is first-in-wins with no RBF-style tiebreak. That is a deliberate policy choice: both
transactions are individually valid, and the only requirement is that they not coexist.
Preferring the incumbent would mean evicting an already-accepted transaction on arrival of a
new one, which is a bigger policy change than this problem warrants.
2. Eviction on block connect (
removeProTxConflicts) — when a collateral-reusingProRegTx is mined, any mempool transaction still targeting the replaced masternode is
removed. This covers the case where the ProRegTx never passed through this node's mempool,
so the admission check above never ran.
3. Refactor — the removal loop inside
removeProTxSpentCollateralConflictsis extractedas
CTxMemPool::removeProTxReferences()and reused by both call sites. No behaviour change;it just avoids a second copy of the iterator-invalidation-safe loop.
One thing reviewers should look at
The eviction in (2) calls
dmnman->GetListAtChainTip()and needs it to resolve to the listat the previous tip, so
GetMNByCollateral()returns the masternode being replaced ratherthan its replacement. That holds because
CDeterministicMNManager::tipIndexis assigned inUpdatedBlockTip(), which fires fromActivateBestChain()(validation.cpp:3339-3340) —after
ConnectTip()callsremoveForBlock()(validation.cpp:3025).removeProTxSpentCollateralConflicts()already depends on this ordering, so the new code isconsistent with the file. The difference is that there it is an optimisation, whereas here it
is load-bearing: if the ordering changed,
GetMNByCollateral()would return the replacementmasternode and the eviction would silently become a no-op. I have noted the dependency in a
comment, but flagging it explicitly since it is the least obvious part of the change.
How Has This Been Tested?
test_mempool_proreg_replacement_update_conflictinsrc/test/evo_deterministicmns_tests.cpp, added in the commit preceding the fix. Itcovers both paths:
existsProviderTxConflict()in both directions;removeForBlock()with a block containing the replacement ProRegTx.Without the fix the admission assertions fail (2 failures) — the conflicting pair is
accepted. With the fix,
evo_dip3_activation_testspasses (19 cases).Built and run on macOS/arm64 against current
develop.Breaking Changes
Mempool policy only; no consensus rules change. Some transaction pairs that were previously
accepted together are now mutually exclusive in the mempool. Neither could have been mined
together, so nothing that was previously minable is rejected.
Known follow-up
Coverage is C++ unit level only. A functional test exercising the miner path end to end
would strengthen this; happy to add one in this PR if reviewers would prefer it before merge.
Checklist: