Skip to content

fix(dash-spv): read rotated quarter ChainLocks from coinbases - #1293

Open
PastaPastaPasta wants to merge 3 commits into
devfrom
pasta/spv-rotated-coinbase-chainlocks
Open

PastaPastaPasta wants to merge 3 commits into
devfrom
pasta/spv-rotated-coinbase-chainlocks

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Nov 2, 2026 •

Copy link
Copy Markdown
Member

dash-spv selected each quarter of a rotated quorum with a ChainLock signature taken from the quorumsCLSigs groups of the QRInfo's diffs. No coinbase commits to quorumsCLSigs, so a QRInfo could carry any value there, and a different signature selected different members and made a valid rotated commitment fail. Dash Core builds each quarter's modifier from the coinbase ChainLock of that quarter's work block.

This PR reads each quarter's ChainLock from the coinbase the engine recorded for the work block's list, as dash-spv already does for non-rotating quorums. A quarter whose list is held without a recorded ChainLock is skipped like a missing list. The inference that assigned quorumsCLSigs groups to work blocks is removed. Nothing produces InferredRotationChainLockSigs or MissingRotationChainLockSigs any more; both stay so persisted statuses still decode.

Part of #1262.

🤖 Generated with Claude Code

PR Hygiene · 0757bed

  • Bots — coderabbitai ✓
  • Self-review — posted; again after any push
  • Build green
  • Approvals
    • dash-spv (dash-spv/src/sml_engine.rs, dash-spv/src/sml_engine/rotated_quorum_construction.rs, dash-spv/src/sml_engine/validation.rs) — QuantumExplorer or ZocoLini or xdustinface
    • files with no dedicated owner (dash/src/sml/llmq_entry_verification.rs, dash/src/sml/quorum_validation_error.rs, dash/tests/data/test_DML_diffs/block_container_597.dat and 1 more) — QuantumExplorer or ZocoLini or xdustinface
    • xdustinface requested changes — waiting for them to re-review or dismiss

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.

Summary by CodeRabbit

  • Bug Fixes
    • Rotated-quorum validation now uses ChainLocks recorded with the relevant work blocks for current and previous cycles.
    • Quarter signatures are read from the corresponding coinbase records; an all-zero signature is used when a record has no ChainLock.
    • Quarter signatures are unavailable when any required quarter list or ChainLock record is missing.
    • Quorum reconstruction reports missing ChainLocks, while successful reconstructions remain reusable for related quorums.
    • Validation no longer relies on inferred signatures from signature groups.

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Rotated-quorum verification and cycle reconstruction now use ChainLocks recorded for work blocks instead of QRInfo-derived signature assignments. The engine requires quarter lists and matching ChainLock records. The change removes inferred-signature failure handling and updates related tests and status documentation.

Changes: Rotated quorum ChainLocks

Layer / File(s) Summary
Quarter ChainLocks in quorum verification
dash-spv/src/sml_engine.rs
Previous- and current-cycle validation obtains quarter signatures from held-list ChainLock records. A quarter is unavailable if its list or matching record is missing; a recorded absence uses an all-zero signature. The engine removes QRInfo signature mapping and inferred-signature settlement handling. Tests cover altered quorumsCLSigs values and unavailable quarter lists.
Cycle reconstruction from recorded ChainLocks
dash-spv/src/sml_engine/rotated_quorum_construction.rs
Cycle reconstruction uses work-block ChainLocks rather than signatures on quorum entries. It returns RequiredChainLockNotPresent when a required lock is missing. The test checks resolution after the lock is recorded.
Work-block validation and persisted statuses
dash-spv/src/sml_engine/validation.rs, dash/src/sml/llmq_entry_verification.rs, dash/src/sml/quorum_validation_error.rs
Quorum validation passes the work-block ChainLock directly to the modifier constructor. Status comments state that the documented variants remain available for decoding but are not produced.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant QRInfo
  participant MasternodeListEngine
  participant quarter_sigs
  participant QuorumValidation
  QRInfo->>MasternodeListEngine: apply diffs and lists
  MasternodeListEngine->>quarter_sigs: look up quarter lists and ChainLock records
  quarter_sigs->>QuorumValidation: provide quarter signatures
  QuorumValidation->>MasternodeListEngine: return quorum status
Loading

Possibly related issues

  • dashpay/rust-dashcore#1262: This PR uses work-block coinbase ChainLocks instead of quorumsCLSigs as the rotated quarter selection source.
  • dashpay/rust-dashcore#1264: This PR changes the quarter ChainLock source used by settle_rotated_quorums; the issue's proposed list comparison is not included.
  • dashpay/rust-dashcore#1302: This PR covers the unavailable-list straggler behavior, but does not add list fetching or later revalidation.

Possibly related PRs

  • dashpay/rust-dashcore#1284: Both PRs use work-block coinbase ChainLocks in quorum validation; this PR applies that source to rotated quarter signatures.
Merge Risk: Minimal · up to 0757b

Rotated quorum verification now takes ChainLocks from the work blocks' coinbases. No actionable merge-blocking risk remains in the supplied evidence.

Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title "fix(dash-spv): read rotated quarter ChainLocks from coinbases" names the main change. The PR stops deriving rotated quarter ChainLocks from quorumsCLSigs groups and instead reads each quart…
Full details: Docstring Coverage

Explanation

Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 12 functions across 5 files.

Full details: Title check

Explanation

The title "fix(dash-spv): read rotated quarter ChainLocks from coinbases" names the main change. The PR stops deriving rotated quarter ChainLocks from quorumsCLSigs groups and instead reads each quarter's work-block coinbase ChainLock. The title matches the changeset and the PR objectives, which give the same primary intent. It is short, single-sentence, and uses a conventional commit prefix with the affected crate.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • 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

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 817a7a6 and 0757bed.

Files ignored due to path filters: 2

  • dash/tests/data/test_DML_diffs/block_container_597.dat is excluded by !**/*.dat
  • dash/tests/data/test_DML_diffs/qrinfo_597.dat is excluded by !**/*.dat
Files selected for processing: 5

  • dash-spv/src/sml_engine.rs
  • dash-spv/src/sml_engine/rotated_quorum_construction.rs
  • dash-spv/src/sml_engine/validation.rs
  • dash/src/sml/llmq_entry_verification.rs
  • dash/src/sml/quorum_validation_error.rs
Files skipped from review as they are similar to previous changes: 2

  • dash/src/sml/quorum_validation_error.rs
  • dash/src/sml/llmq_entry_verification.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. 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 Nov 2, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Nov 2, 2026
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

/self-reviewed c193b5d

@github-actions github-actions Bot removed the waiting-self-review Waiting for the author to post /self-reviewed label Nov 2, 2026
@codecov

codecov Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.34%. Comparing base (451c6cc) to head (0757bed).

Files with missing lines Patch % Lines
...-spv/src/sml_engine/rotated_quorum_construction.rs 97.43% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1293      +/-   ##
==========================================
- Coverage   85.35%   85.34%   -0.02%     
==========================================
  Files         268      268              
  Lines       68948    68732     -216     
==========================================
- Hits        58853    58656     -197     
+ Misses      10095    10076      -19     
Flag Coverage Δ
core 82.42% <ø> (ø)
rpc 52.10% <ø> (ø)
spv 94.25% <99.33%> (+0.02%) ⬆️
wallet 83.39% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/sml_engine.rs 97.51% <100.00%> (+0.02%) ⬆️
dash-spv/src/sml_engine/validation.rs 94.54% <100.00%> (ø)
dash/src/sml/llmq_entry_verification.rs 86.55% <ø> (ø)
dash/src/sml/quorum_validation_error.rs 0.00% <ø> (ø)
...-spv/src/sml_engine/rotated_quorum_construction.rs 94.32% <97.43%> (+0.28%) ⬆️

... and 5 files with indirect coverage changes

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Ready for review — dash-spv: QuantumExplorer or ZocoLini or xdustinface · files with no dedicated owner: QuantumExplorer or ZocoLini or xdustinface.
Full checklist in the description.

@github-actions github-actions Bot added the ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. label Nov 2, 2026

@xdustinface xdustinface 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.

  1. MissingRotationChainLockSigs is no longer produced, but its doc still describes a trigger. The only code that built RequiredRotatedChainLockSigsNotPresent was the Rotating guard in find_rotated_masternodes_for_quorums (rotated_quorum_construction.rs:65 on dev), which this PR removes. After the PR the variant only shows up in the From<QuorumValidationError> arm and its test row. That arm is the only producer of MissingRotationChainLockSigs, so neither one can come out of a fresh validation anymore. The PR trims the MissingRotationChainLockSigs doc in llmq_entry_verification.rs down to "The quorum entry came through without an attached VerifyingChainLockSignaturesType::Rotating", which reads as a live condition. The same diff marks InferredRotationChainLockSigs as no longer produced. Please give MissingRotationChainLockSigs the same "No longer produced, kept so persisted statuses still decode" doc, and add a matching note on QuorumValidationError::RequiredRotatedChainLockSigsNotPresent. Keep both variants where they are: the error is persisted too, inside LLMQEntryVerificationStatus::Invalid, and bincode encodes it by variant position, so deleting it would break decoding of existing snapshots while snapshot_codec::VERSION stays at 1.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: address xdustinface requested changes, then post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. labels Nov 3, 2026
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

Done in 34c627a. MissingRotationChainLockSigs and QuorumValidationError::RequiredRotatedChainLockSigsNotPresent are now documented as no longer produced and kept so persisted statuses still decode. Both stay in place, so the bincode positions and snapshot_codec::VERSION 1 are unchanged.


🤖 Posted autonomously by Claude on behalf of pasta.

@PastaPastaPasta

Copy link
Copy Markdown
Member Author

/self-reviewed 34c627a

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. 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 waiting-bots Waiting for the review bots to report on this head bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-self-review Waiting for the author to post /self-reviewed waiting-bots Waiting for the review bots to report on this head labels Nov 3, 2026
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

@coderabbitai review


🤖 Posted autonomously by Claude on behalf of pasta.

@github-actions github-actions Bot added the ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. label Nov 3, 2026
@github-actions github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

@PastaPastaPasta

Copy link
Copy Markdown
Member Author

/self-reviewed 817a7a6

@github-actions github-actions Bot removed the merge-conflict The PR conflicts with the target branch. label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. 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 waiting-bots Waiting for the review bots to report on this head and removed ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. labels Nov 3, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Nov 3, 2026
@github-actions github-actions Bot removed the waiting-bots Waiting for the review bots to report on this head label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Ready for review — dash-spv: QuantumExplorer or ZocoLini or xdustinface · files with no dedicated owner: QuantumExplorer or ZocoLini or xdustinface · re-review or resolve: xdustinface.
Full checklist in the description.

@github-actions github-actions Bot added the ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. label Nov 3, 2026
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

@xdustinface Thanks, addressed in 34c627a. MissingRotationChainLockSigs and RequiredRotatedChainLockSigsNotPresent are now documented as no longer produced by validation, kept only so persisted statuses still decode. The head is 817a7a6 (merged with dev on 11-03; it still merges cleanly with today's dev), and every CI check passes there. Could you take another look?


🤖 Posted autonomously by Claude on behalf of pasta.

PastaPastaPasta and others added 3 commits November 7, 2026 18:24
A quorum modifier is built from the work block's coinbase ChainLock, or from an all-zero signature when the coinbase carries none. `validate_quorum` spelled that fallback out inline. `work_block_chain_lock` returns it for a recorded work block, so the rotated quarters can share it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Each quarter of a rotated quorum selects its members under a modifier Dash Core builds from the coinbase ChainLock of that quarter's work block. dash-spv took these signatures from the `quorumsCLSigs` groups the QRInfo's diffs serve, which no coinbase commits to, and assigned groups to work blocks by elimination when their quorum heights were unknown. A served signature that differed from the coinbase's made a valid commitment fail.

`quarter_members` reads the coinbase ChainLock the engine recorded for the work block's list, as `validate_quorum` does for a non-rotating quorum. A quarter whose list is held without a recorded ChainLock fails with `RequiredChainLockNotPresent`, so the quorum is skipped as for a missing list. `find_rotated_masternodes_for_quorums` no longer needs signatures on the entry. The inference of quarter signatures and its `Skipped(InferredRotationChainLockSigs)` status are removed. Stored entries still carry their quarter ChainLocks, taken from the recorded coinbases.

Tests: a rotated active set verifies and is stored the same whatever `quorumsCLSigs` serves, and a dashd v23.1.8 regtest QRInfo whose straggler is skipped for a missing list.

Part of #1262.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
`MissingRotationChainLockSigs`, `InferredRotationChainLockSigs` and `QuorumValidationError::RequiredRotatedChainLockSigsNotPresent` are not produced any more. All three stay so persisted statuses still decode.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@PastaPastaPasta
PastaPastaPasta force-pushed the pasta/spv-rotated-coinbase-chainlocks branch from 817a7a6 to 0757bed Compare November 7, 2026 19:11
@PastaPastaPasta PastaPastaPasta changed the title fix(dash-spv): take rotated quarter ChainLocks from the work blocks' coinbases fix(dash-spv): read rotated quarter ChainLocks from coinbases Nov 7, 2026
@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. 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 waiting-bots Waiting for the review bots to report on this head and removed ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. labels Nov 7, 2026
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

/self-reviewed 0757bed

@github-actions github-actions Bot removed the waiting-bots Waiting for the review bots to report on this head label Nov 7, 2026
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Ready for review — dash-spv: QuantumExplorer or ZocoLini or xdustinface · files with no dedicated owner: QuantumExplorer or ZocoLini or xdustinface · re-review or resolve: xdustinface.
Full checklist in the description.

@github-actions github-actions Bot added the ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. label Nov 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants