feat!: implement Decentralized Masternode Shares DIP - #7437
feat!: implement Decentralized Masternode Shares DIP#7437PastaPastaPasta wants to merge 16 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 — Opus deferred (commit b5ce7e3) |
|
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)
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 👍 / 👎.
There was a problem hiding this comment.
Fixed at the new head: TRANSACTION_PROVIDER_DISSOLVE/UPDATE_SHARE/UPDATE_SHARED_REGISTRAR are now in the CMerkleBlock allowlist (folded into f021946), and the filter unit test now drives a block containing the full lifecycle through CMerkleBlock and asserts all four transactions match (b8a4dad), covering the outer gate that the direct CBloomFilter checks bypassed.
🤖 Posted autonomously by Claude on behalf of pasta.
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed at the new head: RebuildListFromBlock now rechecks the voting/payee separation invariant against the list as rebuilt so far after applying each ProUpShareTx and ProUpSharedRegTx, so a same-block update pair that individually passes against pindexPrev can no longer leave the forbidden combination (folded into 25a6fcb). A matching mempool pair guard keeps an honest miner from assembling such a block in the first place (e9e4438).
🤖 Posted autonomously by Claude on behalf of pasta.
| // 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 👍 / 👎.
There was a problem hiding this comment.
Fixed at the new head: CheckProUpSharedRegTx now rejects a new voting key whose P2PKH destination matches any share's refund or effective reward script (bad-proupsharedreg-payee-reuse), via the new IsShareListVotingKeySafe helper that mirrors the registration-time rule (folded into 25a6fcb; functional coverage in fec8b6a).
🤖 Posted autonomously by Claude on behalf of pasta.
![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
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.
This was largely addressed by the rpc, gui commit (CheckWalletOwnsAnyOwnerKey / CheckWalletOwnsAnyPayout already match share owner keys and effective reward scripts in protx list wallet). The one residual gap — a share's refund script going unmatched when a distinct reward script is set — is fixed at the new head with CheckWalletOwnsAnyShareRefund (b5ce7e3), since the non-shared analog (the collateral UTXO via ListProTxCoins) cannot see the shared covenant output.
🤖 Posted autonomously by Claude on behalf of pasta.
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.
Fixed in the rpc, gui commit (now b5ce7e3): with no operator payout script and no feeSourceAddress, protx update_service on a shared masternode throws masternode has no default fee source; specify feeSourceAddress instead of calling front() on the empty payout list. We deliberately do not fall back to share reward scripts: those belong to the participants, not the operator issuing the update, so there is no sensible default fee source. Covered in feature_masternode_shares.py.
🤖 Posted autonomously by Claude on behalf of pasta.
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
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 👍 / 👎.
There was a problem hiding this comment.
This was addressed by the rpc, gui commit (now b5ce7e3) via CDeterministicMNState::GetOwnerRewardScripts(): masternode winners, the masternodelist payee modes, the GUI list, and protx list/info wallet attribution all report each share's effective reward script for shared masternodes. masternode payments was already correct, since it derives payees from the consensus payment computation (GetBlockTxOuts).
🤖 Posted autonomously by Claude on behalf of pasta.
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 👍 / 👎.
There was a problem hiding this comment.
Not reachable on Dash: CTxDestination has no witness variants (std::variant<CNoDestination, PKHash, ScriptHash>, script/standard.h) and DecodeDestination only produces PKHash/ScriptHash — bech32m strings are DIP-18 Platform addresses and decode to CNoDestination. Any address passing the RPC's IsValidDestination check therefore maps to exactly a P2PKH/P2SH script, which is precisely the set CProUpShareTx::IsTriviallyValid accepts (the shared-collateral template it additionally rejects is not expressible as an address). The RPC preflight is already equivalent to the consensus rule, so the claimed late-failure path has no realizable input.
🤖 Posted autonomously by Claude on behalf of pasta.
| 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.
Fixed at the new head: CSimplifiedMNListEntry gained a memory-only shares field (excluded from network serialization, operator==, and CalcHash, exactly like the existing mem-only DIP-0026 payouts), populated by to_sml_entry(), compared in BuildSimplifiedDiff's extended path, and emitted in extended JSON via ShareListToJson — so a ProUpShareTx-only range now appears in protx diff ... true and new shared entries expose their share data, while the consensus SML leaf format is unchanged (folded into 25a6fcb; unit test proving hash-invariance and extended-diff inclusion in fec8b6a).
🤖 Posted autonomously by Claude on behalf of pasta.
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
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 👍 / 👎.
There was a problem hiding this comment.
Fixed at the new head (b5ce7e3): MnEntry now exposes getShareOwnerKeyIds(), the Qt owned-masternode filter scans share owner keys via isSpendable(PKHash(...)), and MasternodeEntry no longer encodes the null singular owner key — it joins the share owner addresses for the display instead.
🤖 Posted autonomously by Claude on behalf of pasta.
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
…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>
e9e86aa to
eb53cbd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb53cbd632
ℹ️ 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 (mnList.HasUniqueProperty(opt_ptx->pubKeyOperator)) { | ||
| auto otherDmn = mnList.GetUniquePropertyMN(opt_ptx->pubKeyOperator); |
There was a problem hiding this comment.
When an existing legacy-scheme masternode holds the same underlying BLS key, this exact-encoding lookup misses it because legacy and basic encodings occupy different unique-property hashes. Unlike the ProRegTx and ordinary registrar paths, this permits unanimous shared owners to assign that key in its basic encoding, leaving two masternodes with the same operator key and violating the deterministic-list uniqueness invariant; use HasOperatorKeyUnderAnyScheme here as the companion paths do.
AGENTS.md reference: AGENTS.md:L166-L169
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at the new head: CheckProUpSharedRegTx now uses HasOperatorKeyUnderAnyScheme(..., self=proTxHash), matching the ProRegTx and ordinary ProUpRegTx paths, and RebuildListFromBlock re-probes the evolving list before UpdateMN when the operator key changes, so a same-block pair cannot reach the collision through an exception (folded into 25a6fcb). The mempool cross-scheme guard also gained an UPDATE_SHARED_REGISTRAR branch so the pair is kept out of honest templates (e9e4438).
🤖 Posted autonomously by Claude on behalf of pasta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Five blocking correctness issues remain at eb53cbd: the new lifecycle transactions are excluded from merkle-block bloom matching, voting/payee invariants can be violated by registrar and same-block updates, extended SML diffs lose shared reward changes, Qt cannot identify ownership through share owner keys, and shared registrar updates do not enforce operator-key uniqueness under both BLS encodings or against evolving block state. The three prior commit-hygiene findings remain valid; the new registration serializer should also reject mismatched share/signature vectors, and the consensus commit message still names the wrong ProRegTx version.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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)
🔴 5 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
7 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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 remain absent, so BIP37 clients cannot match these transactions through the proTxHash, reward script, or voting key handling added to common/bloom.cpp. The new tests invoke CBloomFilter directly and therefore bypass this outer gate; add the three types and cover the behavior through CMerkleBlock.
In `src/evo/smldiff.cpp`:
- [BLOCKING] src/evo/smldiff.cpp:134-138: Carry shared reward data through extended masternode diffs
CDeterministicMN::to_sml_entry() supplies GetOwnerPayouts(state), which is empty for shared states, and the extended comparison checks only scriptPayout, payouts, and scriptOperatorPayout. Consequently, a range containing only a ProUpShareTx can omit the changed masternode from `protx diff ... true`, while newly added shared entries expose none of their share reward data. Add memory-only shared payout/share fields to CSimplifiedMNListEntry, include them in the extended comparison and JSON output, and keep them out of network serialization and CalcHash so the consensus SML leaf format remains unchanged.
In `src/node/interfaces.cpp`:
- [BLOCKING] src/node/interfaces.cpp:107-140: Expose share owner keys to the Qt ownership filter
The interface now exports shared reward scripts, but getKeyIdOwner() still returns the shared state's null singular owner key. MasternodeList checks only that key, voting/collateral/operator ownership, and the reward scripts, so a participant wallet holding a share owner key while directing rewards elsewhere is omitted from the owned-masternode view even though it can authorize share updates and unilateral dissolution. Expose all share owner keys through MnEntry and scan them in the Qt filter; MasternodeEntry must also avoid encoding the null singular key as an owner address.
In `src/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:1681-1686: Enforce operator-key uniqueness for shared registrar updates
This check probes only the payload's exact basic-scheme CBLSLazyPublicKey encoding. It therefore misses an existing masternode holding the same underlying BLS key under the legacy encoding, unlike the ProRegTx and ordinary ProUpRegTx paths that use HasOperatorKeyUnderAnyScheme. RebuildListFromBlock also calls UpdateMN at line 686 without checking the evolving list, so two shared registrar updates in one block can target the same operator key after both pass against pindexPrev; the second update then reaches the collision through an exception, including on the block-template path. Use HasOperatorKeyUnderAnyScheme with proTxHash as self in both the transaction check and the evolving-list path before UpdateMN.
- [BLOCKING] src/evo/specialtxman.cpp:1621-1686: Preserve share payee separation when changing the voting key
(existing thread: https://github.com/dashpay/dash/pull/7437#discussion_r3696863058)
CheckProUpSharedRegTx accepts a voting key whose P2PKH destination is already used by a share's refund or effective reward script, producing a state that registration rejects. The per-transaction checks also use only the list at pindexPrev, while RebuildListFromBlock applies share and registrar updates at lines 643-686 without rechecking the invariant against the evolving state. A voting-key update to X and a reward update to X can therefore both pass against the prior state and leave the forbidden combination. Validate the proposed voting destination against all current share scripts and recheck the invariant after either update against the list rebuilt so far.
In `<commit:d2f6421715b>`:
- [SUGGESTION] <commit:d2f6421715b>:1: Squash the ProDisTx signing-digest fix into the shared-masternode consensus commit
Commit d2f6421715b remains a separate correction to CProDisTx::MakeSignHash introduced by 0493061b170. Before this correction, the signature count was absent from the digest, allowing a signed unanimous dissolution to be reinterpreted as a different one-signature transaction with a different txid, even though the earlier commit message already says the count is committed. Fold the production changes into 0493061b170 and place the regression coverage with the corresponding consensus or shared-masternode tests so the permanent history does not retain the known-broken digest.
In `<commit:15d0c9f3c17>`:
- [SUGGESTION] <commit:15d0c9f3c17>:1: Squash the deferred-dissolution ordering fix into the shared-masternode consensus commit
Commit 15d0c9f3c17 remains a separate correction to the list-rebuild ordering introduced by 0493061b170. In the earlier tree, ProDisTx removes the masternode during the provider-transaction loop, so a dissolution ordered before a same-masternode update makes the later update fail and aborts block-template production. The originating commit message already describes the corrected collateral-spend phase and order-independent behavior. Fold this correction into 0493061b170 and place its regression case with the relevant tests.
In `<commit:eb53cbd6321>`:
- [SUGGESTION] <commit:eb53cbd6321>:1: Fold the shared_sign correction into the original signing flow
Commit eb53cbd6321 is a standalone correction to the shared signing flow introduced by 3062cb52504 and made count-sensitive by d2f6421715b. Without the guard, shared_sign accepts an already-signed unilateral ProDisTx but signs the unanimous signature-count digest; passing the resulting single actor signature through shared_combine creates a one-signature transaction whose signature does not verify. Fold the guard and help text into the commit introducing the count-sensitive RPC flow, with the functional assertion alongside the original shared-masternode tests.
In `src/evo/providertx.h`:
- [SUGGESTION] src/evo/providertx.h:188-199: Reject mismatched share and join-signature counts while serializing
The writer emits shares_count from shares.size() but writes every element of vchJoinSigs without separately encoding its length. If the vectors differ, the writer silently emits a payload that the corresponding reader cannot round-trip: too few signatures make subsequent fields get consumed as signatures, while too many leave or shift extra bytes. Consensus validation rejects the semantic mismatch, but malformed in-memory objects cannot reach that validation coherently. Fail the write before emitting shares_count unless vchJoinSigs.size() equals shares.size(), following CompactSignatureFormatter's existing fail-fast serialization behavior.
In `<commit:0493061b170>`:
- [NITPICK] <commit:0493061b170>:1: Correct the ProRegTx payload version in the consensus commit message
Commit 0493061b170 describes shared registration as using a "version 4 ProRegTx," but ProTxVersion::ExtAddr is version 3 throughout this stack and the PR description and RPC help correctly call it version 3. Correct the commit message so the permanent history does not document the wrong wire-format version.
| 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: Enforce operator-key uniqueness for shared registrar updates
This check probes only the payload's exact basic-scheme CBLSLazyPublicKey encoding. It therefore misses an existing masternode holding the same underlying BLS key under the legacy encoding, unlike the ProRegTx and ordinary ProUpRegTx paths that use HasOperatorKeyUnderAnyScheme. RebuildListFromBlock also calls UpdateMN at line 686 without checking the evolving list, so two shared registrar updates in one block can target the same operator key after both pass against pindexPrev; the second update then reaches the collision through an exception, including on the block-template path. Use HasOperatorKeyUnderAnyScheme with proTxHash as self in both the transaction check and the evolving-list path before UpdateMN.
source: ['codex']
There was a problem hiding this comment.
Fixed at the new head — same change set as the codex thread above: HasOperatorKeyUnderAnyScheme in the per-transaction check, an evolving-list re-probe before UpdateMN in the rebuild path, and an UPDATE_SHARED_REGISTRAR branch in existsProviderTxCrossSchemeConflict (folded into 25a6fcb / e9e4438).
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Enforce operator-key uniqueness for shared registrar updates no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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.
🟡 Suggestion: Reject mismatched share and join-signature counts while serializing
The writer emits shares_count from shares.size() but writes every element of vchJoinSigs without separately encoding its length. If the vectors differ, the writer silently emits a payload that the corresponding reader cannot round-trip: too few signatures make subsequent fields get consumed as signatures, while too many leave or shift extra bytes. Consensus validation rejects the semantic mismatch, but malformed in-memory objects cannot reach that validation coherently. Fail the write before emitting shares_count unless vchJoinSigs.size() equals shares.size(), following CompactSignatureFormatter's existing fail-fast serialization behavior.
source: ['coderabbit']
There was a problem hiding this comment.
Fixed at the new head: the writer throws join signature count mismatch before emitting shares_count unless vchJoinSigs.size() == shares.size(), following CompactSignatureFormatter's fail-fast pattern as suggested (folded into 0543adf; unit test in fec8b6a).
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Reject mismatched share and join-signature counts while serializing no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
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>
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 3 (extended) 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>
…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>
…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>
… 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.
eb53cbd to
b5ce7e3
Compare
|
Pushed
History rework per the commit-hygiene findings: the three corrective tail commits are folded into the commits that introduced the affected code (production hunks into CI notes: the Lint failure was cppcheck 🤖 Posted autonomously by Claude on behalf of pasta. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5ce7e366c
ℹ️ 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".
| // fields and put the MN into PoSe-banned state until a new ProUpServTx arrives | ||
| newState->ResetOperatorFields(); | ||
| newState->BanIfNotBanned(nHeight); | ||
| newState->pubKeyOperator = opt_proTx->pubKeyOperator; |
There was a problem hiding this comment.
When a block places a ProUpSharedRegTx that rotates the operator key before a ProUpServTx for the same masternode, the service transaction was validated against pindexPrev and can therefore be signed by the old operator; this branch installs the new key, but the later service-update branch applies the old operator's transaction to the evolving state without rechecking its signature, allowing a displaced operator to restore its payout/service fields and immediately revive the masternode after the unanimous rotation. Reject this pairing/order or revalidate the service update against the evolving operator key.
AGENTS.md reference: AGENTS.md:L166-L169
Useful? React with 👍 / 👎.
| "\nIMPORTANT: once the registration confirms, every participant should create a standby dissolution\n" | ||
| "(\"protx dissolve <proTxHash> <shareIndex> <fee> false\") and store the returned hex with their\n" | ||
| "refund-key backup, separately from the share owner key. Dissolution validity is monotone, so a\n" | ||
| "standby never expires; if the owner key is later lost, broadcasting the standby recovers the\n" |
There was a problem hiding this comment.
When this command is followed during the early period, omitting the fifth payPenalty argument makes it default to true, so the stored transaction permanently transfers nEarlyPenalty away from the actor even if it is not broadcast until years after the penalty period ends. Since the text promises recovery of the participant's principal and the penalty may approach the entire share, recommend ... <fee> false false for a post-boundary standby (or explicitly recommend storing both variants and warn about the loss).
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Two blocking issues remain: operator-authorized lifecycle transactions are not revalidated after a same-block shared registrar key rotation, and the recommended standby-dissolution command can permanently pay an avoidable early penalty while promising principal recovery. The RPC result documentation for update_share and one stale version reference in the RPC commit message also need correction.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 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/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:477-485: Revalidate old-operator transactions after a shared registrar update
CheckProUpServTx and CheckProUpRevTx verify signatures against the operator key in the list at pindexPrev, but RebuildListFromBlock applies them to the evolving state without rechecking authorization. If a ProUpSharedRegTx rotates the operator key earlier in the block, a later ProUpServTx signed by the displaced operator can overwrite service and operator-payout fields and revive the masternode under the new key. A later old-key ProUpRevTx can reset the newly installed operator key and ban the masternode. Revalidate operator-authorized lifecycle transactions against the evolving operator key, or reject these same-block orderings; mempool conflict handling is not sufficient because a miner can construct the block directly.
In `src/rpc/evo.cpp`:
- [BLOCKING] src/rpc/evo.cpp:2669-2673: Do not recommend a penalty-paying standby as principal recovery
The recommended command omits payPenalty. During the early period, protx dissolve therefore defaults payPenalty to true and permanently encodes nEarlyPenalty into the transaction outputs. Broadcasting that stored transaction after the early period still redistributes the penalty because the later zero-penalty rule is minimum-based and accepts the overpayment. This contradicts the promise that the standby recovers the participant's principal and can cause the loss of most of a share. Recommend the explicit submit=false, payPenalty=false form for a standby that becomes valid at the early-period boundary, or document separate immediate and post-boundary variants with a clear penalty warning.
- [SUGGESTION] src/rpc/evo.cpp:1776: Document the submit=false result for update_share
protx update_share passes the submit argument to SignAndSendSpecialTx, which returns the serialized signed transaction when submit=false. The result schema still promises only a transaction ID, unlike the established conditional schemas used by the other ProTx RPCs. Document both return forms so generated help and RPC clients correctly describe the API.
In `<commit:064d25a0662>`:
- [NITPICK] <commit:064d25a0662>:1: Correct the payload version in the RPC commit message
Commit 064d25a0662 says protx register_shared_prepare appends a "v4 shared payload," but the implementation uses ProTxVersion::ExtAddr, which is version 3. The implementation, RPC help, consensus commit, and PR description all identify the payload as version 3. Correct the remaining commit-message reference so permanent history does not document a nonexistent wire-format version.
| "\nIMPORTANT: once the registration confirms, every participant should create a standby dissolution\n" | ||
| "(\"protx dissolve <proTxHash> <shareIndex> <fee> false\") and store the returned hex with their\n" | ||
| "refund-key backup, separately from the share owner key. Dissolution validity is monotone, so a\n" | ||
| "standby never expires; if the owner key is later lost, broadcasting the standby recovers the\n" | ||
| "participant's principal without any other party's cooperation.\n", |
There was a problem hiding this comment.
🔴 Blocking: Do not recommend a penalty-paying standby as principal recovery
The recommended command omits payPenalty. During the early period, protx dissolve therefore defaults payPenalty to true and permanently encodes nEarlyPenalty into the transaction outputs. Broadcasting that stored transaction after the early period still redistributes the penalty because the later zero-penalty rule is minimum-based and accepts the overpayment. This contradicts the promise that the standby recovers the participant's principal and can cause the loss of most of a share. Recommend the explicit submit=false, payPenalty=false form for a standby that becomes valid at the early-period boundary, or document separate immediate and post-boundary variants with a clear penalty warning.
| "\nIMPORTANT: once the registration confirms, every participant should create a standby dissolution\n" | |
| "(\"protx dissolve <proTxHash> <shareIndex> <fee> false\") and store the returned hex with their\n" | |
| "refund-key backup, separately from the share owner key. Dissolution validity is monotone, so a\n" | |
| "standby never expires; if the owner key is later lost, broadcasting the standby recovers the\n" | |
| "participant's principal without any other party's cooperation.\n", | |
| "\nIMPORTANT: once the registration confirms, every participant should create a zero-penalty\n" | |
| "standby dissolution (\"protx dissolve <proTxHash> <shareIndex> <fee> false false\") and store\n" | |
| "the returned hex with their refund-key backup, separately from the share owner key. It becomes\n" | |
| "valid when the early period ends (or immediately when no early period applies), then never expires;\n" | |
| "broadcasting it after that point recovers the participant's principal without cooperation.\n", |
source: ['codex']
| {"feeSourceAddress", RPCArg::Type::STR, RPCArg::Optional::NO, "Wallet address to pay the transaction fee from."}, | ||
| {"submit", RPCArg::Type::BOOL, RPCArg::Default{true}, "Submit the transaction to the network."}, | ||
| }, | ||
| RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction id"}, |
There was a problem hiding this comment.
🟡 Suggestion: Document the submit=false result for update_share
protx update_share passes the submit argument to SignAndSendSpecialTx, which returns the serialized signed transaction when submit=false. The result schema still promises only a transaction ID, unlike the established conditional schemas used by the other ProTx RPCs. Document both return forms so generated help and RPC clients correctly describe the API.
| 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"}, | |
| }, |
source: ['coderabbit']
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 extended-address
(version 3) ProTx payload. The version 3 parser shipped in v23.0.x, but no
released chain can contain version 3 data: every tagged release rejects v3
ProTx with
bad-protx-versionuntilDEPLOYMENT_V24activates, and V24 isNEVER_ACTIVEon mainnet and testnet in all releases. The layout has alreadybeen redefined once on develop (DIP-0026), unreleased, under the same policy;
both changes 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) and is carried through extended
protx diffoutput via a memory-onlyCSimplifiedMNListEntryfield so share reward changes are visible toextended-diff consumers without touching the consensus leaf format; share
owner keys share the
keyIDOwneruniqueness namespace so reuse is rejected inboth directions; voting-key and share-script separation is enforced at
registration, on every update (including cross-BLS-encoding operator-key
uniqueness and same-block interplay, rechecked against the evolving list
during block processing), and mirrored by mempool pair guards so honest block
assembly can never pick a combination consensus would reject; mempool conflict
tracking and eviction cover the new types; the template output/prevout get
targeted relay carve-outs; and bloom/GCS filters and
CMerkleBlockmatch thenew transactions and share fields for light clients.
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, the full ProDisTx validation matrix (penalty floors,
monotonicity across the early-period boundary, and the signature-count
malleability guard), fail-fast serialization of a share/join-signature count
mismatch,
CMerkleBlock-level matching of the full shared lifecycle, andextended-SML-diff inclusion of share reward changes with an unchanged
consensus leaf hash.
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, rejection of a registrar update
moving the voting key onto a share's payee script, and both directions of the
mempool guard that keeps a conflicting share-update/registrar-update pair out
of one block template. 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).
Follow-up work
Planned after this PR, deliberately out of scope here:
ownership); a guided multi-party registration and consent-signing flow (and
standby-dissolution management) is a natural follow-up once the RPC flow has
soaked.
deviations listed above) before the release that activates v24.
on a fresh devnet, including the documented reset of any pre-release chain
that activated v24 on an older extended-payload layout.
protx diffoutput and the bloom/GCS matching in mobile SDKs and Platformservices that track masternode payees.
standby dissolutions as the required penalty decays.
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 extended (v3) ProRegTx payload — which no released chain can
contain, since V24 is
NEVER_ACTIVEon mainnet and testnet in every release —it must ship in the same release as DIP-0026 (#7340); any pre-release
devnet/regtest chain that already activated v24 on an older extended format
needs a reset.
Checklist:
doc/release-notes-7437.md)