docs: add decentralized masternode shares draft DIP#185
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new DIP draft, "Decentralized Masternode Shares," is added as ChangesDecentralized Masternode Shares DIP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 51ec7ac) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent blocking findings on the v5 shared-masternode draft: the registration-consent sharesWithoutJoinSigs serialization is consensus-ambiguous, and the ProUpShareTx / ProUpSharedRegTx authorization digests sign nLockTime without binding input sequences (asymmetric with the registration and dissolution digests that already commit to sequences). One additional blocking issue: the draft claims DIP-0026 occupies provider payload version 4, but DIP-0026 in this repo currently specifies version 2, leaving the version ladder for SharedCollateral = 5 undefined. Two nitpicks on a wording inconsistency for the ProUpShareTx key-reuse rule and a logically inconsistent reorg test case.
🔴 3 blocking | 💬 2 nitpick(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 `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:514-516: `sharesWithoutJoinSigs` serialization is ambiguous and consensus-critical
The text says `sharesWithoutJoinSigs` is the concatenation of every CollateralShare with the `joinSig` field `omitted (treated as a zero-length byte string)`. Two reasonable readings produce different byte sequences and therefore different SharedRegConsentHash values:
1. The `joinSig` field is omitted entirely — no bytes at all in the concatenation.
2. The `joinSig` field is serialized as a zero-length compact-size bytestring — i.e., the single byte `0x00` (compact-size length 0, followed by no payload bytes), preserving the field's framing.
`CollateralShare` (line 339) defines `joinSig` as `vector<unsigned char> (compact-size length-prefixed, ...)`, which makes interpretation (2) the natural literal reading of `serialize(share)` with an empty vector — yet the word `omitted` suggests (1). Because every participant signs this digest and consensus recomputes it to verify `joinSig`, divergent implementations would either reject otherwise-valid v5 registrations or, once one implementation is patched, cause a chain split. The DIP must pin one encoding normatively and ideally show pseudocode (e.g., `serialize(share) with joinSig replaced by compact_size(0)` vs. `serialize(share) skipping the joinSig field entirely`).
- [BLOCKING] dip-decentralized-masternode-shares.md:573-636: ProUpShareTx and ProUpSharedRegTx digests sign nLockTime without binding input sequences
The registration digest (line 468) commits to `sequencesHash`, and the dissolution digest (line 775) explicitly commits to `tx.vin[0].nSequence`; in both cases the DIP states the reason is to stop a coordinator/fee-payer from changing finality, RBF, or `nLockTime` semantics after participants sign (lines 500–504, 783–787). The ProUpShareTx digest (lines 573–580) and the ProUpSharedRegTx digest (lines 628–636) commit only to `inputsHash`, `tx.nLockTime`, and `tx.nVersion`/`tx.nType`; they bind no input `nSequence`.
Because `nLockTime` is only enforced when at least one input has a non-final sequence, a party that controls or replaces the funding input signatures (which need not be SIGHASH_ALL/AC-payable in a way that commits to sequences) can flip an input to a final sequence after the participant signs, effectively neutralising the signed `nLockTime` (e.g., causing a future-dated reward-script update or shared-registrar update to take effect immediately). The risk is particularly acute for ProUpSharedRegTx, where every participant signs but a separate broadcaster/fee-payer typically supplies the funding inputs.
Mirror the registration/dissolution model by adding `sequencesHash` (or an enumerated commitment to each input's `nSequence`) to both update digests, or — if the omission is deliberate because these updates do not bind finality — add a normative one-line note explaining why and remove `nLockTime` from these digests so implementers do not believe the spec enforces something it does not.
- [BLOCKING] dip-decentralized-masternode-shares.md:111-116: Provider payload versioning conflicts with the referenced DIP-0026
This draft normatively claims `DIP-0026 introduces provider transaction payload version 4 (MultiPayout)` and then allocates `SharedCollateral = 5` for v5. The DIP-0026 document checked into this repository (`dip-0026.md` line 46) says it introduces `version 2` of ProRegTx/ProUpRegTx with `payoutShares`/`payoutSharesSize`, and makes no claim about versions 3 or 4. As a result, implementers reading only the merged DIPs cannot determine whether DIP-0026 is v2 or v4, what occupies version 3, or whether `SharedCollateral` should actually be version 3 (next free after DIP-0026) rather than version 5.
The provider payload version is a consensus selector, so this is the kind of registry mismatch the review guidance flags as blocking. Either amend DIP-0026 in this repo to match (renumber its payload to v4 and reserve v3 / specify what it is), or change this DIP's version allocation to be consistent with DIP-0026 as merged. The 'Open Issues' section does not currently cover this dependency, so add it there or in 'Relationship to DIP-0026' with the resolution path.
| * `sharesWithoutJoinSigs` is the concatenation of every `CollateralShare` | ||
| in share order with the `joinSig` field omitted (treated as a zero-length | ||
| byte string) so that signatures are not self-referential. |
There was a problem hiding this comment.
🔴 Blocking: sharesWithoutJoinSigs serialization is ambiguous and consensus-critical
The text says sharesWithoutJoinSigs is the concatenation of every CollateralShare with the joinSig field omitted (treated as a zero-length byte string). Two reasonable readings produce different byte sequences and therefore different SharedRegConsentHash values:
- The
joinSigfield is omitted entirely — no bytes at all in the concatenation. - The
joinSigfield is serialized as a zero-length compact-size bytestring — i.e., the single byte0x00(compact-size length 0, followed by no payload bytes), preserving the field's framing.
CollateralShare (line 339) defines joinSig as vector<unsigned char> (compact-size length-prefixed, ...), which makes interpretation (2) the natural literal reading of serialize(share) with an empty vector — yet the word omitted suggests (1). Because every participant signs this digest and consensus recomputes it to verify joinSig, divergent implementations would either reject otherwise-valid v5 registrations or, once one implementation is patched, cause a chain split. The DIP must pin one encoding normatively and ideally show pseudocode (e.g., serialize(share) with joinSig replaced by compact_size(0) vs. serialize(share) skipping the joinSig field entirely).
source: ['claude']
| ```text | ||
| SHA256d("DashSharedMNUpShare" || | ||
| chainGenesisHash || | ||
| LE16(tx.nVersion) || LE16(tx.nType) || LE32(tx.nLockTime) || | ||
| inputsHash || | ||
| LE16(payload.nVersion) || | ||
| proTxHash || LE16(shareIndex) || newRewardScript) | ||
| ``` | ||
|
|
||
| `chainGenesisHash` is the 32-byte genesis block hash of the network | ||
| validating the transaction. `payload.nVersion` is the ProUpShareTx payload | ||
| version (currently `1`). | ||
|
|
||
| A valid `ProUpShareTx` replaces `shares[shareIndex].rewardScript` in the | ||
| deterministic masternode state. It does not revive a PoSe-banned masternode | ||
| and does not affect any other field. | ||
|
|
||
| #### ProUpSharedRegTx (type 12) | ||
|
|
||
| A `ProUpSharedRegTx` updates fields that affect all participants. In this | ||
| DIP the mutable shared-registrar fields are `pubKeyOperator`, `keyIDVoting`, | ||
| and `nOperatorReward`. | ||
|
|
||
| ```text | ||
| CProUpSharedRegTx { | ||
| nVersion : uint16_t // MUST be 1 (payload version of this new special tx type) | ||
| proTxHash : uint256 | ||
| pubKeyOperator : CBLSPublicKey // basic scheme | ||
| keyIDVoting : CKeyID | ||
| nOperatorReward : uint16_t | ||
| inputsHash : uint256 | ||
| vchSigs : vector<vector<unsigned char>> // exactly shares.size() entries, in share order | ||
| } | ||
| ``` | ||
|
|
||
| As with `ProUpShareTx`, this `nVersion` is the independent payload version | ||
| of the new `TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR` special | ||
| transaction and is unrelated to `ProTxVersion::SharedCollateral = 5`. | ||
|
|
||
| Validation: | ||
|
|
||
| 1. The masternode identified by `proTxHash` MUST exist and MUST be a v5 | ||
| masternode. | ||
| 2. `pubKeyOperator` MUST be a valid basic-scheme BLS public key and MUST | ||
| NOT collide with the operator key of any other registered masternode. | ||
| 3. `keyIDVoting` MUST NOT equal any `shares[i].ownerKey`. | ||
| 4. No current `refundScript` and no current effective `rewardScript` may be a | ||
| P2PKH script paying to `keyIDVoting`. For this check, an empty | ||
| `rewardScript` is interpreted as the corresponding `refundScript`. | ||
| 5. `nOperatorReward <= 10000`. | ||
| 6. `inputsHash` MUST equal `CalcTxInputsHash(tx)`. | ||
| 7. `vchSigs.size()` MUST equal `shares.size()`. | ||
| 8. For each `i` in `[0, shares.size())`, `vchSigs[i]` MUST be a valid | ||
| ECDSA compact signature by `shares[i].ownerKey` over: | ||
|
|
||
| ```text | ||
| SHA256d("DashSharedMNUpSharedReg" || | ||
| chainGenesisHash || | ||
| LE16(tx.nVersion) || LE16(tx.nType) || LE32(tx.nLockTime) || | ||
| inputsHash || | ||
| LE16(payload.nVersion) || | ||
| proTxHash || pubKeyOperatorSerialized || | ||
| keyIDVoting || LE16(nOperatorReward)) | ||
| ``` |
There was a problem hiding this comment.
🔴 Blocking: ProUpShareTx and ProUpSharedRegTx digests sign nLockTime without binding input sequences
The registration digest (line 468) commits to sequencesHash, and the dissolution digest (line 775) explicitly commits to tx.vin[0].nSequence; in both cases the DIP states the reason is to stop a coordinator/fee-payer from changing finality, RBF, or nLockTime semantics after participants sign (lines 500–504, 783–787). The ProUpShareTx digest (lines 573–580) and the ProUpSharedRegTx digest (lines 628–636) commit only to inputsHash, tx.nLockTime, and tx.nVersion/tx.nType; they bind no input nSequence.
Because nLockTime is only enforced when at least one input has a non-final sequence, a party that controls or replaces the funding input signatures (which need not be SIGHASH_ALL/AC-payable in a way that commits to sequences) can flip an input to a final sequence after the participant signs, effectively neutralising the signed nLockTime (e.g., causing a future-dated reward-script update or shared-registrar update to take effect immediately). The risk is particularly acute for ProUpSharedRegTx, where every participant signs but a separate broadcaster/fee-payer typically supplies the funding inputs.
Mirror the registration/dissolution model by adding sequencesHash (or an enumerated commitment to each input's nSequence) to both update digests, or — if the omission is deliberate because these updates do not bind finality — add a normative one-line note explaining why and remove nLockTime from these digests so implementers do not believe the spec enforces something it does not.
source: ['claude', 'codex']
| DIP-0026 introduces provider transaction payload version `4` (`MultiPayout`). | ||
| Under DIP-0026 v4, the registrar owner remains the sole signer for ProRegTx | ||
| and ProUpRegTx, and may freely rewrite the payout list at any time. | ||
|
|
||
| This DIP does not modify DIP-0026 v4 semantics. Shared masternodes use a new | ||
| provider transaction payload version, `5` (`SharedCollateral`), with the |
There was a problem hiding this comment.
🔴 Blocking: Provider payload versioning conflicts with the referenced DIP-0026
This draft normatively claims DIP-0026 introduces provider transaction payload version 4 (MultiPayout) and then allocates SharedCollateral = 5 for v5. The DIP-0026 document checked into this repository (dip-0026.md line 46) says it introduces version 2 of ProRegTx/ProUpRegTx with payoutShares/payoutSharesSize, and makes no claim about versions 3 or 4. As a result, implementers reading only the merged DIPs cannot determine whether DIP-0026 is v2 or v4, what occupies version 3, or whether SharedCollateral should actually be version 3 (next free after DIP-0026) rather than version 5.
The provider payload version is a consensus selector, so this is the kind of registry mismatch the review guidance flags as blocking. Either amend DIP-0026 in this repo to match (renumber its payload to v4 and reserve v3 / specify what it is), or change this DIP's version allocation to be consistent with DIP-0026 as merged. The 'Open Issues' section does not currently cover this dependency, so add it there or in 'Relationship to DIP-0026' with the resolution path.
source: ['codex']
| 4. `newRewardScript` MUST satisfy the same key-reuse restrictions as | ||
| registration: it MUST NOT be a P2PKH paying to any | ||
| `shares[i].ownerKey` and MUST NOT pay to `keyIDVoting`. |
There was a problem hiding this comment.
💬 Nitpick: ProUpShareTx key-reuse rule wording is inconsistent with registration rule 8
Registration rule 8 (lines 373–377) restricts refundScript/rewardScript from being a P2PKH script paying to the listed key IDs. ProUpShareTx rule 4 reads it MUST NOT be a P2PKH paying to any shares[i].ownerKey and MUST NOT pay to keyIDVoting. The second clause drops the P2PKH qualifier, so it can be read either as more restrictive (any script form paying to keyIDVoting, which is not well-defined for non-P2PKH templates) or as a wording slip. Aligning the wording with registration rule 8 removes the ambiguity for the consensus check.
| 4. `newRewardScript` MUST satisfy the same key-reuse restrictions as | |
| registration: it MUST NOT be a P2PKH paying to any | |
| `shares[i].ownerKey` and MUST NOT pay to `keyIDVoting`. | |
| 4. `newRewardScript` MUST satisfy the same key-reuse restrictions as | |
| registration: it MUST NOT be a P2PKH paying to any | |
| `shares[i].ownerKey` and MUST NOT be a P2PKH paying to `keyIDVoting`. |
source: ['claude']
| 4. A reorg that replays a `ProUpSharedRegTx` followed by a `ProDisTx` | ||
| in a different order produces the same final state. |
There was a problem hiding this comment.
💬 Nitpick: Reorg test case 4 description is logically inconsistent
Test case 4 says A reorg that replays a ProUpSharedRegTx followed by a ProDisTx in a different order produces the same final state. Read literally, the two orders are [ProUpSharedRegTx, ProDisTx] and [ProDisTx, ProUpSharedRegTx]. Under this spec a valid ProDisTx removes the masternode entry, so the subsequent ProUpSharedRegTx against the same proTxHash would be invalid (the masternode no longer exists). The two orders cannot both produce the same successful final state. Reword so the test target is unambiguous (e.g., independent updates on disjoint masternodes interleaved across a reorg, or that disconnect-then-reconnect reproduces the original deterministic state regardless of intra-block ordering).
source: ['claude']
|
@coderabbitai review |
| independent `nVersion` field that starts at `1` (see each payload | ||
| definition). | ||
|
|
||
| Provider payload version `5` is reserved for shared-collateral semantics. |
There was a problem hiding this comment.
I think that this is a poor decision. It seems to be that we should be able to have a specific individual version to serve both shared and non-shared masternodes. Either we have a flag which indicates shared, or we detect it based on length of the owners or something where only 1 shows it's not shared.
There was a problem hiding this comment.
Acknowledged. I removed the paragraph that tried to reserve v5/future versioning as a generic-mode decision. I left the current draft using separate SharedCollateral = 5, but called out in the PR description that this is open design feedback to resolve before ready-for-review: separate provider payload version vs one version with an explicit shared flag or share-count discriminator. I did not silently redesign that part without a concrete direction.
There was a problem hiding this comment.
I am saying this IS the design decision. I do not wish 1 version to mean semantically shared and another to mean not. Instead a flag or something else such as length of scripts should say that
There was a problem hiding this comment.
Addressed in 1b778bc by removing the new ProRegTx provider payload version and making shared collateral a v4 mode selected by an explicit isSharedCollateral discriminator. The draft now states that non-shared v4 remains governed by DIP-0026, shared behavior gates on state.isSharedCollateral, and the registration consent digest commits to the discriminator.
| | `TRANSACTION_PROVIDER_UPDATE_SHARE` | 11 | `ProUpShareTx` payload (this DIP). | | ||
| | `TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR` | 12 | `ProUpSharedRegTx` payload (this DIP). | | ||
|
|
||
| The numeric assignments `10`, `11`, and `12` are tentative. Before this DIP |
There was a problem hiding this comment.
Don't have hedging paragraphs like this. For stuff similar to this keep it in the PR description not in the dip.
There was a problem hiding this comment.
Done — moved the dependency/merge-blocking prose out of the DIP and into the PR description. The DIP now states the DIP-0026 v4 relationship directly without the PR-status hedging.
Introduces a draft DIP describing shared-collateral masternodes with 2-8 participants, ProTxVersion 5 (SharedCollateral), per-participant consent for registration and shared-registrar updates, consensus enforcement of a dissolution covenant on the shared collateral output, and a new CProDisTx (TRANSACTION_PROVIDER_DISSOLVE) with strict output/penalty rules. Filed under the alias name per DIP editor policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Replace 1051200 (~2 yr at 60 s) early-period ceiling with 420480 (~2 yr at nominal 2.5 min/block) and document wall-clock conversion. * Narrow the shared collateral covenant: consensus protects only active or same-block-pending v5 collateral outpoints, not every output whose scriptPubKey matches the template, so unrelated UTXOs with the same script are not retroactively locked. * Fix DistributeByWeight remainder distribution to skip zero-weight indices and require sum_w > 0, and add helper tests. * Allow the actor's dissolution output to be omitted when its value would be zero (e.g. when the fee consumes the full pre-fee remainder); reject stray zero-value actor outputs; specify output matching by walking the share table. * Special-case unanimous dissolution so DistributeByWeight is not invoked with an all-zero weight vector; bonus is set to zero directly. * Update test cases and penalty-bounds rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address compatibility/versioning review on the draft Shared Masternode DIP: * Bind SharedRegConsentHash and SharedDisHash to chainGenesisHash so consensus digests are network-scoped; signers MUST verify chain context before signing. Rewrite the cross-chain replay security consideration accordingly: proTxHash alone is not sufficient. * Clarify that ProTxVersion::SharedCollateral = 5 is the v5 ProRegTx payload version and the shared-MN state tag; the new ProDisTx, ProUpShareTx, and ProUpSharedRegTx payloads carry their own independent nVersion starting at 1. Reserve future provider payload versions (6+) for single-owner ProRegTx changes; v5 is not a generic mode flag. * Clean up ProUpRegTx/ProUpServTx wording: ProUpRegTx is invalid against a v5 masternode; ProUpServTx remains operator-authorized for service/operator-payout fields only and MUST NOT touch owner or shared-collateral fields. * Spell out the SML/filter trust boundary: SML hashes MUST NOT include v5-only fields, full nodes validate v5 data from reconstructed state, filter matches exist for client discovery only and are not consensus commitments. A future DIP-0004 extension would be required for SPV commitments. Mirrors the non-commitment direction taken for DIP-0026 payout metadata. * Mark the DIP explicitly non-final for activation until concrete SHARED_COLLATERAL_SCRIPT bytes are assigned and inserted, and strengthen the open issue requiring re-check of the tentative 10/11/12 special transaction type numbers against the authoritative DIP-0002 / Dash Core allocations before merge or activation, with mandatory renumbering on conflict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re-vector diffs Address consensus review blockers ahead of activation-final: * Deterministic MN state diffs: remove the MAY-compact alternative and require a single canonical encoding - full replacement of the share vector on any share-field change. * Same-block ProRegTx + ProDisTx: forbid dissolution in the same block as the v5 registration. The protected set is split into an active set (allows ProDisTx) and a same-block pending set (rejects every spend, including ProDisTx). Update covenant rules, rationale, implementation notes, and test cases accordingly. * Activation language: tighten draft-status wording so the DIP is reviewable but explicitly not activation-final until script bytes and DIP-0023 deployment parameters are assigned. No specific Dash Core release version is claimed.
thepastaclaw
force-pushed
the
docs/dip-decentralized-mn-shares
branch
from
1550555 to
c14a4cd
Compare
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
docs/dip-decentralized-mn-shares
branch
from
c14a4cd to
b64a580
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Draft DIP for decentralized masternode shares (ProTx v5 + ProDisTx/ProUpShareTx/ProUpSharedRegTx) is tight on the covenant model, digests, penalty bounds, same-block dissolution, and reorg replay. One blocking spec ambiguity remains: the covenant uses the undefined term "active v5 masternode" which could let PoSe-banned masternode collateral escape mempool/block-level rejection if implementations interpret "active" as the valid (non-banned) subset rather than the registered set.
🔴 1 blocking
🤖 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 `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:957-961: Define "active v5 masternode" to include PoSe-banned registrations
The active-set definition keys the mempool and block-level covenant rejection on "every collateral outpoint recorded against an active v5 masternode in the deterministic masternode list", but "active" is not defined. DIP-0003 distinguishes the registered set from the valid (non-PoSe-banned) subset, and an implementer reasonably reading "active" as "valid" would exclude PoSe-banned v5 masternodes from the active set. Mempool rule 1 (line 985) and block-connection rule 2 (line 994-1000) then would not reject an ordinary spend of a PoSe-banned v5 masternode's shared collateral, even though the later list-removal rule 4 (line 1009-1016) says any v5 masternode must only be removed by ProDisTx. That mismatch means a PoSe-banned masternode could be removed via collateral spend on one implementation and rejected on another — a consensus split. Same ambiguity affects the parallel summary at lines 264-269 and references to "active v5 masternode" at lines 1455, 1458, 1476. Specify that the protected active set is every registered v5 masternode in the deterministic list, including PoSe-banned ones, then propagate the wording consistently.
GitHub does not allow PastaClaw to request changes on their own PR, so this is posted as a COMMENT review while preserving the blocking finding.
Inline posting also hit GitHub HTTP 422, so I posted the same verified finding as a top-level review body.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #185 at f6bd78a. Prior b64a580 finding (active-vs-registered v5 masternode set, PoSe-banned membership) is FIXED at f6bd78a — the protected-set sections, collateral-spend enforcement, and Test 17 all now use 'registered' and explicitly include PoSe-banned entries until removal by a valid ProDisTx. One new latest-delta finding: the early-period prose at line 150 and the consensus formula at line 817 disagree on how many blocks the early penalty actually covers once same-block dissolution is forbidden, which leaks into consensus-binding ProDisTx output amounts. CodeRabbit posted only a processing summary with zero inline findings, so coderabbit_reactions is empty.
🔴 1 blocking
🤖 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 `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:817-818: Early-period formula and prose disagree on the size of the early window
The terminology entry at line 150 defines the early period as 'The window of `earlyPeriodBlocks` blocks after registration during which `earlyPenalty` applies.' The consensus formula at line 817 is `early = (H - state.nRegisteredHeight) < state.earlyPeriodBlocks`. However, same-block dissolution is explicitly forbidden (lines 925-936, 1302-1310, 1465-1471), so the first connectable `ProDisTx` is at `H = state.nRegisteredHeight + 1`. With the strict `<` test, the early-penalty regime then covers `earlyPeriodBlocks - 1` eligible blocks rather than `earlyPeriodBlocks`, and the edge case `earlyPeriodBlocks == 1` covers zero eligible blocks despite the prose promising a one-block window. Because `penalty` directly sets the consensus-required non-actor refund amounts and the actor's maximum output (lines 1421-1432), an implementer who follows the prose (`H - nRegisteredHeight <= earlyPeriodBlocks` or counts the natural N blocks after registration) will accept different `ProDisTx` outputs than one who follows the formula. Pick one semantics and align both the prose and the formula (and document the `earlyPeriodBlocks == 0` and `== 1` degenerate cases) before activation.
GitHub does not allow PastaClaw to request changes on their own PR, so this is posted as a COMMENT review while preserving the blocking finding.
Inline/request-changes posting hit GitHub HTTP 422, so I posted the same verified finding as a top-level review body.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The prior f6bd78a early-period prose/formula mismatch is FIXED at dcd170b: the terminology entry now defines the early period in terms of eligible dissolution blocks (excluding same-block dissolution and explicitly disabling the window when earlyPeriodBlocks == 0), and the selector at lines 817-819 is gated on earlyPeriodBlocks > 0 with <=, matching the prose. One carried-forward latest-delta-adjacent docs nit remains in the Reorg test list where 'in a different order' is ambiguous against the existence precondition for ProUpSharedRegTx. No new blocking issues.
🟡 1 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 `dip-decentralized-masternode-shares.md`:
- [SUGGESTION] dip-decentralized-masternode-shares.md:1498-1499: Reorg test #4 wording conflicts with the update-target-must-exist rule
Test 4 states that replaying a `ProUpSharedRegTx` followed by a `ProDisTx` 'in a different order' produces the same final state. Read literally, swapping the order places the `ProDisTx` first, which under the normative rules at lines 615-616 (the masternode identified by `proTxHash` MUST exist and MUST be a v5 masternode) and the dissolution-removes-the-entry semantics makes the later `ProUpSharedRegTx` invalid because the masternode no longer exists. The intent appears to be disconnect/reconnect determinism along the original chain order, not a branch with reversed transaction ordering. Tighten the wording so implementers do not infer that order-swapped reorg branches must converge.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (dcd170b..7c91aa5) is a single targeted wording fix that resolves the prior reorg-test #4 ordering contradiction by preserving original chain order. Cumulative draft remains under-specified in two places: the on-wire framing of the top-level share vector in the v5 ProRegTx payload, and the state-diff bit value for the share-vector field (neither assigned nor listed in Open Issues). Both are suggestion-grade improvements for a draft; no blocking defects.
🟡 2 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 `dip-decentralized-masternode-shares.md`:
- [SUGGESTION] dip-decentralized-masternode-shares.md:386: Specify on-wire framing for the share vector
The v5 ProRegTx layout claims to be exhaustive and normative for `nVersion == 5`, but the `shares` row only says `CollateralShare[]` without naming the vector's length prefix. Every other vector in this DIP — `refundScript`, `rewardScript`, `joinSig`, and `vchSigs` — explicitly calls out `compact-size length-prefixed`, and the registration consent digest separately uses `LE8(shares.size())`. A reader synthesising the payload framing from those two sources alone could plausibly serialize the outer count as either CompactSize or LE8. CompactSize is the Dash/Bitcoin convention for top-level vectors, but the asymmetry with neighbouring vector fields is the kind of ambiguity the spec elsewhere takes care to remove. Add an explicit `sharesSize` row (or an inline note that `CollateralShare[]` is a CompactSize-prefixed vector) so the byte boundary preceding `earlyPeriodBlocks` is unambiguous.
- [SUGGESTION] dip-decentralized-masternode-shares.md:1070-1079: Assign the share-vector state-diff bit or mark it as a pre-activation open item
This clause makes the share-vector diff encoding normative (full replacement is the single canonical representation), but it only says the state-diff bitfield 'gains a new field bit' without assigning the bit value. The state-diff bitfield is a consensus selector for how serialized deterministic-MN state diffs are decoded, so leaving the value unspecified prevents implementers from independently producing interoperable snapshots and reorg-replay payloads. The Open Issues section (line 1634) already enumerates other pre-activation deferred items (SHARED_MIN_SHARE_DUFFS, activation deployment name) — either assign the exact bit value here or add an explicit entry to Open Issues so the gap is tracked.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The 113b147 delta correctly fixes both prior 7c91aa5 findings: the v5 ProRegTx shares row at line 386 now specifies it as a CompactSize-prefixed vector (resolving the byte-framing ambiguity before earlyPeriodBlocks), and the new state-diff bit for the share vector is explicitly deferred at lines 1070-1071 with a matching pre-activation Open Issues entry at lines 1645-1646. No new in-scope spec defects remain; the draft is internally consistent with its DIP: TBD posture and the proposed DIP-0002 registry rows for types 10/11/12.
GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than APPROVE.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dip-decentralized-masternode-shares.md (1)
45-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the DIP-0026 version reference before this draft lands.
This section still hardcodes
DIP-0026 v4, but the checked-indip-0026.mdin this branch describes the multi-payout payload as version 2. Until the companion update is part of the same stack, readers have two conflicting version contracts and can’t tell which payload layout to implement.Also applies to: 115-123
🤖 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 `@dip-decentralized-masternode-shares.md` around lines 45 - 67, The document contains hardcoded references to DIP-0026 v4, but the actual dip-0026.md file describes the multi-payout payload as version 2, creating conflicting version contracts for readers. Locate all instances where DIP-0026 is referenced with a version number in dip-decentralized-masternode-shares.md (including the sections around lines 45-67 and 115-123 as indicated in the comment), verify the correct version from the dip-0026.md file in the branch, and update all version references to match consistently so readers have a single authoritative version contract to implement against.
🤖 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.
Duplicate comments:
In `@dip-decentralized-masternode-shares.md`:
- Around line 45-67: The document contains hardcoded references to DIP-0026 v4,
but the actual dip-0026.md file describes the multi-payout payload as version 2,
creating conflicting version contracts for readers. Locate all instances where
DIP-0026 is referenced with a version number in
dip-decentralized-masternode-shares.md (including the sections around lines
45-67 and 115-123 as indicated in the comment), verify the correct version from
the dip-0026.md file in the branch, and update all version references to match
consistently so readers have a single authoritative version contract to
implement against.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfe3be39-7f94-4c84-973d-02876210436c
📒 Files selected for processing (1)
dip-decentralized-masternode-shares.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta replaces a separate shared provider payload version with an isSharedCollateral discriminator embedded inside the existing DIP-0026 v4 ProRegTx payload. The prior CompactSize share-vector framing and deferred state-diff-bit concerns from 7c91aa5 remain resolved at 1b778bc. One blocking spec-correctness issue remains: the discriminator byte's position is normatively defined here but not reconciled with the pending DIP-0026 v4 wire layout, leaving the v4 framing ambiguous across shared vs non-shared parsers.
GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than REQUEST_CHANGES.
🔴 1 blocking
🤖 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 `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:223-244: Discriminator byte position must be reserved by DIP-0026 v4 or moved to its own payload version
This section makes `isSharedCollateral` a `uint8_t` placed in the v4 ProRegTx payload after `nMode` (Section 5.6 confirms the position at line 428-429), and then states that `isSharedCollateral == 0x00` payloads `follow the unchanged DIP-0026 multi-payout layout` (line 232-236) and `deserialize and validate exactly as DIP-0026 specifies` (line 1330-1333). DIP-0026 v4 is itself a pending/unmerged dependency (acknowledged at line 168 as the `pending DIP-0026 v4 design`), and nothing in this DIP or in the current repository copy of DIP-0026 records that DIP-0026 v4 reserves a discriminator byte at that position. As written, a shared-mode parser consumes one byte after `nMode` to select the variant, while a DIP-0026 v4 non-shared parser would treat that same byte as the first byte of `collateralOutpoint.hash`. The two parsers therefore frame the same v4 bytes differently, which shifts the rest of the payload for non-shared v4 registrations, invalidates payload hashes/signatures across implementations, and turns valid external-collateral hashes that happen to begin with a byte other than `0x00` into illegal discriminator values. Before activation, either DIP-0026 v4 must explicitly reserve this discriminator byte at the same wire offset for all v4 ProRegTx payloads (and that coordination must be called out here as an unresolved cross-DIP dependency in `Open Issues`), or shared collateral must be carried by a distinct provider payload version so the wire-format selector is unambiguous. The current `Open Issues` section does not list this coordination, and the rationale at line 1317-1333 argues against a separate version without addressing the framing prerequisite.
| The discriminator is a single `uint8_t` field in the v4 ProRegTx payload: | ||
|
|
||
| | Name | Type | Encoding | | ||
| | --- | --- | --- | | ||
| | `isSharedCollateral` | `uint8_t` | `0x00` for non-shared (DIP-0026 multi-payout) mode; `0x01` for shared-collateral mode. Any other value is invalid. | | ||
|
|
||
| The discriminator selects mutually exclusive variant fields in the same v4 | ||
| payload: | ||
|
|
||
| * `isSharedCollateral == false`: the v4 payload follows the unchanged | ||
| DIP-0026 multi-payout layout (single `keyIDOwner`, `scriptPayout` / | ||
| `payouts`, no share table, no shared collateral output, no penalty | ||
| parameters, no `earlyPeriodBlocks`). All DIP-0026 v4 semantics, validation, | ||
| state, and authorization apply verbatim and are unaffected by this DIP. | ||
| * `isSharedCollateral == true`: the v4 payload follows the shared-collateral | ||
| layout defined in [Shared Registration (ProRegTx v4, shared | ||
| mode)](#shared-registration-proregtx-v4-shared-mode) (no `keyIDOwner`, no | ||
| `scriptPayout`, no `payouts`, a `CollateralShare[]` table, an internal | ||
| collateral output that pays `SHARED_COLLATERAL_SCRIPT`, and per-masternode | ||
| penalty parameters). The shared-collateral variant is selected solely by | ||
| the discriminator; consensus MUST NOT infer the variant from any other | ||
| field length, script shape, or output count. |
There was a problem hiding this comment.
🔴 Blocking: Discriminator byte position must be reserved by DIP-0026 v4 or moved to its own payload version
This section makes isSharedCollateral a uint8_t placed in the v4 ProRegTx payload after nMode (Section 5.6 confirms the position at line 428-429), and then states that isSharedCollateral == 0x00 payloads follow the unchanged DIP-0026 multi-payout layout (line 232-236) and deserialize and validate exactly as DIP-0026 specifies (line 1330-1333). DIP-0026 v4 is itself a pending/unmerged dependency (acknowledged at line 168 as the pending DIP-0026 v4 design), and nothing in this DIP or in the current repository copy of DIP-0026 records that DIP-0026 v4 reserves a discriminator byte at that position. As written, a shared-mode parser consumes one byte after nMode to select the variant, while a DIP-0026 v4 non-shared parser would treat that same byte as the first byte of collateralOutpoint.hash. The two parsers therefore frame the same v4 bytes differently, which shifts the rest of the payload for non-shared v4 registrations, invalidates payload hashes/signatures across implementations, and turns valid external-collateral hashes that happen to begin with a byte other than 0x00 into illegal discriminator values. Before activation, either DIP-0026 v4 must explicitly reserve this discriminator byte at the same wire offset for all v4 ProRegTx payloads (and that coordination must be called out here as an unresolved cross-DIP dependency in Open Issues), or shared collateral must be carried by a distinct provider payload version so the wire-format selector is unambiguous. The current Open Issues section does not list this coordination, and the rationale at line 1317-1333 argues against a separate version without addressing the framing prerequisite.
source: ['codex']
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agent reviews and the prior discriminator/framing concern were reconciled against the current head 51ec7ac. The discriminator byte position is explicitly stated (line 224, immediately after nMode) and the cross-DIP dependency is now captured as Open Issue #3 (lines 1805-1809), which is acceptable for a draft per the repo review skill. No in-scope blocking, suggestion, or nitpick defects remain; previously deferred items (SHARED_MIN_SHARE_DUFFS, deployment name, share-vector state-diff bit) are still appropriately listed under Open Issues.
GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than APPROVE.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Adds a draft, unnumbered DIP for Decentralized Masternode Shares.
The draft builds on DIP-0003, DIP-0023, DIP-0026 v4, and DIP-0028 to specify shared-collateral masternode registration, per-share reward distribution, participant-authorized updates, and constrained dissolution.
Also adds:
TBDplaceholder until editor assignment.10—ProDisTx11—ProUpShareTx12—ProUpSharedRegTxDependency / review note
This PR is intentionally draft-blocked on #184, which updates DIP-0026 to the v4 provider-payload semantics this draft references. The shared-masternode branch has been reviewed against that DIP-0026 v4 base; once #184 lands, this PR should become repository-consistent without bundling the DIP-0026 changes here.
Open design feedback to resolve before ready-for-review: whether shared-collateral semantics should remain a separate provider payload version (
SharedCollateral = 5, as currently drafted) or be folded into a single version that supports both shared and non-shared masternodes via an explicit flag or share-count discriminator.Notes for editors/reviewers
DIP: TBDand uses the alias filenamedip-decentralized-masternode-shares.md; authors must not self-assign DIP numbers.10,11, and12are registered here as proposed values for this draft. If any value is consumed before merge, this DIP must be updated to the next free values before merge.SHARED_MIN_SHARE_DUFFS, remain explicitly open before activation.Validation
git diff --checknpx cspell --no-progress README.md dip-decentralized-masternode-shares.md dip-0002/special-transactions.mdnpx markdownlint-cli2 README.md dip-decentralized-masternode-shares.md dip-0002/special-transactions.md --config .markdownlint.jsonfix/dip26-review-followup...docs/dip-decentralized-mn-shares):shipSummary by CodeRabbit