docs: add decentralized masternode shares draft DIP#185
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 b64a580) |
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
| | `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.
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):ship