Repository navigation
Conversation
… same operator key Once DEPLOYMENT_V24 is active, Dash Core re-serializes a legacy-scheme operator key in the basic scheme when a masternode's state version is raised, and protx listdiff reports pubKeyOperator as changed. Platform created a second operator identity for the same key and disabled the BLS and node-id keys of the existing one. update_state_masternode_list v0 and v1 now clear pub_key_operator from a diff entry whose bytes are the basic encoding of the stored legacy key, or equal the stored bytes, while V24 is active at the block's Core height. The gate reads the chain-locked block's coinbase payload version (4 exactly where V24 and V20 are active) through the new CoreRPCLike::get_coinbase_payload_version. The list keeps the bytes the operator identity id is derived from, so the identity, its keys and its balance stay, and later changes and removals keep reaching it. Before V24 every reported key is applied exactly as before. v0 is selected by protocol versions 1 to 13 and is changed in place on purpose: no block on mainnet or testnet has V24 active, so executed blocks replay unchanged. Test would have caught this: on the code before the fix should_keep_the_operator_identity_of_a_legacy_key_core_re_encodes, should_keep_the_operator_identity_for_a_negated_key_once_v24_is_active and should_reach_the_kept_operator_identity_after_a_re_encode_and_a_restart failed (re-encoded bytes stored, second identity created); after it they pass. The fixtures are protx listdiff output recorded from Dash Core v24.0.0-rc.3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit details
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 |
|
⛔ Final review complete — 1 blocking finding(s) (commit 026d9eb) · triage: critical |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 026d9eb and consolidated the overlapping reports into one blocking consensus-activation issue. The Core-height gate preserves pre-V24 behavior, but the normalization changes a shipped Platform generation without a Platform protocol activation boundary. Validation was static only; Rust workspace tests were still running in the supplied CI snapshot from 2026-10-15T16:29:45Z.
🔴 1 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 11: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate drop_unchanged_operator_keys logic in update_state_masternode_list/mod.rs changes consensus-visible operator key handling, identity continuity, and persisted masternode state, including an activation-gated modification to shipped v0 behavior. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 90% left, 5h 65% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:130-138: Activate operator-key normalization through a new Platform generation
This call changes consensus behavior for shipped protocol versions 1–13, whose method tables still select `update_masternode_list = 0`. With protocol 13 active and a V24-active Core height, the previous implementation applies the re-encoded bytes, derives another operator identity, and disables the previous identity's operator and node-id keys. This implementation clears that key change before both list mutation and identity processing, retaining the original identity and producing different committed state. The new fixture test explicitly exercises retention at protocol 13. The PR's evidence that neither network has reached the triggering Core activation establishes historical replay compatibility, but not consensus equivalence for the shipped generation. Requiring every executing node to upgrade before Core V24 is an external rollout requirement, not the PlatformVersion boundary required by `book/src/contributing/coding-conventions.md`; mixed binaries can disagree under the same active Platform protocol. Restore v0's behavior and confine normalization to an unreleased `update_masternode_list` generation selected through the protocol-14 method tables. Keep coverage showing protocol 13 retains the original identity-switch behavior even with V24-active input, while the unreleased version preserves the identity, and coordinate that Platform activation before Core V24.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Complete legacy-encoding operator identity lookup in the wallet —
tracked.rsderives a single operator identity ID from the current listed key, andlocator.rs::platform_roles_from_identityrecognizes only that same derivation. When this patch retains a legacy-derived identity while Core lists basic-encoded bytes, wallet refresh can miss that identity and role recognition can miss its operator-payout role. This is a concrete client integration consequence, explicitly assigned to the separate wallet change #5462 rather than this server-side PR.- Follow-up: Address in #5462 with shared candidate-ID handling and tests that discover and recognize a legacy-derived identity from a basic-encoded Core listing.
- Preserve historical operator bytes during state-sync reconstruction — The incremental update now retains historical operator bytes, whereas the from-scratch path skips normalization and accepts Core's current bytes. Reconstructing the list solely from a current Core listing can therefore produce an operator identity reference inconsistent with the restored identity tree. Ordinary persistence is covered here; reconstruction is a concrete integration requirement for the separate state-sync work in #4648.
- Follow-up: Track in #4648 and validate reconstructed state with subsequent operator-payout changes, genuine key rotation, and masternode removal.
| if !start_from_scratch { | ||
| // Protocol versions 1 to 13 select this generation. The call changes no entry unless | ||
| // DEPLOYMENT_V24 is active at `core_block_height`, so blocks at earlier Core heights | ||
| // execute unchanged. | ||
| self.drop_unchanged_operator_keys( | ||
| state.full_masternode_list(), | ||
| core_block_height, | ||
| &mut masternode_diff.updated_mns, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Activate operator-key normalization through a new Platform generation
This call changes consensus behavior for shipped protocol versions 1–13, whose method tables still select update_masternode_list = 0. With protocol 13 active and a V24-active Core height, the previous implementation applies the re-encoded bytes, derives another operator identity, and disables the previous identity's operator and node-id keys. This implementation clears that key change before both list mutation and identity processing, retaining the original identity and producing different committed state. The new fixture test explicitly exercises retention at protocol 13. The PR's evidence that neither network has reached the triggering Core activation establishes historical replay compatibility, but not consensus equivalence for the shipped generation. Requiring every executing node to upgrade before Core V24 is an external rollout requirement, not the PlatformVersion boundary required by book/src/contributing/coding-conventions.md; mixed binaries can disagree under the same active Platform protocol. Restore v0's behavior and confine normalization to an unreleased update_masternode_list generation selected through the protocol-14 method tables. Keep coverage showing protocol 13 retains the original identity-switch behavior even with V24-active input, while the unreleased version preserves the identity, and coordinate that Platform activation before Core V24.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
This branch has not been deployed
2 participants
Issue being fixed or feature implemented
Once Dash Core's
DEPLOYMENT_V24is active, the first ProUpServTx or ProUpRegTx of a masternode raises its state version. If the state still holds its operator key in the legacy BLS scheme, Core re-serializes the same key in the basic scheme. Only the 48 bytes change, butprotx listdiffreportspubKeyOperatoras changed.Platform treated that as a new operator. It derived a new operator identity id from the new bytes (
sha256(proTxHash ‖ key bytes)), created that identity, and disabled the BLS and node-id keys of the existing one. The existing identity kept its payout key and balance. Every masternode registered with a legacy-scheme operator key would get a second, empty operator identity the first time it updates after V24.Nothing breaks or halts, but the operator identity, its keys and its balance end up split across two identities. V24 is not active on mainnet or testnet, so this has not happened on either network.
What was done?
update_state_masternode_listv0 and v1 now call a shared helper,drop_unchanged_operator_keys, right after fetching Core's diff (not when the list is built from scratch). It clearspub_key_operatorfrom an updated entry when the reported bytes:and only while V24 is active at the block's Core height.
What this does downstream:
V24 gate. It reads the coinbase payload version of the chain-locked block at
core_block_height. Core requires version 4 exactly at the blocks where V24 and V20 are active, and V20 is active on every chain Platform runs on.CoreRPCLike::get_coinbase_payload_version, which shares thegetblockhash+getspecialtxesfetch ofget_credit_pool_balance. Drive already probes that fetch at startup.Before V24 every reported key is applied exactly as before, so blocks already executed replay to the same result.
Known ambiguity, accepted. The bytes do not say which scheme a stored key is in. A basic-scheme key with its sign bit clear also decodes in the legacy scheme, to its negation. So once V24 is active, an owner changing such a key to its negation keeps the operator identity. Whoever holds the secret of one key holds the secret of the other.
The stored operator key now depends on the list's history. A list rebuilt from Core's full listing holds Core's current bytes instead. Any future rebuild, such as state sync (feat(drive-abci)!: state sync via ABCI snapshots with reduced platform state (protocol v15) #4648), has to recover the stored bytes.
Clients that derive the operator identity id from the key bytes Core currently lists should also try the legacy encoding of the same key. The wallet change is fix(platform-wallet): find a masternode's operator identity after Core re-encodes its operator key #5462.
In-place changes to shipped generations
update_state_masternode_listv0 (update_masternode_list0)drop_unchanged_operator_keysupdate_state_masternode_listv1 (update_masternode_list1)The v0 edit is a deliberate exception to the rule for shipped generations, not an inert change. It changes protocol version 13 consensus for one input: a block whose Core height has V24 active. No such block exists on mainnet or testnet:
DEPLOYMENT_V24isNEVER_ACTIVEin Dash Core v24's chainparams.So every block either network has executed replays unchanged. Before V24 the helper changes no entry, whatever Core reports; the only addition is a Core call when an entry would be cleared. Every other versioned method (
update_operator_identityv0,update_masternode_identitiesv0/v1, the identity id derivations) is unchanged.Deployment rule (for the rc.4 notes)
Every node that executes Platform blocks must run this release (5.0.0-rc.4 or later) before
DEPLOYMENT_V24activates on its network's Core. That includes validators, other evonodes and full/DAPI nodes. A node without this change computes a different app hash at the first block where Core re-encodes an operator key. Until V24 activates, old and new binaries agree.How Has This Been Tested?
The fixtures are real Core output:
protx listdiffrecorded from the official Dash Core v24.0.0-rc.3 binary on regtest (tests/supporting_files/core_v24_protx_list_diffs/legacy_operator_key/).New tests, each at protocol versions 13 and 14 through
update_masternode_list, as block execution calls it:should_keep_the_operator_identity_of_a_legacy_key_core_re_encodes, with the recorded steps one per Core height and as one diff. Checks:should_switch_the_operator_identity_on_a_re_encoded_key_while_v24_is_not_active: the same recorded diff with coinbase version 3 behaves as before.should_reach_the_kept_operator_identity_after_a_re_encode_and_a_restart: a later key change and a removal reach the kept identity. A store/reload of the platform state after the re-encode ends with the same GroveDB root hash.Red → green:
Before the fix, 3 of the new tests failed:
should_keep_the_operator_identity_of_a_legacy_key_core_re_encodes;should_keep_the_operator_identity_for_a_negated_key_once_v24_is_active;should_reach_the_kept_operator_identity_after_a_re_encode_and_a_restart.They failed because the re-encoded bytes were stored and a second identity was created.
After the fix they pass.
The gate tests cannot fail on the old code by design. Removing the gate from the helper turns 3 of them red, and making the helper clear every keyed entry turns 7 tests red.
Also run:
cargo test -p drive-abci --lib: 4279 passed. That run predates the review follow-ups; the masternode list, identity and RPC tests (116) pass on the final code.cargo test -p drive-abci --test strategy_testswithRUST_MIN_STACK=4194304, as in CI.cargo clippy -p drive-abci --all-targets --features mocks -- -D warnings.Multi-block behaviour is tested at the dispatcher level, not with the strategy harness. The dispatcher is the exact call
run_block_proposalmakes, and it can run real Core output; the harness would only produce synthetic diffs.Breaking Changes
Consensus: at protocol versions 13 and 14, a block whose Core height has
DEPLOYMENT_V24active computes different masternode identities than before when Core re-encodes an operator key. See the deployment rule above. There are no API or storage format changes, apart from a new required method on theCoreRPCLiketrait.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
026d9eb/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.