feat!: implement Decentralized Masternode Shares DIP#7437
feat!: implement Decentralized Masternode Shares DIP#7437PastaPastaPasta wants to merge 14 commits into
Conversation
PastaPastaPasta
force-pushed
the
claude/masternode-shares-dip-c40ac2
branch
from
16421b7 to
a675299
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
✅ Review complete (commit a55b9f9) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (22)
WalkthroughThis change introduces v24 decentralized masternode shares with shared collateral scripts, share-aware registration and state, three provider transaction types, participant signatures, consensus and mempool rules, RPC workflows, proportional rewards, filtering and JSON support, and unit and functional tests. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WalletRPC
participant SharedTransaction
participant Validator
participant DeterministicMNState
WalletRPC->>SharedTransaction: prepare and sign shared transaction
SharedTransaction->>Validator: submit transaction
Validator->>DeterministicMNState: apply registration or update
DeterministicMNState-->>Validator: return updated shared state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 16421b7d78
ℹ️ 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".
| TRANSACTION_PROVIDER_DISSOLVE = 10, | ||
| TRANSACTION_PROVIDER_UPDATE_SHARE = 11, | ||
| TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR = 12, |
There was a problem hiding this comment.
After adding these transaction types, CMerkleBlock still gates bloom matching through the hard-coded allowedTxTypes set in src/merkleblock.cpp, and it only calls filter->IsRelevantAndUpdate when the type is present there. Because these three IDs are absent from that allowlist, BIP37/SPV clients will not receive matched ProDisTx/ProUpShareTx/ProUpSharedRegTx transactions even though the bloom and compact-filter code now extracts their proTxHash and share fields. Add the new types to the merkle-block allowlist as part of introducing them.
Useful? React with 👍 / 👎.
| if (dest == CTxDestination(PKHash(mnState.keyIDVoting))) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-proupshare-payee-reuse"); | ||
| } |
There was a problem hiding this comment.
In block validation this comparison uses mnState from dmnman.GetListForBlock(pindexPrev), but RebuildListFromBlock later applies ProUpSharedRegTx and ProUpShareTx from the same block without rerunning the payee-reuse rule against the evolving list. A block can therefore change the shared MN voting key to X and include a share update setting scriptReward to X; each transaction passes against the previous voting key, leaving a state that registration and ordinary ProUpShareTx validation are meant to reject. Validate share reward updates against the in-block/evolving state, or recheck the invariant after registrar updates.
Useful? React with 👍 / 👎.
| // A shared registrar update requires unanimity: one signature per share, in share order | ||
| if (opt_ptx->vchSigs.size() != mnState.shares.size()) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-proupsharedreg-sig-count"); |
There was a problem hiding this comment.
A shared registrar update can set keyIDVoting to an address already used by an existing share refund or reward script because this path checks signature count and operator uniqueness but never reapplies the share payee-reuse rule to the current share table. With all share owners signing such an update, the masternode reaches a state that could not be registered and that ProUpShareTx would normally reject for new reward scripts. Reject keyIDVoting values that match any current share scriptRefund or RewardScript().
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/messagesigner.h (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the recovery-header constraint.
The implementation also rejects compact headers outside
27..34; omitting this makes the stated non-malleability contract incomplete.Proposed documentation update
- /// Verify the hash signature and additionally require the exact 65-byte size and a low-S value, - /// making the signature bytes non-malleable by third parties. Returns true if successful. + /// Verify the hash signature and additionally require the exact 65-byte size, a canonical + /// recovery header (27..34), and a low-S value, making the signature bytes non-malleable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/messagesigner.h` around lines 36 - 38, Update the documentation for VerifyHashCanonical to explicitly state that compact recovery headers must be within the 27..34 range, alongside the existing 65-byte size and low-S requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/evo/core_write.cpp`:
- Around line 281-285: Update the serializers around IsShared() in
src/evo/core_write.cpp and the corresponding RPC help: emit ownerAddress only
for non-shared records, omit it entirely for shared records whose keyIDOwner is
null, and mark the field optional in the RPC documentation.
In `@src/policy/policy.cpp`:
- Around line 114-120: The ProRegTx policy exception is too broad because it
accepts any shared-collateral script. In the conditional using
`sharedcollateral::IsSharedCollateralScript`, decode the shared-collateral
payload and continue only when `IsShared()` is true, the output is internal
collateral, and `collateralOutpoint.n` equals the current output index;
otherwise apply normal policy checks.
In `@src/rpc/evo.cpp`:
- Around line 2609-2610: Make the `shared_combine` RPC available when wallets
are disabled by moving its wallet-independent registration and dissolution
combining paths out of `GetWalletEvoRPCCommands` into the globally registered
EVO RPC commands. Keep only the shared-registrar funding-input signing path
wallet-dependent, and update all related command registrations and help text so
wallet-free workflows can complete.
- Around line 1738-1740: Update the RPC result schema for SignAndSendSpecialTx
to document both outcomes of the submit parameter: identify the result as a
transaction ID when submit is true and signed transaction hex when submit is
false, using the appropriate conditional/result description supported by the RPC
schema.
---
Nitpick comments:
In `@src/messagesigner.h`:
- Around line 36-38: Update the documentation for VerifyHashCanonical to
explicitly state that compact recovery headers must be within the 27..34 range,
alongside the existing 65-byte size and low-S requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6308fa88-db54-493e-97d1-94e0cfdee437
📒 Files selected for processing (32)
doc/release-notes-7437.mdsrc/Makefile.amsrc/Makefile.test.includesrc/common/bloom.cppsrc/core_write.cppsrc/evo/assetlocktx.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/dmnstate.cppsrc/evo/dmnstate.hsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/providertx_util.cppsrc/evo/sharedcollateral.hsrc/evo/specialtx.cppsrc/evo/specialtx.hsrc/evo/specialtx_filter.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/masternode/payments.cppsrc/messagesigner.cppsrc/messagesigner.hsrc/policy/policy.cppsrc/primitives/transaction.hsrc/rpc/client.cppsrc/rpc/evo.cppsrc/rpc/rawtransaction.cppsrc/test/evo_sharedmn_tests.cppsrc/txmempool.cppsrc/validation.cpptest/functional/feature_masternode_shares.pytest/functional/test_runner.py
| if (IsShared()) { | ||
| obj.pushKV("shares", ShareListToJson(shares)); | ||
| obj.pushKV("earlyPeriodBlocks", static_cast<int64_t>(nEarlyPeriodBlocks)); | ||
| obj.pushKV("earlyPenalty", nEarlyPenalty); | ||
| } else if (nVersion >= ProTxVersion::MultiPayout) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Omit the nonexistent single owner from shared JSON.
Shared validation requires keyIDOwner to be null, but both serializers already emit it as a zero-hash Dash address. Emit ownerAddress only for non-shared records and mark it optional in the corresponding RPC help.
Also applies to: 366-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/evo/core_write.cpp` around lines 281 - 285, Update the serializers around
IsShared() in src/evo/core_write.cpp and the corresponding RPC help: emit
ownerAddress only for non-shared records, omit it entirely for shared records
whose keyIDOwner is null, and mark the field optional in the RPC documentation.
| // The shared-collateral template is intentionally nonstandard everywhere except as an | ||
| // output of a ProRegTx (consensus restricts it to the collateral slot of a valid | ||
| // shared registration; policy only needs to let well-formed registrations relay) | ||
| if (tx.IsSpecialTxVersion() && tx.nType == TRANSACTION_PROVIDER_REGISTER && | ||
| sharedcollateral::IsSharedCollateralScript(txout.scriptPubKey)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict the exception to the declared shared collateral output.
This currently allows any matching output in any ProRegTx. Before v24, a normal ProRegTx can therefore relay unrelated template outputs which become permanently frozen at activation.
Decode the payload and require IsShared(), internal collateral, and collateralOutpoint.n matching the current output index before continuing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/policy/policy.cpp` around lines 114 - 120, The ProRegTx policy exception
is too broad because it accepts any shared-collateral script. In the conditional
using `sharedcollateral::IsSharedCollateralScript`, decode the shared-collateral
payload and continue only when `IsShared()` is true, the output is internal
collateral, and `collateralOutpoint.n` equals the current output index;
otherwise apply normal policy checks.
| "returned consent hash via \"protx shared_sign\"; combine with \"protx shared_combine\", then have the\n" | ||
| "funding inputs signed (signrawtransactionwithwallet) and broadcast with sendrawtransaction.\n" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep shared_combine available in wallet-disabled configurations.
The globally registered preparation RPCs instruct callers to use shared_combine, but that command is only registered through GetWalletEvoRPCCommands. Nodes built without wallet support or started with -disablewallet therefore cannot complete the advertised wallet-free registration/dissolution workflows.
Split out the wallet-independent combining paths, requiring a wallet only for the shared-registrar input-signing path.
Also applies to: 2730-2732, 2932-2932, 2954-2955
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/evo.cpp` around lines 2609 - 2610, Make the `shared_combine` RPC
available when wallets are disabled by moving its wallet-independent
registration and dissolution combining paths out of `GetWalletEvoRPCCommands`
into the globally registered EVO RPC commands. Keep only the shared-registrar
funding-input signing path wallet-dependent, and update all related command
registrations and help text so wallet-free workflows can complete.
There was a problem hiding this comment.
Code Review
No blocking consensus issues at head a675299. The Decentralized Masternode Shares DIP implementation is coherent: v4 ProRegTx shared payload, template collateral covenant, three new special tx types (ProDisTx / ProUpShareTx / ProUpSharedRegTx), amount-weighted reward split, mempool conflict tracking, filter coverage, and RPC surface all line up with the DIP. Previously-fixed items (sig-count commit in the ProDisTx digest, deferred dissolution in the collateral-spend phase, and the removeProTxKeyChangedConflicts end() guard) are present. The only in-scope observations are two commit-hygiene suggestions: two of the tail commits fix consensus-safety / miner-liveness defects introduced earlier in the same PR and should be squashed so develop history does not carry the intermediate broken states.
Source: reviewers — opus (general, dash-core-commit-history; completed), gpt-5.5 (general, dash-core-commit-history; failed/unparseable). Verifier — opus.
🟡 2 suggestion(s)
The two findings are commit-history suggestions anchored to commits rather than changed lines; full details are included below.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:7f6f8834e9c>`:
- [SUGGESTION] <commit:7f6f8834e9c>:1: Squash the ProDisTx signing-digest fix into the commit that introduced CProDisTx
Commit 7f6f8834e9c ('evo: commit the signature count into the ProDisTx signing digest') patches a txid-malleability / consensus-safety defect in code introduced by 2eb1ec2f7ac ('evo: shared masternode consensus rules, state, and special transactions'). CProDisTx::MakeSignHash was added in 2eb1ec2f7ac and only briefly existed without committing the signature count before being corrected here. Because Dash merges without squashing, leaving these as separate commits permanently records an intermediate state on develop where a signed unanimous ProDisTx can be malleated to a byte-identical unilateral variant — the exact hazard the fix commit's own body identifies. Even though v24 gating keeps the defect inert on-chain, a permanent bisect trap on a consensus-critical signing digest is precisely what the atomic-commit hygiene guideline is meant to prevent. Please fixup 7f6f8834e9c into 2eb1ec2f7ac and fold the rationale from its message into the target commit's body.
In `<commit:2c78c1b1397>`:
- [SUGGESTION] <commit:2c78c1b1397>:1: Squash the deferred-dissolution ordering fix into the shared-masternode consensus commit
Commit 2c78c1b1397 ('evo: apply shared masternode dissolution in the collateral-spend phase') corrects a miner-liveness DoS in RebuildListFromBlock introduced by 2eb1ec2f7ac. The message itself frames it as a fix to code introduced earlier in the same PR: previously the removal ran mid-loop, so a ProDisTx ordered before a same-masternode ProUpShareTx / ProUpSharedRegTx aborted block-template production. Keeping the fix as a distinct commit permanently records a broken ordering in the shared-masternode dissolution path in develop's history — a bisect hazard on the consensus-critical list rebuild that any participant of a shared masternode could otherwise trigger. Please fixup 2c78c1b1397 into 2eb1ec2f7ac; the new functional case that mines a pending dissolution with a same-masternode update can either fold into 62e7eb7aafa or stay with the consensus commit.
…ypes Adds the 7-byte shared masternode collateral template script (0x04 "DSHC" OP_DROP OP_TRUE, hex 04445348437551) with exact-match helpers, and reserves special transaction types 10 (ProDisTx), 11 (ProUpShareTx) and 12 (ProUpSharedRegTx) for the decentralized masternode shares DIP. The new types are whitelisted in ContextualCheckTransaction unconditionally, matching the asset lock/unlock pattern: pre-activation rejection happens inside the per-type check functions, which gate on v24. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A version 4 ProRegTx with a non-zero share count is a shared masternode registration: 2 to 8 participants each record an immutable collateral amount, refund script and share owner key, plus an updatable reward script. The new fields (share table, one 65-byte consent signature per share, early-period length and penalty) are appended to the v4 payload after the DIP-0026 payouts field; a non-shared v4 payload serializes a zero share count and zeroed penalty fields. Version 4 is unreleased, so this wire change is safe. Stateless validation (IsShareListTriviallyValid) enforces the DIP rules: regular masternodes only, internal collateral, null owner key, empty payout list, per-share minimum of 100 DASH summing exactly to the collateral, penalty below the smallest share, early period of at most 420480 blocks, P2PKH/P2SH refund and reward scripts that are not the template and do not pay any table owner key or the voting key, no duplicate owner keys or refund scripts, and one join signature per share. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the consensus core of the decentralized masternode shares DIP. These concerns are combined into one commit because they interlock through the shared special-transaction machinery (the CheckSpecialTxInner dispatch, the special-tx JSON writers, and the deterministic-list rebuild), and do not build in isolation. - Shared registration: a version 4 ProRegTx with a non-empty share table is a shared registration. Stateless rules (share count 2..8, per-share minimum and exact collateral sum, penalty bounds, P2PKH/P2SH non-template refund/reward scripts, no duplicate owner keys or refund scripts, null keyIDOwner, empty DIP-0026 payouts) plus the SharedRegConsentHash digest that every share owner signs, binding the funding inputs, all outputs, the share table, the penalty terms and the registrar configuration. CheckProRegTx additionally requires the internal collateral to pay exactly the template script and enforces cross-list owner-key uniqueness. - Masternode state: CDeterministicMNState stores the share table (excluded from joinSigs), early period and penalty, with a single Field_shares diff plus the penalty-term diff bits; share owner keys occupy the keyIDOwner uniqueness namespace so reuse is rejected in both directions; the null owner key of a shared masternode is handled in AddMN/RemoveMN and the revive path. - ProDisTx (type 10): dissolves a shared masternode, refunding every share to its refund script. One signature (unilateral, penalised in the early period) or one per share (unanimous, penalty-free); the count is committed into the signed digest. Minimum-based output rules with a non-increasing required penalty make validity monotone. Removal happens in the collateral-spend phase of list construction, so an update and a dissolution of the same masternode are valid together in one block in either order. - ProUpShareTx (type 11) updates one share's reward script; ProUpSharedRegTx (type 12) updates the operator and voting keys with a signature from every share owner. A plain ProUpRegTx on a shared masternode is invalid. - Template covenant: consensus restricts creation of the 7-byte template output (only as the collateral of a valid shared registration, checked for every transaction including the coinbase) and its spending (only via a ProDisTx), hooked at mempool acceptance and block connection. Asset-unlock destinations are covered by the generic creation check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The owner reward of a shared masternode is split across the share table with DIP-0026's single rounding convention, using the recorded collateral amounts as weights: every entry except the last receives floor(ownerReward * amount / collateral) over a 128-bit intermediate and the last entry receives the remainder, so the split sums exactly. Each portion pays the share's reward script, falling back to its refund script when no reward script is set; zero-value outputs are omitted and operator reward handling is unchanged. The v24 multiplicity-correct coinbase matching already handles duplicate reward scripts across shares. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tions Shared registrations track each share owner key in mapProTxPubKeyIDs (the null keyIDOwner must never enter the map, or two shared registrations would silently collide on it and the wrong entry would be erased on removal), so pending shared and non-shared registrations conflict on owner keys in both directions. ProDisTx, ProUpShareTx and ProUpSharedRegTx register in mapProTxRefs, which makes the existing spent-collateral sweep evict pending updates for a masternode (and any competing dissolution) when its ProDisTx confirms. ProUpSharedRegTx reuses the ProUpRegTx operator-key-change bookkeeping: one pending key change per masternode, and eviction of transactions signed with a replaced key. Competing dissolutions conflict naturally on the collateral input; there is no replacement, first-seen wins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The template output is nonstandard by Solver, so two targeted exemptions are needed for well-formed shared-masternode transactions to relay: IsStandardTx permits a template output only inside a ProRegTx (consensus restricts it to the collateral slot of a valid shared registration), and AreInputsStandard permits a template prevout only inside a ProDisTx (which spends it with an empty scriptSig; 7 bytes of OP_DROP/OP_TRUE carry no script-evaluation DoS surface). Everywhere else the template remains nonstandard, which is also the required pre-activation posture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bloom filters and compact-filter element extraction (kept in lockstep, per the note in both files) now cover shared registrations (every refund script, reward script and share owner key), ProDisTx (proTxHash; the refund payments are literal outputs already matched by the base filter), ProUpShareTx (proTxHash and the new reward script) and ProUpSharedRegTx (proTxHash and voting key, matching ProUpRegTx). Share data stays out of the simplified masternode list entry hash, matching DIP-0026's treatment of payout data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the RPC surface for shared masternodes: - protx register_shared_prepare appends the template collateral output and a v4 shared payload to a caller-supplied funding transaction (which must already contain every participant's inputs and change outputs; the consent digest binds all of them) and returns the consent hash. - protx shared_sign signs a shared registration, dissolution or shared registrar update with every share owner key the wallet holds; protx shared_combine inserts the collected signatures and can submit dissolutions and registrar updates directly (a combined registration still needs its funding inputs signed, then sendrawtransaction). - protx dissolve builds, signs and submits a unilateral ProDisTx, paying the early-period penalty when required; with submit=false it returns hex suitable for offline standby storage (ProDisTx validity is monotone). protx dissolve_prepare builds the unanimous, penalty-free variant. - protx update_share updates one share's reward address with that share owner's key; protx update_shared_registrar_prepare builds an unsigned ProUpSharedRegTx with signed fee inputs (re-signed on combine, since inserting signatures changes the payload). protx update_registrar now rejects shared masternodes up front instead of dereferencing their (empty) payout list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
evo_sharedmn_tests covers the template script constant and its script-layer spendability, the share-list validation matrix, shared payload serialization round-trips, consent-digest field coverage, canonical (low-S) signature verification including a malleated high-S rejection, the amount-weighted reward split, state serialization and single-logical-field share diffs, bidirectional owner-key uniqueness (pinning the untagged unique-property hashing it relies on), and the full ProDisTx validation matrix including monotone validity across the early-period boundary. feature_masternode_shares exercises the feature end to end on regtest: registration via prepare/sign/combine, reward split to the duff via getblocktemplate, template covenant rejections at the mempool, reward script updates, unanimous registrar updates, plain ProUpRegTx rejection, penalized unilateral dissolution with exact refund outputs, standby dissolutions becoming valid across the early-period boundary, and penalty-free unanimous dissolution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Defer a ProDisTx's RemoveMN out of the provider-transaction loop in RebuildListFromBlock and into the existing collateral-spend sweep, so a shared masternode's removal happens in the same phase as every other collateral spend. Previously the removal ran mid-loop, so a ProDisTx ordered before a same-masternode ProUpShareTx / ProUpSharedRegTx in a block removed the masternode before the later update was processed, failing BuildNewListFromBlock and aborting block-template production (a miner-liveness DoS reachable by any participant of a shared masternode). With the removal deferred, the update always applies during the provider-transaction pass and the sweep removes the masternode afterward, so the block is valid regardless of ordering. The collateral-spend guard is folded into the sweep: a shared masternode's collateral spent by anything other than a ProDisTx is still rejected. A functional case mines a pending dissolution together with a same-masternode update and asserts the masternode dissolves without aborting the miner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tions protx register_shared_prepare now runs the payload through the same stateless validation consensus applies (IsTriviallyValid), so consensus-invalid terms — share sums, penalty bounds, payee reuse, script types — fail immediately with a clear error instead of after every participant has signed the consent hash and the funding inputs are finalized. The registration help text now points participants at the standby-dissolution workflow the DIP recommends: create "protx dissolve <proTxHash> <shareIndex> <fee> false" once the registration confirms and store the hex with the refund-key backup, so a lost owner key can never strand the principal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CProDisTx::MakeSignHash now also commits the signature count, which selects the dissolution mode (1 = unilateral, sharesCount = unanimous). Without it, a penalty-free unanimous dissolution (or an early-period unanimous one that overpays the penalty) has outputs byte-identical to a valid unilateral dissolution, so a third party could drop all signatures except the actor's and rebroadcast a variant with the same inputs and outputs but a different txid, invalidating a pre-signed refund spend or an actor-output CPFP child. Callers pass the count the transaction will carry (1 for protx dissolve, sharesCount for the dissolve_prepare/shared_sign unanimous flow); verification passes the actual vchSigs.size(), so a mutated count no longer verifies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… entries The eviction loop dereferenced mapTx.find(txHash) without checking for end(). If two stale provider updates for one masternode form a parent/child chain, removeRecursive() on the parent also evicts the child, so the child's saved hash then resolves to mapTx.end() and dereferencing it can crash the node. This is a pre-existing hazard in the conflict-eviction path (surfaced while reviewing the shared-masternode changes to this function); guard the lookup and skip entries already gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
claude/masternode-shares-dip-c40ac2
branch
from
a675299 to
a55b9f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a55b9f9091
ℹ️ 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".
| std::shared_ptr<NetInfoInterface> netInfo{nullptr}; | ||
| CScript scriptPayout; | ||
| MasternodePayoutShares payouts; | ||
| CollateralShares shares; // non-empty = shared masternode (joinSigs are not kept in state) |
There was a problem hiding this comment.
Adding shared masternode state here makes keyIDOwner null and leaves payouts empty for these entries, but the wallet filter in protx list wallet still only checks collateral, keyIDOwner, voting, legacy payouts, and operator payout (src/rpc/evo.cpp:2166-2170). A participant wallet that holds only a share owner key or refund/reward script will therefore not see its shared masternode in wallet-scoped listings or ownership metadata; please scan shares for owner/refund/reward ownership when IsShared() is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
Cumulative review at head a55b9f909120ac049010f26dcbd6cfb46b1371b1 finds no consensus or correctness blockers. The delta from a6752995 is a small wallet-gating cleanup in src/rpc/evo.cpp: it moves shared-masternode RPC helpers under ENABLE_WALLET, fixes two ParseBLSPubKey argument-name typos, and registers the two shared-prepare RPCs with the wallet commands. The complete current-head stack was also re-reviewed.
Source: reviewers — opus (general, dash-core-commit-history; completed), gpt-5.5 (general, dash-core-commit-history; failed/unparseable due revoked ACP credential). Verifier — opus.
Prior finding reconciliation
- STILL VALID: the prior ProDisTx signing-digest squash suggestion. The rebase rewrote
7f6f8834e9cto5495e02f04f, but it remains a separate corrective commit for consensus code introduced byc7a7a408bdb. - STILL VALID: the prior deferred-dissolution ordering squash suggestion. The rebase rewrote
2c78c1b1397to4ac7f9b6d57, but it remains a separate corrective commit for the same earlier consensus commit.
New findings in the latest delta
None.
Additional cumulative findings
None.
🟡 2 carried-forward suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:5495e02f04f>`:
- [SUGGESTION] <commit:5495e02f04f>:1: Squash the ProDisTx signing-digest fix into the shared-masternode consensus commit
Commit 5495e02f04f ('evo: commit the signature count into the ProDisTx signing digest') patches a txid-malleability / consensus-safety defect in CProDisTx::MakeSignHash — code introduced earlier in the same PR by c7a7a408bdb ('evo: shared masternode consensus rules, state, and special transactions'). Without the signature count in the digest, a signed unanimous ProDisTx has outputs byte-identical to a valid unilateral variant and can be malleated to a different txid, invalidating pre-signed refund spends or actor-output CPFP children. Because Dash merges without squashing, keeping this as a separate tail commit permanently records an intermediate state on develop where the consensus-critical signing digest is known-broken. Please fixup 5495e02f04f into c7a7a408bdb and fold the rationale from its message into the consensus commit's body; the test changes can move with it or into the tests commit 8911c80c103.
In `<commit:4ac7f9b6d57>`:
- [SUGGESTION] <commit:4ac7f9b6d57>:1: Squash the deferred-dissolution ordering fix into the shared-masternode consensus commit
Commit 4ac7f9b6d57 ('evo: apply shared masternode dissolution in the collateral-spend phase') corrects a miner-liveness DoS in RebuildListFromBlock introduced earlier in the same PR by c7a7a408bdb. Previously, a ProDisTx ordered before a same-masternode ProUpShareTx / ProUpSharedRegTx removed the masternode before the later update was processed, failing BuildNewListFromBlock and aborting block-template production. Leaving this as a separate tail commit permanently records the broken ordering on the consensus-critical list-rebuild path. Please fixup 4ac7f9b6d57 into c7a7a408bdb; the functional regression case can move into tests commit 8911c80c103 or stay with the consensus commit.
2 participants
Issue being fixed or feature implemented
Implements the Decentralized Masternode Shares DIP
(dashpay/dips#187), which extends
DIP-0003 and the DIP-0026 multi-party payouts introduced by #7340.
DIP-0026 lets the owner reward be split across multiple payout scripts, but its
Security Considerations note that it deliberately leaves the collateral UTXO
under a single key's control: "a share arrangement that requires immutable
payout rights must use additional contractual, wallet, or protocol mechanisms
outside the scope of this DIP." This PR is that protocol mechanism.
It lets 2–8 participants trustlessly co-own one masternode: they fund the
collateral atomically in a single registration, consensus splits the owner
reward across them in proportion to their recorded contributions, and the
collateral can leave the masternode only through a consensus-enforced
dissolution that refunds each participant's principal to a refund script fixed
at registration. No participant, operator, miner, or compromised update path
can redirect another participant's principal, and no participant can be
prevented from exiting.
There is no open issue; this PR tracks the DIP.
What was done?
All behaviour is gated behind
DEPLOYMENT_V24and deploys together withDIP-0026; it is inert until activation. It extends the unreleased ProTx
version 4 payload, so the two must ship in the same release.
sharestable turns a v4 registrationinto a shared registration. Each share records an immutable
amount,refundScript, andownerKeyIDplus an updatablerewardScript; the payloadalso carries one consent signature per share, the early-period length, and the
penalty. A non-shared v4 payload serialises a zero share count.
paying exactly the 7-byte script
04445348437551(
0x04 "DSHC" OP_DROP OP_TRUE). Consensus restricts its creation (only as thecollateral of a valid shared registration, checked for every transaction
including the coinbase) and its spending (only via a ProDisTx). Pre-activation
template outputs become permanently unspendable, following the reserved-pattern
precedent.
SharedRegConsentHashbinds the exact funding inputs, alloutputs, the share table, the penalty terms, and the registrar configuration;
every share owner signs it. Registration is atomic and co-signer txid
malleability is harmless.
its refund script. One signature (unilateral, penalised during the early
period) or one per share (unanimous, penalty-free); the count is committed
into the signed digest. Output rules are minimum-based and the required
penalty is non-increasing in height, so a signed dissolution's validity is
monotone — the basis for offline "standby dissolutions".
share owner.
signature from every share owner. A plain ProUpRegTx on a shared masternode
is invalid.
sequential-floor, remainder-to-last convention), paying each share's reward
script or its refund script.
CDeterministicMNState(excluded from the SML leaf hash, like DIP-0026 payoutdata); share owner keys share the
keyIDOwneruniqueness namespace so reuse isrejected in both directions; mempool conflict tracking and eviction cover the
new types; the template output/prevout get targeted relay carve-outs; and
bloom/GCS filters match share scripts and keys.
protx register_shared_prepare/shared_sign/shared_combinefor the multi-party registration and unanimous flows,
protx dissolve/dissolve_prepare(with standby-dissolution support),protx update_share,and
protx update_shared_registrar_prepare.The branch is organised as one reviewable commit per concept.
Deliberate deviations from the original DIP draft, all now reconciled in
dips#187:
operatorRewardis fixed at registration and dropped fromProUpSharedRegTx (matching DIP-0003 ProUpRegTx); the non-normative
replace-by-higher-fee relay suggestion is removed (Dash has no replacement);
the signature count is committed into the dissolution digest; a dissolution
takes effect in the collateral-spend phase of list construction (so an update
and a dissolution of the same masternode are valid together in one block in
either order); and lifecycle transactions are filter-matched by
proTxHash,like ProUpRegTx/ProUpRevTx, since they carry no share table.
How Has This Been Tested?
src/test/evo_sharedmn_tests.cpp, plus updatedevo_deterministicmns_tests,masternode_payments_tests,bloom_tests):template-script matching, the share-list validation matrix, v4 payload
round-trips, consent-digest field coverage, canonical (low-S and canonical
recovery-header) signature verification including malleation rejection, the
amount-weighted reward split, MN-state diffs, bidirectional owner-key
uniqueness, and the full ProDisTx validation matrix (penalty floors,
monotonicity across the early-period boundary, and the signature-count
malleability guard).
test/functional/feature_masternode_shares.py): end-to-endon regtest — multi-party registration via prepare/sign/combine, reward split
verified to the duff, template covenant rejections driven through block
connection (not just policy), reward-script and unanimous registrar updates,
penalised unilateral dissolution, standby dissolutions valid across the
early-period boundary, penalty-free unanimous dissolution, and an
update+dissolution coexisting in one block. Preflight rejection of invalid
registration terms.
feature_masternode_payout_shares,feature_dip3_deterministicmns, andfeature_asset_lockspass unchanged.The full
test_dashunit suite passes. Every commit builds individually.confirmed findings (a pre-activation consensus-split guard, two txid
malleability vectors, a miner-abort DoS, and a mempool crash guard) are fixed.
Testing environment: regtest and unit tests on macOS (clang).
Breaking Changes
This is a consensus change activated by the
DEPLOYMENT_V24EHF, deployedtogether with DIP-0026. Before activation there is no behaviour change: shared
registrations and the new special transactions are invalid, and template
outputs are nonstandard but not consensus-invalid. Because it changes the
layout of the unreleased v4 ProRegTx payload, it must ship in the same release
as DIP-0026 (#7340); any pre-release chain that already activated v24 on the
DIP-0026-only v4 format would need a reset (no such mainnet or testnet chain
exists — v24 is not yet active there).
Checklist:
doc/release-notes-7437.md)