Skip to content

fix(drive-abci): apply masternode list diffs as v0 and v1 always have - #5455

Open
shumkov wants to merge 1 commit into
v5.1-devfrom
shumkov/core51-c5-mn-diff
Open

shumkov wants to merge 1 commit into
v5.1-devfrom
shumkov/core51-c5-mn-diff

Conversation

@shumkov

@shumkov shumkov commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Part of #5440. dashpay/rust-dashcore#1179 changes how the Core RPC client reads protx listdiff:

  • it decodes an updated masternode's operatorPayoutAddress, which it used to drop;
  • DMNState::apply_diff keeps PoSeRevivedHeight when a diff does not print it, where it used to clear it.

Every protocol version so far (1-15, through update_state_masternode_list v0 and v1) processed its blocks the old way:

  • operator payouts came only from full masternode states;
  • a revived masternode's next diff cleared its revived height.

The stored masternode lists, the operator identities' TRANSFER keys and the validator sets of every processed block follow from that. Taking the new client behaviour as it is would change those at historical heights: replay would diverge, and old and new binaries would disagree. This PR keeps v0 and v1 exactly as they were. It is safe to merge before the rust-dashcore pin bump, and it must be merged before or together with it.

What was done?

  • Operator payouts: without_operator_payout_changes drops them from every updated masternode right after get_protx_diff_with_masternodes in update_state_masternode_list v0 and v1. That is the only place a decoded diff enters Platform, so neither the stored list nor update_masternode_identities sees them. Full states (added_mns) are untouched; they always carried the payout.

  • Revived height: apply_diff_replacing_revived_height applies a diff with the revived height replaced by the diff's. Every production apply of a Core diff in v0 and v1 goes through it:

    • v0's early-return check;
    • the PlatformState accessor apply_masternode_state_diff, used only by v0, for both lists;
    • v1's stored-form apply.

    It is re-imposed in Platform, not left to the client, because of v0's full path. On that path, validator-set members are refreshed from the fields a diff prints, so whether a block takes it can reach the validator sets.

In-place changes to shipped generations

Generation Selected by Change Why consensus cannot change
update_state_masternode_list v0 update_masternode_list 0, protocol versions 1-13 Strip updated masternodes' operator payouts after the fetch. Apply diffs (early-return check, and through apply_masternode_state_diff) with the revived height replaced by the diff's. With the current pin both are no-ops: the client decodes no diff payout, and apply_diff already replaces the revived height. After the bump they give v0 the same decoded fields and the same state transitions as before.
update_state_masternode_list v1 update_masternode_list 1, protocol versions 14-15 Same strip. apply_diff_in_stored_form uses the helper. Same.

One input does change with the bump, at every version: a malformed non-empty operatorPayoutAddress now fails the whole diff instead of being ignored. Core cannot print one. It prints the field only for a destination it extracts from the payout script, and ProUpServTx validation limits that script to P2PKH or P2SH. That is the same parser path as payoutAddress, which already fails on malformed input.

No version table changes and no new generation.

How Has This Been Tested?

Tests run at protocol version 13 (v0) and latest() (v1), through the update_masternode_list dispatcher:

  • should_keep_operator_payouts_and_keys_when_a_diff_carries_an_operator_payout. Three diffs: one sets an operator payout, one clears it (Some(None)), and one carries a payout with a new operator key. Afterwards the operator identities' keys and the stored payouts are as they were, and the new operator's identity gets the stored payout, not the diff's. Before the strip it fails at 13: the old TRANSFER key is disabled and a new one added.
  • should_clear_a_revived_height_a_diff_does_not_print. A revived evonode's payment-only diff, alone in its block, clears the revived height in both masternode lists, and a revive diff sets it.
    • It passes with the current pin.
    • With the pin bumped to rust-dashcore#1179 (scratch worktree), it fails without the re-imposed clearing (Some(900) kept). It also fails when only v0's early-return check uses the client's apply_diff. With this PR it passes.
  • Unit tests of apply_diff_replacing_revived_height.

Suites run:

  • Current pin: cargo test -p drive-abci --lib core_based_updates (92), the update_masternode_list tests, and cargo clippy -p drive-abci --all-targets -- -D warnings.
  • Pin bumped to ci: cancel runs for already merged PRs #1179: the core_based_updates, platform_state and masternode drive-abci tests (222) pass.
  • The masternode strategy tests pass.

Breaking Changes

None. Consensus is unchanged at every protocol version, with the current rust-dashcore pin and after bumping it to dashpay/rust-dashcore#1179.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 04d6f00

  • Bots — coderabbitai not yet · thepastaclaw ✓ — /skip-bots proceeds without the ones not yet reported
  • Self-review — post /self-reviewed
  • Build green
  • Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

…ys have

The Core RPC client is changing how it reads `protx listdiff`
(dashpay/rust-dashcore#1179): it starts decoding an updated masternode's
`operatorPayoutAddress`, and `DMNState::apply_diff` stops clearing
`PoSeRevivedHeight` when a diff does not print it. Every protocol
version so far (1-15, through update_state_masternode_list v0 and v1)
processed its blocks the old way: operator payouts came only from full
masternode states, and a revived masternode's next diff cleared its
revived height. The stored masternode lists, the operator identities'
TRANSFER keys and the validator sets of every processed block follow
from that, so v0 and v1 keep it:

- the operator payouts of updated masternodes are dropped right after
  the fetch, the only place a decoded diff enters Platform, so neither
  the stored list nor the identity updates see them;
- every diff is applied with the revived height replaced by the diff's
  (`apply_diff_replacing_revived_height`, used by v0's early-return
  check, the state accessor only v0 uses, and v1's stored-form apply).

Both are in place, without a new generation: with the current pin they
change nothing, and after the bump v0 and v1 see the same decoded fields
and make the same state transitions as before. The revived height is
re-imposed rather than left to the client because in v0 whether a block
takes the full path decides whether validator-set members are refreshed
from the fields a diff prints, so it can reach the validator sets. One
input does change with the bump: a malformed non-empty
`operatorPayoutAddress` now fails the whole diff instead of being
ignored. Core cannot print one: it prints the field only for a
destination it extracts from the payout script, which ProUpServTx
validation limits to P2PKH or P2SH.

Tests, at protocol versions 13 and latest:
- a diff that sets an operator payout, clears one, or carries one with
  a new operator key leaves the operator identities' keys and the stored
  payouts as they were, and the new operator's identity gets the stored
  payout. Before the strip it fails at 13 (the old TRANSFER key is
  disabled and a new one added); after it passes.
- a revived evonode's payment-only diff, alone in its block, clears the
  revived height in both masternode lists, and a revive diff sets it.
  It passes with the current pin. With the pin bumped to #1179 it fails
  without the re-imposed clearing (`Some(900)` kept) and when only v0's
  early-return check applies diffs the client's way, and passes with
  this commit; the core_based_updates, platform_state and masternode
  drive-abci tests (222) pass there too.
- unit tests of the helper: an unprinted revived height is cleared, a
  printed one is taken.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shumkov shumkov added this to the v5.1.0 milestone Oct 15, 2026
@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 65a171de-70f1-4073-8571-3e6fcc9c2398

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 15, 2026
@thepastaclaw

thepastaclaw commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 04d6f00) · triage: normal

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

Verified the complete diff at head 04d6f00 against the pinned RPC implementation, version tables, production callers, and regression tests; no in-scope defects were identified. The compatibility helpers preserve historical operator-payout and revived-height behavior across both masternode-list generations, including change detection, stored lists, and downstream identity updates, supporting the documented exception for output-identical edits to shipped generations. This was a static review without local builds or test execution; the supplied CI snapshot reports successful Rust workspace tests, with PR Hygiene pending.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: normal by gpt-6.1-sol (effort low) — The production diff is a contained compatibility fix in masternode diff application across v0 and v1, with most added lines devoted to regression tests, so it does not meet the large-or-intricate threshold for critical despite touching consensus-sensitive state.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

@github-actions

Copy link
Copy Markdown
Contributor

@coderabbitai review

No review for 04d6f00c yet, so PR Hygiene is asking once. If nothing arrives, the requirement is dropped for this commit and the pull request is labelled bot-review-skipped.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-bots Waiting for the review bots to report on this head

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants