feat!: implement Decentralized Masternode Shares DIP - #7437
feat!: implement Decentralized Masternode Shares DIP#7437PastaPastaPasta wants to merge 20 commits into
Conversation
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 these PRs merge firstThis PR will likely need a rebase:
|
|
⛔ Blockers found — Sonnet deferred (commit e9e86aa) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change implements decentralized masternode shares for v24. It adds shared collateral registration, share-aware deterministic state, dissolution and update transactions, participant signatures, consensus and mempool enforcement, RPC workflows, proportional rewards, filtering, JSON serialization, and unit and functional tests. Shared collateral rules activate only after v24 activation. 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
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
| "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.
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.
|
This pull request has conflicts, please rebase. |
a55b9f9 to
bdc584d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc584d2c4
ℹ️ 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.
When a shared masternode has no operator payout script (for example, operatorReward is zero) and protx update_service is called without feeSourceAddress, the existing fallback in src/rpc/evo.cpp:1293-1294 calls GetOwnerPayouts(*dmn->pdmnState).front(). Shared states deliberately leave payouts empty and store payees in shares, so this dereferences an empty vector and can crash the node instead of creating the update. Make that fallback select a share reward/refund script or return a validation error requiring an explicit fee source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/rpc/evo.cpp (1)
1728-1731: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
submit=falseresult forprotx update_share.
SignAndSendSpecialTxreturns the signed transaction hex whensubmitis false, but the schema declares onlytxid. This repeats the result-schema concern raised in the previous review.Proposed RPC result schema
- RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction id"}, + { + RPCResult{"if \"submit\" is not set or set to true", + RPCResult::Type::STR_HEX, "txid", "The transaction id"}, + RPCResult{"if \"submit\" is set to false", + RPCResult::Type::STR_HEX, "hex", "The serialized signed ProUpShareTx in hex format"}, + },🤖 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 1728 - 1731, Update the RPCResult schema for the protx update_share handler to document both outcomes of the submit argument: the transaction ID when submitted and the signed transaction hex when submit is false. Keep the existing submit parameter and RPC example unchanged, and use the established result-schema conventions for representing alternative return values.
🧹 Nitpick comments (1)
src/evo/providertx.cpp (1)
428-446: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCommit the input and output counts, or reuse the existing hash helpers.
The writer concatenates all prevouts, then all sequences, then all outputs, with no element counts. The section boundaries are therefore implicit. Two transactions with different input counts can in principle produce the same byte string, because a shifted boundary can be absorbed by the following variable-length output data.
CProRegTx::MakeSharedRegConsentHashavoids this by delegating toCalcTxInputsHashandCalcTxOutputsHash. Use the same helpers here, or writetx.vin.size()andtx.vout.size()before each section. This also removes the hand-rolled loops.♻️ Proposed change
hw << tx.nLockTime; - for (const auto& in : tx.vin) { - hw << in.prevout; - } - for (const auto& in : tx.vin) { - hw << in.nSequence; - } - for (const auto& out : tx.vout) { - hw << out; - } + hw << static_cast<uint32_t>(tx.vin.size()); + for (const auto& in : tx.vin) { + hw << in.prevout; + } + for (const auto& in : tx.vin) { + hw << in.nSequence; + } + hw << static_cast<uint32_t>(tx.vout.size()); + for (const auto& out : tx.vout) { + hw << out; + } hw << proTxHash;🤖 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/providertx.cpp` around lines 428 - 446, Update the shared MN dissolve hash construction around the visible CHashWriter flow to reuse the existing CalcTxInputsHash and CalcTxOutputsHash helpers, matching CProRegTx::MakeSharedRegConsentHash, instead of manually serializing vin and vout in separate loops. Preserve the existing hash fields and ordering while replacing the hand-rolled input/output serialization with the helper results.
🤖 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/providertx.cpp`:
- Around line 260-289: Update CProRegTx::MakeSharedRegConsentHash to serialize
collateralOutpoint into the consent digest, preserving the existing field
ordering and hashing all of the outpoint including its payload index. Do not
alter the shared-registration validation or require a non-null hash; the index
must be bound while the existing null-hash requirement remains unchanged.
In `@src/evo/providertx.h`:
- Around line 188-199: Update the serialization write path around shares_count
to validate that obj.vchJoinSigs.size() equals obj.shares.size() before emitting
the count or signature data. Fail fast using the existing project convention for
serialization validation, while leaving the read-side resizing and iteration
behavior unchanged.
In `@src/rpc/evo.cpp`:
- Around line 2918-2924: Update the RPC command registration so
protx_shared_combine and protx_dissolve_prepare are available through the
non-wallet command set, not only GetWalletEvoRPCCommands. Preserve their
existing wallet registrations only if needed for wallet-enabled builds, and
ensure wallet-free nodes can complete the documented shared workflows.
- Around line 1796-1798: Update the help text describing ProUpSharedRegTx in the
surrounding RPC help definition to state that wallet fee inputs are added but
not signed, and that signing occurs later during protx shared_combine; preserve
the existing instructions for share-owner signing and combining.
- Around line 1924-1929: Resize opt_ptx->vchJoinSigs to at least
opt_ptx->shares.size() before the loop that processes sigs, while preserving the
existing share-index bounds validation and assignments.
---
Duplicate comments:
In `@src/rpc/evo.cpp`:
- Around line 1728-1731: Update the RPCResult schema for the protx update_share
handler to document both outcomes of the submit argument: the transaction ID
when submitted and the signed transaction hex when submit is false. Keep the
existing submit parameter and RPC example unchanged, and use the established
result-schema conventions for representing alternative return values.
---
Nitpick comments:
In `@src/evo/providertx.cpp`:
- Around line 428-446: Update the shared MN dissolve hash construction around
the visible CHashWriter flow to reuse the existing CalcTxInputsHash and
CalcTxOutputsHash helpers, matching CProRegTx::MakeSharedRegConsentHash, instead
of manually serializing vin and vout in separate loops. Preserve the existing
hash fields and ordering while replacing the hand-rolled input/output
serialization with the helper results.
🪄 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 Plus
Run ID: b6416075-1bd0-4da6-a462-1cc3ad062aa9
📒 Files selected for processing (28)
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.cpp
🚧 Files skipped from review as they are similar to previous changes (19)
- src/primitives/transaction.h
- src/rpc/rawtransaction.cpp
- src/evo/assetlocktx.cpp
- src/Makefile.am
- src/messagesigner.h
- src/evo/sharedcollateral.h
- src/masternode/payments.cpp
- src/Makefile.test.include
- src/evo/specialtx.h
- src/core_write.cpp
- src/rpc/client.cpp
- src/evo/dmnstate.cpp
- src/common/bloom.cpp
- src/policy/policy.cpp
- src/evo/specialtxman.h
- src/evo/core_write.cpp
- src/evo/deterministicmns.cpp
- src/evo/dmnstate.h
- src/evo/specialtxman.cpp
| uint8_t shares_count{0}; | ||
| SER_WRITE(obj, shares_count = static_cast<uint8_t>(obj.shares.size())); | ||
| READWRITE(shares_count); | ||
| SER_READ(obj, obj.shares.resize(shares_count)); | ||
| for (auto& share : obj.shares) { | ||
| READWRITE(share); | ||
| } | ||
| SER_READ(obj, obj.vchJoinSigs.resize(shares_count)); | ||
| for (auto& sig : obj.vchJoinSigs) { | ||
| READWRITE(Using<CompactSignatureFormatter>(sig)); | ||
| } | ||
| READWRITE(obj.nEarlyPeriodBlocks, obj.nEarlyPenalty); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard the write path against a vchJoinSigs/shares size mismatch.
On write, the code emits shares_count from shares.size(), then iterates obj.vchJoinSigs. If the two vectors have different sizes, the stream declares one count but carries a different number of signatures. The result is a payload that round-trips incorrectly, or an out-of-range read on the peer. Read validates nothing here either, because vchJoinSigs is resized from shares_count.
Fail fast on the write path instead of emitting an inconsistent payload.
🛡️ Proposed guard
uint8_t shares_count{0};
SER_WRITE(obj, shares_count = static_cast<uint8_t>(obj.shares.size()));
READWRITE(shares_count);
SER_READ(obj, obj.shares.resize(shares_count));
for (auto& share : obj.shares) {
READWRITE(share);
}
+ SER_WRITE(obj, [&] {
+ if (obj.vchJoinSigs.size() != obj.shares.size()) {
+ throw std::ios_base::failure("join signature count mismatch");
+ }
+ }());
SER_READ(obj, obj.vchJoinSigs.resize(shares_count));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint8_t shares_count{0}; | |
| SER_WRITE(obj, shares_count = static_cast<uint8_t>(obj.shares.size())); | |
| READWRITE(shares_count); | |
| SER_READ(obj, obj.shares.resize(shares_count)); | |
| for (auto& share : obj.shares) { | |
| READWRITE(share); | |
| } | |
| SER_READ(obj, obj.vchJoinSigs.resize(shares_count)); | |
| for (auto& sig : obj.vchJoinSigs) { | |
| READWRITE(Using<CompactSignatureFormatter>(sig)); | |
| } | |
| READWRITE(obj.nEarlyPeriodBlocks, obj.nEarlyPenalty); | |
| uint8_t shares_count{0}; | |
| SER_WRITE(obj, shares_count = static_cast<uint8_t>(obj.shares.size())); | |
| READWRITE(shares_count); | |
| SER_READ(obj, obj.shares.resize(shares_count)); | |
| for (auto& share : obj.shares) { | |
| READWRITE(share); | |
| } | |
| SER_WRITE(obj, [&] { | |
| if (obj.vchJoinSigs.size() != obj.shares.size()) { | |
| throw std::ios_base::failure("join signature count mismatch"); | |
| } | |
| }()); | |
| SER_READ(obj, obj.vchJoinSigs.resize(shares_count)); | |
| for (auto& sig : obj.vchJoinSigs) { | |
| READWRITE(Using<CompactSignatureFormatter>(sig)); | |
| } | |
| READWRITE(obj.nEarlyPeriodBlocks, obj.nEarlyPenalty); |
🤖 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/providertx.h` around lines 188 - 199, Update the serialization write
path around shares_count to validate that obj.vchJoinSigs.size() equals
obj.shares.size() before emitting the count or signature data. Fail fast using
the existing project convention for serialization validation, while leaving the
read-side resizing and iteration behavior unchanged.
| {"evo", &protx_register_shared_prepare}, | ||
| {"evo", &protx_shared_sign}, | ||
| {"evo", &protx_shared_combine}, | ||
| {"evo", &protx_dissolve}, | ||
| {"evo", &protx_dissolve_prepare}, | ||
| {"evo", &protx_update_share}, | ||
| {"evo", &protx_update_shared_registrar_prepare}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep the wallet-free shared commands available without a wallet.
protx shared_combine and protx dissolve_prepare need no wallet. Both appear only in GetWalletEvoRPCCommands, so nodes built without wallet support cannot complete the documented shared workflows. This repeats the concern raised in the previous review.
🤖 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 2918 - 2924, Update the RPC command
registration so protx_shared_combine and protx_dissolve_prepare are available
through the non-wallet command set, not only GetWalletEvoRPCCommands. Preserve
their existing wallet registrations only if needed for wallet-enabled builds,
and ensure wallet-free nodes can complete the documented shared workflows.
bdc584d to
fe3d5c0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3d5c016c
ℹ️ 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 (dmnPayee->pdmnState->IsShared()) { | ||
| // Shared masternodes split the owner reward by recorded collateral contribution, paying | ||
| // each share's reward script (or its refund script when no reward script is set) | ||
| const auto& shares = dmnPayee->pdmnState->shares; |
There was a problem hiding this comment.
When the selected payee is shared, rewards now go to shares[i].RewardScript(), but the existing reporting paths still enumerate only GetOwnerPayouts(state), which is empty for shared states. Consequently, masternode winners omits the actual payees and masternode list payee reports UNKNOWN for every shared masternode (src/rpc/masternode.cpp:232 and :268-279), also breaking address-based filtering of those results. Update these consumers to enumerate the share reward scripts when IsShared() is true.
Useful? React with 👍 / 👎.
…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>
An extended-address (version 3) 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 extended payload after the DIP-0026 payouts field; a non-shared extended payload serializes a zero share count and zeroed penalty fields. The extended version 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>
fe3d5c0 to
7773601
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7773601dd5
ℹ️ 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 (!IsValidDestination(rewardDest)) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("invalid reward address: %s", request.params[2].get_str())); | ||
| } | ||
| ptx.scriptReward = GetScriptForDestination(rewardDest); |
There was a problem hiding this comment.
When rewardAddress is a valid destination type other than P2PKH/P2SH, such as a witness address, this check accepts it and the RPC proceeds to fund and sign the transaction, but CProUpShareTx::IsTriviallyValid later rejects the generated script via IsValidPayoutScript. Consequently, protx update_share fails after transaction construction for an address the RPC explicitly accepted; validate the destination against the same P2PKH/P2SH restriction before calling FundSpecialTx.
Useful? React with 👍 / 👎.
| 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.
Once this field makes a state shared, its legacy payouts vector is empty, but CDeterministicMN::to_sml_entry() still copies only GetOwnerPayouts(state) and BuildSimplifiedDiff(..., extended=true) compares only scriptPayout, payouts, and the operator payout. Thus protx diff ... true never exposes a shared masternode's actual reward scripts, and a range containing only a ProUpShareTx can omit that masternode from the diff entirely because none of the compared fields changed. Extend the simplified extended entry, comparison, and JSON output to carry the share reward data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/evo/providertx_util.cpp (1)
104-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a guard for a zero weight total.
base_uint::operator/=throwsuint_erroron a zero divisor. Ifsharesis non-empty and everyamountis zero, line 119 divides by zero and throws inside the payment path. Consensus validation of the share list should prevent this state, so this is defensive only.♻️ Optional guard
std::vector<CAmount> ret; ret.reserve(shares.size()); + if (weight_total <= 0) return std::vector<CAmount>(shares.size(), CAmount{0}); CAmount paid{0};🤖 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/providertx_util.cpp` around lines 104 - 121, Guard the payout calculation in the share-processing loop against a zero weight_total before the arith_uint256 division, preserving the existing behavior for valid nonzero totals and the final-share remainder calculation.src/evo/specialtxman.cpp (1)
583-587: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
check_sigsis hardcoded during list rebuild.
ProcessSpecialTxsInBlockpassesfCheckCbTxMerkleRootsascheck_sigsintoCheckSpecialTxInner(line 821-822), so signature verification is skipped on paths where the caller opts out. This call always verifies dissolution signatures. Each dissolution then costs one to eight ECDSA public-key recoveries per rebuild, including reindex and diff reconstruction, and repeats work already done byCheckProDisTx.Consider threading the caller's
check_sigsvalue intoRebuildListFromBlockand passing it here.🤖 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/specialtxman.cpp` around lines 583 - 587, The list-rebuild path hardcodes signature verification for dissolution transactions, duplicating work when the caller disables checks. Thread the caller’s check_sigs value through RebuildListFromBlock and its callers, then pass that value to CheckProDisTxForList instead of true; preserve existing validation behavior when signature checking is enabled.test/functional/feature_masternode_shares.py (1)
165-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-sign the transaction after you append the extra template output.
Line 172 reuses
good_hex, which the wallet already signed. Line 173 appends an output, so the existing input signatures no longer commit to the output set. The assertion on line 176 then depends on mempool checks running standardness before script verification. That ordering holds today, but the test would reportscriptpubkeyfor the wrong reason if the check order changes.Build the extra-output transaction from the unsigned combined hex and sign it again, so the only defect left is the extra template output.
♻️ Proposed change to isolate the policy rejection
- extra_out_tx = tx_from_hex(good_hex) + combined = node.protx("shared_combine", sigtest_prepared["tx"], sigtest_sigs) + extra_out_tx = tx_from_hex(combined) extra_out_tx.vout.append(CTxOut(1000, CScript(bytes.fromhex(SHARED_COLLATERAL_SCRIPT)))) - res = node.testmempoolaccept([extra_out_tx.serialize().hex()])[0] + extra_signed = node.signrawtransactionwithwallet(extra_out_tx.serialize().hex()) + assert_equal(extra_signed["complete"], True) + res = node.testmempoolaccept([extra_signed["hex"]])[0] assert_equal(res["allowed"], False) assert_equal(res["reject-reason"], "scriptpubkey")🤖 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 `@test/functional/feature_masternode_shares.py` around lines 165 - 176, Update the extra-output test setup to start from the unsigned combined transaction hex rather than the already signed good_hex, append the additional CTxOut, and re-sign the modified transaction before calling testmempoolaccept. Keep the assertions unchanged so rejection is isolated to the extra template output.
🤖 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/specialtxman.cpp`:
- Around line 1528-1541: Update the reward-script validation in the ProUpShareTx
path around opt_ptx->scriptReward to enforce the same P2PKH-or-P2SH destination
restriction as IsShareListTriviallyValid, rather than accepting every
destination supported by ExtractDestination. Preserve the existing payee-reuse
checks after applying this registration-equivalent validation.
- Around line 1577-1592: Update CheckProUpSharedRegTx to reject
opt_ptx->keyIDVoting when it matches any existing mnState.shares keyIDOwner,
preserving the existing duplicate-property validation and returning the
appropriate invalid transaction result for this conflict.
---
Nitpick comments:
In `@src/evo/providertx_util.cpp`:
- Around line 104-121: Guard the payout calculation in the share-processing loop
against a zero weight_total before the arith_uint256 division, preserving the
existing behavior for valid nonzero totals and the final-share remainder
calculation.
In `@src/evo/specialtxman.cpp`:
- Around line 583-587: The list-rebuild path hardcodes signature verification
for dissolution transactions, duplicating work when the caller disables checks.
Thread the caller’s check_sigs value through RebuildListFromBlock and its
callers, then pass that value to CheckProDisTxForList instead of true; preserve
existing validation behavior when signature checking is enabled.
In `@test/functional/feature_masternode_shares.py`:
- Around line 165-176: Update the extra-output test setup to start from the
unsigned combined transaction hex rather than the already signed good_hex,
append the additional CTxOut, and re-sign the modified transaction before
calling testmempoolaccept. Keep the assertions unchanged so rejection is
isolated to the extra template output.
🪄 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 Plus
Run ID: 92bec2de-f47d-46db-807a-5da439b586ab
📒 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
🚧 Files skipped from review as they are similar to previous changes (24)
- src/rpc/rawtransaction.cpp
- src/evo/specialtx.h
- src/evo/dmnstate.cpp
- src/Makefile.am
- src/Makefile.test.include
- src/rpc/client.cpp
- src/messagesigner.h
- src/masternode/payments.cpp
- src/evo/assetlocktx.cpp
- src/core_write.cpp
- src/evo/sharedcollateral.h
- src/primitives/transaction.h
- doc/release-notes-7437.md
- src/common/bloom.cpp
- src/evo/deterministicmns.cpp
- src/policy/policy.cpp
- src/evo/specialtxman.h
- src/evo/dmnstate.h
- src/evo/core_write.cpp
- src/validation.cpp
- test/functional/test_runner.py
- src/evo/providertx.h
- src/txmempool.cpp
- src/rpc/evo.cpp
| const auto& mnState = *dmn->pdmnState; | ||
| if (!mnState.IsShared()) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-proupsharedreg-not-shared"); | ||
| } | ||
|
|
||
| // 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"); | ||
| } | ||
|
|
||
| if (mnList.HasUniqueProperty(opt_ptx->pubKeyOperator)) { | ||
| auto otherDmn = mnList.GetUniquePropertyMN(opt_ptx->pubKeyOperator); | ||
| if (opt_ptx->proTxHash != otherDmn->proTxHash) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the registration-time key-reuse rules for shares.
rg -n -C12 'IsShareListTriviallyValid' src/evo/providertx.cppRepository: dashpay/dash
Length of output: 2795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant ProUpShareTx validators.
rg -n -C8 'CheckProUpShareTx|bad-proupsharedreg-not-shared|bad-protx-payee-reuse|keyIDVoting|share.keyIDOwner' src/evo src/consensus -g '*.cpp' -g '*.h'
printf '\n--- specialtxman relevant slice ---\n'
sed -n '1500,1625p' src/evo/specialtxman.cpp
printf '\n--- providertx share validation slice ---\n'
sed -n '1,260p' src/evo/providertx.cppRepository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- providertx.cpp share validation continuation ---'
sed -n '64,150p' src/evo/providertx.cpp
printf '%s\n' '--- specialtxman.cpp CheckProUpSharedRegTx ---'
sed -n '1520,1615p' src/evo/specialtxman.cpp
printf '%s\n' '--- CProUpSharedRegTx definition ---'
sed -n '490,530p' src/evo/providertx.hRepository: dashpay/dash
Length of output: 10368
Check the new keyIDVoting against existing share keys.
IsShareListTriviallyValid rejects share refund/reward scripts that target keyIDVoting or any share.keyIDOwner, but CheckProUpSharedRegTx only validates opt_ptx->keyIDVoting for duplicate unique properties. Add the reverse check so a shared registrar update cannot set keyIDVoting to an existing share owner key.
🤖 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/specialtxman.cpp` around lines 1577 - 1592, Update
CheckProUpSharedRegTx to reject opt_ptx->keyIDVoting when it matches any
existing mnState.shares keyIDOwner, preserving the existing duplicate-property
validation and returning the appropriate invalid transaction result for this
conflict.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5ac81d12
ℹ️ 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.
For shared states, payouts is empty and keyIDOwner is null, but the unchanged GetOwnerPayoutScripts()/MnEntryImpl path in src/node/interfaces.cpp:105-140 still exports only GetOwnerPayouts(state) and that singular owner key. The Qt ownership scan in src/qt/masternodelist.cpp:221-235 therefore cannot recognize a participant wallet by any share owner, refund, or reward script, while MasternodeEntry displays UNKNOWN for its payout. Expose the share keys and effective reward/refund scripts through this interface and use them for Qt ownership and display.
AGENTS.md reference: AGENTS.md:L159-L164
Useful? React with 👍 / 👎.
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>
…ternode A ProDisTx whose masternode is not in the previous block's list is now invalid in blocks exactly as it always was in the mempool. The mempool could never hold a dissolution for an unconfirmed registration anyway (the masternode is not in the list yet), so no miner could assemble the pair from normal operation; only a hand-crafted block could, and a standby dissolution simply becomes valid one block after registration. Everything a dissolution is validated against (refund scripts, amounts, registration height) is immutable after registration, so the pindexPrev-based check in CheckProDisTx is complete and blocks need no re-validation against the evolving list. This deletes the SpecialTxContext plumbing and the RebuildListFromBlock re-check, and dissolution signatures are now verified exactly once per block, gated by the same check_sigs flag as every other provider transaction (they were previously verified twice for fresh blocks, and unconditionally during sync where other types skip them). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bd5ac81 to
0c15a43
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/evo_sharedmn_tests.cpp (4)
600-616: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
owner_keys[ptx.actorIndex]against an out-of-range index.In the non-unanimous branch,
Signindexesowner_keyswithptx.actorIndex.owner_keysholds 3 entries. The current cases never callSignwithunanimous=falseand an out-of-rangeactorIndex, so there is no present defect. The out-of-range actor case at Line 695 usesunanimous=true. If a later case combines an out-of-rangeactorIndexwithunanimous=false, the test reads out of bounds and the failure mode is undefined behavior instead of a clear assertion.🛡️ Proposed guard
} else { + BOOST_REQUIRE_LT(ptx.actorIndex, std::size(owner_keys)); std::vector<unsigned char> sig; BOOST_REQUIRE(CHashSigner::SignHash(hash, owner_keys[ptx.actorIndex], sig)); ptx.vchSigs.push_back(sig); }🤖 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/test/evo_sharedmn_tests.cpp` around lines 600 - 616, In the non-unanimous branch of Sign, validate that ptx.actorIndex is within owner_keys before indexing it, using a clear test assertion that fails for out-of-range values. Preserve the existing unanimous signing loop and valid actor-index signing behavior.
503-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the collision that the comment describes.
The comment states that a normal registration reusing a share owner key must be detected. The test only queries
HasUniquePropertyfor the share owner keys. It never adds a second, non-shared masternode whosekeyIDOwnerequalsowner_keys[0]and asserts thatAddMNrejects it. The stated invariant stays unproven.Add a case that builds a non-shared
CDeterministicMNStatewithkeyIDOwner == owner_keys[0].GetPubKey().GetID()and assertslist.AddMNthrows or fails, matching how the existing deterministic-MN tests assert duplicate-key rejection.As per coding guidelines: "For Dash-specific review hotspots, prefer small tests that prove the invariant being changed."
🤖 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/test/evo_sharedmn_tests.cpp` around lines 503 - 513, Add a focused collision test near the existing shared-owner uniqueness assertions: construct a non-shared CDeterministicMNState whose keyIDOwner equals owner_keys[0].GetPubKey().GetID(), then call list.AddMN and assert it throws or otherwise fails using the established duplicate-key rejection pattern in these deterministic-MN tests. Keep the existing HasUniqueProperty and cleanup assertions unchanged.Source: Coding guidelines
361-388: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
netInfoandpubKeyOperatorin the consent digest test.
MakeSharedRegConsentHashalso commitsnType,nMode,netInfoandpubKeyOperator. The test does not mutate those fields.netInfois serialized through a version-conditionalNetInfoSerWrapper, andpubKeyOperatorthroughCBLSLazyPublicKeyVersionWrapper. Both are the entries most likely to break in a later refactor. If either commitment is dropped, a third party can rewrite the operator key or the service address of a fully consented registration, and this test still passes.Add mutation cases for the operator public key and the network address, plus
nTypeandnMode, in the same style as the existing blocks.As per coding guidelines: "Exercise extra care around consensus and script flags, transaction payload serialization, masternodes, LLMQs, ... BLS transitions".
🤖 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/test/evo_sharedmn_tests.cpp` around lines 361 - 388, Extend the “Every covered field changes the digest” cases around CProRegTx::MakeSharedRegConsentHash to mutate and verify nType, nMode, netInfo, and pubKeyOperator individually. Use valid alternate values for the network address and operator public key, preserving the existing pattern that each mutated registration produces a hash different from base and exercises their versioned serialization wrappers.Source: Coding guidelines
198-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
scriptRefundreuse case to match the comment.The comment states that refund and reward scripts must not pay a table owner key or the voting key. The block tests only
scriptReward. The refund side of the invariant stays untested, so a regression that drops the refund check would pass.♻️ Proposed additional assertions
{ CollateralShares shares{two_shares}; shares[0].scriptReward = GetScriptForDestination(PKHash(shares[1].keyIDOwner)); CheckShares(shares, DummyJoinSigs(2), 0, 0, voting_id, "bad-protx-shares-payee-reuse"); shares[0].scriptReward = GetScriptForDestination(PKHash(voting_key.GetPubKey())); CheckShares(shares, DummyJoinSigs(2), 0, 0, voting_id, "bad-protx-shares-payee-reuse"); } + { + CollateralShares shares{two_shares}; + shares[0].scriptRefund = GetScriptForDestination(PKHash(shares[1].keyIDOwner)); + CheckShares(shares, DummyJoinSigs(2), 0, 0, voting_id, "bad-protx-shares-payee-reuse"); + } + { + CollateralShares shares{two_shares}; + shares[0].scriptRefund = GetScriptForDestination(PKHash(voting_key.GetPubKey())); + CheckShares(shares, DummyJoinSigs(2), 0, 0, voting_id, "bad-protx-shares-payee-reuse"); + }Note: the second case replaces
shares[0].scriptRefund, so it no longer duplicatesshares[1].scriptRefund. Confirm the expected reject reason if the implementation orders the duplicate-refund check first.As per coding guidelines: "For Dash-specific review hotspots, prefer small tests that prove the invariant being changed."
🤖 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/test/evo_sharedmn_tests.cpp` around lines 198 - 205, Add corresponding scriptRefund reuse cases in the existing CollateralShares block, assigning shares[0].scriptRefund to shares[1].keyIDOwner and then to voting_key.GetPubKey(), and assert both are rejected with the appropriate reason. Ensure the second setup does not retain or trigger an unintended duplicate-refund condition; use the expected reject reason for the validation order.Source: Coding guidelines
🤖 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/specialtxman.cpp`:
- Around line 604-622: The shared registrar path around SetStateVersion must
preserve higher CProUpSharedRegTx payload versions. Update the version
assignment for newState so it uses the maximum of old_version and
opt_proTx->nVersion, matching the ProUpRegTx path, while retaining the existing
state-version validation and return behavior.
- Around line 781-792: Update ProcessSpecialTxsInBlock’s per-transaction v24
validation to also invoke the shared-collateral spends check used by
ConnectBlock and mempool validation, not only
CheckSharedCollateralTemplateOutputs. Reject the block through the existing
TxValidationState and block-invalid return path when either check fails, while
preserving validation for all transactions including coinbase and non-special
transactions.
---
Nitpick comments:
In `@src/test/evo_sharedmn_tests.cpp`:
- Around line 600-616: In the non-unanimous branch of Sign, validate that
ptx.actorIndex is within owner_keys before indexing it, using a clear test
assertion that fails for out-of-range values. Preserve the existing unanimous
signing loop and valid actor-index signing behavior.
- Around line 503-513: Add a focused collision test near the existing
shared-owner uniqueness assertions: construct a non-shared CDeterministicMNState
whose keyIDOwner equals owner_keys[0].GetPubKey().GetID(), then call list.AddMN
and assert it throws or otherwise fails using the established duplicate-key
rejection pattern in these deterministic-MN tests. Keep the existing
HasUniqueProperty and cleanup assertions unchanged.
- Around line 361-388: Extend the “Every covered field changes the digest” cases
around CProRegTx::MakeSharedRegConsentHash to mutate and verify nType, nMode,
netInfo, and pubKeyOperator individually. Use valid alternate values for the
network address and operator public key, preserving the existing pattern that
each mutated registration produces a hash different from base and exercises
their versioned serialization wrappers.
- Around line 198-205: Add corresponding scriptRefund reuse cases in the
existing CollateralShares block, assigning shares[0].scriptRefund to
shares[1].keyIDOwner and then to voting_key.GetPubKey(), and assert both are
rejected with the appropriate reason. Ensure the second setup does not retain or
trigger an unintended duplicate-refund condition; use the expected reject reason
for the validation order.
🪄 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 Plus
Run ID: f457c6c4-384e-418e-a0ae-0ef778bf551e
📒 Files selected for processing (29)
doc/release-notes-7437.mdsrc/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/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/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
🚧 Files skipped from review as they are similar to previous changes (23)
- src/Makefile.test.include
- test/functional/test_runner.py
- src/evo/specialtx.h
- src/masternode/payments.cpp
- src/evo/assetlocktx.cpp
- src/messagesigner.h
- src/core_write.cpp
- src/rpc/rawtransaction.cpp
- src/evo/deterministicmns.cpp
- src/evo/dmnstate.cpp
- doc/release-notes-7437.md
- src/rpc/client.cpp
- src/common/bloom.cpp
- src/policy/policy.cpp
- src/evo/providertx.h
- test/functional/feature_masternode_shares.py
- src/evo/dmnstate.h
- src/validation.cpp
- src/txmempool.cpp
- src/evo/providertx.cpp
- src/evo/core_write.cpp
- src/rpc/evo.cpp
- src/evo/providertx_util.cpp
… submit time
SignAndSendSpecialTx ignored the completeness of the wallet signing
step, so submitting a transaction whose inputs this wallet cannot sign
died later in the mempool with an opaque script-verification error
("Operation not valid with the current stack size"). Check the signing
result and fail with the actual cause and the recovery path instead.
The typical way to hit this is running "protx shared_combine" for a
ProUpSharedRegTx on a wallet other than the one that ran
update_shared_registrar_prepare: combining inserts the share signatures
into the payload, which invalidates the prepare-time fee-input
signatures, and only the preparing wallet can re-sign them. Document
that wallet affinity in both RPCs' help text.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unit: bloom filters match a shared registration on every share's refund script, effective reward script and share owner key id, and the lifecycle transactions (ProDisTx, ProUpShareTx, ProUpSharedRegTx) on proTxHash plus their own new elements; BLOOM_UPDATE_ALL inserts the proTxHash on a registration or reward-script match so a watcher follows the masternode lifecycle automatically. Compact-filter element extraction is checked against the same field set, guarding the sync requirement between CheckSpecialTransactionMatchesAndUpdate and ExtractSpecialTxFilterElements. Functional: disconnecting a block containing a ProUpShareTx rolls the share table back and reconnecting replays it; disconnecting a dissolution restores the masternode with an identical share table and registration height and returns the ProDisTx to the mempool (where the restored masternode makes it valid again), and reconnecting removes both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The vbparams start time rendered as 0 (self.mocktime is unset when set_test_params runs) and the min_activation_height of 100 falls inside the ~119 blocks the framework setup mines, so v24 was already active when run_test began and activate_v24() was a silent no-op: no pre-activation behavior was ever exercised. Raise the earliest activation height to 250 and assert the pre-activation window actually exists. New coverage: each lifecycle transaction type (ProDisTx, ProUpShareTx, ProUpSharedRegTx) carries a trivially-valid payload and is driven through block connection before activation, rejecting with its dedicated too-early reason; the same dissolution after activation fails at the masternode lookup instead, proving the gate itself caused the rejections. The wallet path is also closed pre-activation (register_shared_prepare requires payload version 3). Also exercise the shared-specific revive gate: an operator-key change via ProUpSharedRegTx resets operator fields and PoSe-bans the masternode, and the new operator's ProUpServTx must revive it despite the null keyIDOwner (without the IsShared() carve-out in the revive condition a banned shared masternode could never be revived). Verify the reward split resumes from the unchanged share table afterwards.
…ters GetOwnerPayouts on a shared masternode state is empty (the owner reward derives from the share table instead), which left shared masternodes invisible wherever that helper fed a display or wallet filter: masternode winners printed an empty payee list, masternodelist's payee mode showed UNKNOWN, the GUI could not attribute a masternode to a wallet holding a share, protx list wallet skipped it, protx info reported hasOwnerKey/ownsPayeeScript false, and update_service's default fee-source fallback called front() on an empty vector (undefined behavior, reachable by any shared masternode operator omitting feeSourceAddress with no operator payout script set). Add CDeterministicMNState::GetOwnerRewardScripts(), returning each share's effective reward script for shared masternodes and the payout-list scripts otherwise, and use it in those display and filter paths. Wallet attribution also recognizes share owner keys (hasOwnerKey, protx list wallet). update_service now fails with a clear error asking for an explicit feeSourceAddress: a shared masternode's reward scripts belong to the participants, not the operator running the command, so there is no sensible default.
The ProDisTx signing digest commits to the signature count, and shared_sign always signs the unanimous digest (one signature per share). Passing it a transaction that already carries signatures - typically a unilateral standby built by "protx dissolve" with submit=false - silently produced signatures that could never verify on a one-signature transaction after shared_combine. A unilateral dissolution is built, signed and submitted in one step by "protx dissolve" and never involves the multi-party sign/combine flow, so fail fast with an error saying exactly that. Also spell the intended split out in the shared_sign help text.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9e86aa56b
ℹ️ 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".
| uint8_t shares_count{0}; | ||
| SER_WRITE(obj, shares_count = static_cast<uint8_t>(obj.shares.size())); | ||
| READWRITE(shares_count); |
There was a problem hiding this comment.
On an existing devnet that activated v24 under an earlier release, historical non-shared version-3 ProRegTx payloads encode inputsHash immediately after payouts. This decoder now unconditionally interprets those bytes as shares_count, shares, and penalty fields while still using ProTxVersion::ExtAddr, so old blocks can no longer be deserialized or reindexed correctly. Introduce a new payload version for the share fields, or otherwise retain the old version-3 layout.
Useful? React with 👍 / 👎.
| obj.nVersion >= ProTxVersion::ExtAddr)); | ||
| if (obj.nVersion >= ProTxVersion::ExtAddr) { | ||
| READWRITE(obj.payouts); | ||
| READWRITE(obj.payouts, obj.shares, obj.nEarlyPeriodBlocks, obj.nEarlyPenalty); |
There was a problem hiding this comment.
When upgrading a devnet node whose EvoDB already contains an ExtAddr masternode, existing dmn_S3 snapshots and dmn_D4 added-MN records encode scriptOperatorPayout immediately after payouts. The new reader instead consumes that position as shares and the two penalty fields, but the database keys are unchanged, so restart-time list reconstruction reads the old records with a shifted layout and can fail. Bump and migrate the affected database formats, or make the state encoding backward-aware.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Four in-scope correctness blockers remain: merkle-block filtering gates out the new lifecycle transactions, shared registrar and share updates can violate the voting/payee separation invariant, extended masternode diffs omit shared reward changes, and the Qt ownership filter misses wallets that hold only share owner keys. The three corrective commits at the end of the stack are valid commit-hygiene suggestions and should be folded into the commits that introduced the affected consensus or RPC behavior.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 3 suggestion(s)
6 additional finding(s) omitted (not in diff).
🤖 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 `src/merkleblock.cpp`:
- [BLOCKING] src/merkleblock.cpp:40-49: Allow the new lifecycle transactions through merkle-block filtering
CMerkleBlock calls CBloomFilter::IsRelevantAndUpdate only for special transaction types in this allowlist. TRANSACTION_PROVIDER_DISSOLVE, TRANSACTION_PROVIDER_UPDATE_SHARE, and TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR are absent, so BIP37 clients cannot match these transactions by the proTxHash, reward script, or voting key handled in common/bloom.cpp. The new tests invoke CBloomFilter directly and therefore bypass this outer gate; add the three types and cover them through CMerkleBlock.
In `src/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:1561-1571: Preserve share payee separation when changing the voting key
CheckProUpSharedRegTx accepts a voting key that is already the P2PKH destination of a share's refund or effective reward script, even though IsShareListTriviallyValid rejects that state at registration and CheckProUpShareTx rejects an equivalent reward update. There is also an in-block bypass because every transaction is checked against pindexPrev while RebuildListFromBlock applies ProUpShareTx and ProUpSharedRegTx sequentially without rechecking the invariant: a voting-key update to X and a reward update to X can both pass against the prior state and leave an otherwise forbidden final state. Validate the proposed voting key against all current share scripts and recheck the invariant against the evolving list after either update is applied.
In `src/evo/smldiff.cpp`:
- [BLOCKING] src/evo/smldiff.cpp:136-138: Carry shared reward data through extended masternode diffs
CDeterministicMN::to_sml_entry() maps shared states to an empty legacy payouts vector, while the extended comparison only checks scriptPayout, payouts, and scriptOperatorPayout. A range containing only a ProUpShareTx therefore omits the changed masternode from `protx diff ... true`, and newly added shared entries expose none of their share reward data in the extended response. Add the share data as memory-only CSimplifiedMNListEntry fields, include it in the extended comparison and JSON output, and continue excluding it from network serialization and CalcHash so the SML leaf format is unchanged.
In `src/node/interfaces.cpp`:
- [BLOCKING] src/node/interfaces.cpp:134-138: Expose share owner keys to the Qt ownership filter
The shared-aware payout vector only recognizes a participant when the GUI wallet can spend one of the effective reward scripts. MnEntryImpl::getKeyIdOwner still returns the shared state's null singular owner key, and MasternodeList::updateMasternodeList checks only that key plus payout scripts. A wallet holding a share owner key while directing rewards elsewhere is therefore omitted from the owned-masternode view even though it can authorize share updates and unilateral dissolution. Expose all share owner keys through the interface and test them in the Qt ownership filter; the owner-address display must also avoid encoding the null singular key as a synthetic address.
In `<commit:ef371ede648>`:
- [SUGGESTION] <commit:ef371ede648>:1: Squash the ProDisTx signing-digest fix into the shared-masternode consensus commit
Commit ef371ede648 remains a separate correction to CProDisTx::MakeSignHash introduced by ac616bc67c9. The tree at ac616bc67c9 declares MakeSignHash(const CTransaction&) and does not commit the signature count, allowing a signed unanimous dissolution to be reinterpreted as a different one-signature transaction with a different txid. The earlier commit's message already describes the corrected count commitment, so leaving ef371ede648 separate records a known-broken consensus-critical intermediate state and makes the earlier message inaccurate. Fold ef371ede648 into ac616bc67c9, placing its regression coverage in the consensus or shared-masternode test commit as appropriate.
In `<commit:a87101cae5c>`:
- [SUGGESTION] <commit:a87101cae5c>:1: Squash the deferred-dissolution ordering fix into the shared-masternode consensus commit
Commit a87101cae5c remains a separate correction to the list-rebuild path introduced by ac616bc67c9. In the earlier commit's tree, a ProDisTx validates and removes the masternode inside the provider-transaction loop, so a dissolution ordered before a same-masternode update makes the later update fail list construction. a87101cae5c moves removal into the collateral-spend sweep, matching the behavior already claimed by ac616bc67c9's commit message. Fold the correction into ac616bc67c9 so permanent history does not retain the miner-liveness failure.
In `<commit:e9e86aa56bf>`:
- [SUGGESTION] <commit:e9e86aa56bf>:1: Fold the shared_sign correction into the original signing flow
Commit e9e86aa56bf only corrects an unusable path created by the earlier RPC flow and signature-count change. Before this commit, shared_sign accepts an already-signed unilateral ProDisTx but signs the unanimous signature-count digest, so shared_combine cannot produce a transaction for which those signatures verify. Because this RPC behavior has not shipped independently, fold the guard and help changes into the commit introducing the count-sensitive signing flow, with the functional assertion placed alongside the existing shared-masternode tests.
| // 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"); | ||
| } | ||
|
|
||
| if (mnList.HasUniqueProperty(opt_ptx->pubKeyOperator)) { | ||
| auto otherDmn = mnList.GetUniquePropertyMN(opt_ptx->pubKeyOperator); | ||
| if (opt_ptx->proTxHash != otherDmn->proTxHash) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Preserve share payee separation when changing the voting key
CheckProUpSharedRegTx accepts a voting key that is already the P2PKH destination of a share's refund or effective reward script, even though IsShareListTriviallyValid rejects that state at registration and CheckProUpShareTx rejects an equivalent reward update. There is also an in-block bypass because every transaction is checked against pindexPrev while RebuildListFromBlock applies ProUpShareTx and ProUpSharedRegTx sequentially without rechecking the invariant: a voting-key update to X and a reward update to X can both pass against the prior state and leave an otherwise forbidden final state. Validate the proposed voting key against all current share scripts and recheck the invariant against the evolving list after either update is applied.
source: ['codex']
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 extended-address (version 3) ProTx
payload, so the two must ship in the same release.
sharestable turns an extended (v3) 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 extended 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, extended 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 extended (v3) 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 extended format would need a reset (no such mainnet or testnet chain
exists — v24 is not yet active there).
Checklist:
doc/release-notes-7437.md)