[PastaPastaPasta]

dash-spv selects a non-rotating quorum's members using the ChainLock signature that a diff serves in quorumsCLSigs. No coinbase commits to that value. Since #1200 the commitments themselves are proven against their block, but quorumsCLSigs is not. If a diff serves a different signature there, or an all-zero one in place of a real one, dash-spv selects other members. A commitment that is valid on chain then fails its signature check and is stored Invalid, and ChainLocks from that quorum are refused.

Dash Core takes the signature from the work block's own coinbase (cites are to Dash Core develop at 15db47875):

• GetHashModifierFromWorkBlock builds the modifier from (llmqType, workBlock.nHeight, bestCLSignature) when the work block's coinbase carries a ChainLock, and from (llmqType, workBlock.hash) otherwise (src/llmq/utils.cpp:87-97).
• GetNonNullCoinbaseChainlock counts the coinbase ChainLock only if four checks pass: the payload is version 3 or later, its height is the block's, bestCLSignature.IsValid() holds, and bestCLHeightDiff is below the height (src/evo/cbtx.cpp:200-211). IsValid() is false for all-zero bytes, for bytes the parser refuses, and for the identity element (src/bls/bls.h:90-122).
• quorumsCLSigs repeats that value for each new quorum, with an all-zero signature when there is none (src/evo/smldiff.cpp:87-102).

This PR does the same for non-rotating quorums. Part of #1262; rotated quarters follow in a separate PR.

What changes

• dash / crypto (first commit): adds BlsSignature::is_valid (Core's CBLSSignature::IsValid(), on the existing parser) and CoinbasePayload::chain_lock_candidate. The latter returns the payload's bestCLSignature when the version, height, zero-signature and height-difference checks of GetNonNullCoinbaseChainlock pass.
• Engine: when the engine takes a diff's list (after the coinbase proof and roots pass), it records that block's coinbase ChainLock together with the block hash. The record is pruned, truncated above a fork and rolled back along with the lists. It is not persisted, so the snapshot format is unchanged.
• validate_quorum: passes the work block's record to the existing LLMQModifierType::new_quorum_modifier_type, with the zero signature when the coinbase carries none. Without a record for that block, the quorum is Skipped(MissingChainLock). The constructor's v20 height check gives the same result as Core: a version 3 coinbase exists only once v20 is active at its block (src/evo/cbtx.cpp:40-43).
• Re-fetch: a work-block list held without its record (after a restart, for example) is requested as a diff from its own block. Core builds a diff whose base equals its block (src/evo/smldiff.cpp:181-183), and the reply carries just the coinbase and its merkle proof. These requests use the existing work-block list path from fix(dash-spv)!: keep masternode sync live on bad or missing diffs #1258: they are queued after the QRInfo and with tip updates, given up at MNLISTDIFF_DEADLINE, and retried once. Dash Core replies with mnlistdiff only when it can build the diff (src/net_processing.cpp:5615-5618), so fix(dash-spv)!: keep masternode sync live on bad or missing diffs #1258's deadline and single retry bound these requests like any other work-block list.

Non-rotating quorums restored as Verified stay Verified. A persisted Invalid or Skipped quorum is validated again. While its work-block list is held without a coinbase record, it is Skipped(MissingChainLock) until the diff from that block arrives, and then it is validated under the coinbase ChainLock. Without the list it is Skipped(MissedList), as before.

#1202 adds a coinbase field at the same points in apply_diff_to_base, ListsAndCycles/QRInfoRollback and truncate_above. Whichever of the two lands second rebases onto the other.

Changelog

• dash-spv: non-rotating quorum members are selected with the ChainLock from the work block's coinbase, as Dash Core does, instead of the quorumsCLSigs value of a diff.

Tests

• non_rotating_members_follow_the_work_block_coinbase_not_quorums_cl_sigs (three cases on the 2240504 fixture). The first two are the issue's: a Llmqtype100_67 quorum's quorumsCLSigs value is replaced, in every diff that brings it, with the zero signature or with another block's ChainLock, and the quorum must still verify. On dev both fail with Invalid(AllCommitmentAggregatedSignatureNotValid). In the third case, a different real ChainLock in the work block's coinbase makes the quorum Invalid.
• a_restored_quorum_waits_for_its_work_block_coinbase (two cases: persisted Unknown served the right signature, persisted Invalid served a wrong one). After a snapshot round trip the quorum is Skipped(MissingChainLock) and its held work-block list is requested from its own block. Once that diff is applied, the quorum is Verified.
• a_self_diff_records_its_coinbase_chain_lock_only_once_proven (two cases, on the 2518986 fixture with coinbase checks on): after a restart, a diff from the newest list's block to itself is refused if its coinbase proof or roots fail, and records nothing. The untampered one applies and records the coinbase ChainLock.
• coinbase_chain_locks_go_with_their_lists, plus the existing refused-QRInfo rollback tests and work-block request tests, now asserting the record.
• dash / crypto: is_valid_follows_dash_core_is_valid (one case per input) and chain_lock_candidate_follows_get_non_null_coinbase_chainlock (one case per cbtx.cpp:206-207 clause, plus the accepted case).
• cargo test -p dash-spv with the dashd regtest integration tests.

🤖 Generated with Claude Code

PR Hygiene · 310a9d3

• Bots — coderabbitai ✓
• Self-review — posted; again after any push
• Build running
• Approvals
  • files with no dedicated owner (crypto/Cargo.toml, crypto/src/bls.rs, dash/src/blockdata/transaction/special_transaction/coinbase.rs) — QuantumExplorer or ZocoLini or xdustinface
  • dash-spv (dash-spv/src/sml_engine.rs, dash-spv/src/sml_engine/helpers.rs, dash-spv/src/sml_engine/validation.rs) — QuantumExplorer or ZocoLini or xdustinface

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

• New Features
  • Added a way to check whether a BLS signature is valid for its signature scheme.
  • Coinbase data can now provide a ChainLock candidate when its version, height, and signature meet validation requirements.
• Bug Fixes
  • Improved SPV quorum validation and masternode-list requests to use ChainLock information from the relevant block’s coinbase.
  • Kept ChainLock records in sync when masternode lists are rolled back, pruned, or truncated.

[github-actions]

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.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/rust-dashcore
• Branch: pasta/spv-work-block-coinbase-chainlock
• File: .coderabbit.yaml
• Commit: 310a9d3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5811c248-7b34-4d2e-8ee6-f19248e1b644

Commits

Reviewing files that changed from the base of the PR and between 9b384c7 and 310a9d3.

Files selected for processing: 6

• crypto/Cargo.toml
• crypto/src/bls.rs
• dash-spv/src/sml_engine.rs
• dash-spv/src/sml_engine/helpers.rs
• dash-spv/src/sml_engine/validation.rs
• dash/src/blockdata/transaction/special_transaction/coinbase.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.

Walkthrough

The changes add checks for coinbase ChainLock candidates and BLS signature validity. The masternode list engine records candidate data with list block hashes, uses it when requesting work-block lists, and uses work-block coinbase ChainLocks during quorum validation.

Changes: Coinbase ChainLock quorum validation

Layer / File(s)	Summary
Candidate extraction and signature checks dash/src/blockdata/transaction/special_transaction/coinbase.rs, crypto/src/bls.rs, crypto/Cargo.toml	CoinbasePayload::chain_lock_candidate checks payload version, height, best ChainLock height, and signature presence and value. BlsSignature::is_valid parses the signature according to its scheme. Tests cover accepted and rejected candidates and signatures.
Engine ChainLock records and lifecycle dash-spv/src/sml_engine.rs, dash-spv/src/sml_engine/helpers.rs	The engine records each list’s block hash and optional valid coinbase ChainLock. Rollback restores these records, while pruning and truncation remove records with their lists.
Work-block requests and quorum validation dash-spv/src/sml_engine.rs, dash-spv/src/sml_engine/validation.rs	A held work-block list is requested from its own block when no matching ChainLock record exists. Quorum validation uses the work block’s coinbase ChainLock. Tests cover request behavior, validation, restoration, and candidate recording.

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant MasternodeListEngine
  participant QRInfo
  participant CoinbasePayload
  participant QuorumValidator
  MasternodeListEngine->>MasternodeListEngine: Check for a ChainLock record matching the work-block hash
  MasternodeListEngine->>QRInfo: Request the work-block list when no matching record exists
  QRInfo->>MasternodeListEngine: Provide the work-block diff
  MasternodeListEngine->>CoinbasePayload: Read the candidate at the diff height
  MasternodeListEngine->>MasternodeListEngine: Store the block hash and candidate with the list
  QuorumValidator->>MasternodeListEngine: Read the work-block ChainLock data

Loading

Possibly related issues

• dashpay/rust-dashcore#1262 — The PR uses the work block’s coinbase ChainLock rather than quorumsCLSigs for non-rotating quorum validation.
• dashpay/rust-dashcore#1233 — The PR requests the work-block list when its ChainLock record is missing. It does not add attempt limits or request withdrawal.

Suggested reviewers: kwvg

Merge Risk: Minimal · up to 310a9

No actionable issue is established from the available evidence. The change appears mergeable after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: non-rotating quorum member selection now takes its ChainLock from the work block's coinbase. The changeset matches this. The coinbase ChainLock candidate, the validate…
Docstring Coverage		Docstring coverage is 88.89% 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		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: non-rotating quorum member selection now takes its ChainLock from the work block's coinbase. The changeset matches this. The coinbase ChainLock candidate, the validate_quorum change, and the ChainLock record in the engine all serve that goal. The title is in the conventional commit format and is specific. It is slightly long at 94 characters, but it is still a single clear sentence.

Full details: Docstring Coverage

Explanation

Docstring coverage is 88.89% 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 18 functions across 5 files. (1 skipped: 1 unsupported.)

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

• X
• Mastodon
• Reddit
• LinkedIn

[codecov]

Codecov Report

❌ Patch coverage is 98.43137% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.06%. Comparing base (3bcdc29) to head (310a9d3).

Files with missing lines	Patch %	Lines
dash-spv/src/sml_engine.rs	98.63%	3 Missing ⚠️
...ckdata/transaction/special_transaction/coinbase.rs	96.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1284      +/-   ##
==========================================
+ Coverage   82.00%   82.06%   +0.05%     
==========================================
  Files         290      290              
  Lines       75955    76201     +246     
==========================================
+ Hits        62287    62532     +245     
- Misses      13668    13669       +1     

Flag	Coverage Δ
core	81.92% <96.77%> (+0.02%)	⬆️
ffi	59.73% <ø> (ø)
rpc	52.10% <ø> (ø)
spv	93.89% <98.66%> (+0.05%)	⬆️
wallet	83.07% <ø> (ø)

Files with missing lines	Coverage Δ
dash-spv/src/sml_engine/helpers.rs	100.00% <100.00%> (+0.29%)	⬆️
dash-spv/src/sml_engine/validation.rs	94.54% <100.00%> (+3.55%)	⬆️
...ckdata/transaction/special_transaction/coinbase.rs	92.84% <96.77%> (+0.31%)	⬆️
dash-spv/src/sml_engine.rs	97.43% <98.63%> (+0.12%)	⬆️

... and 1 file with indirect coverage changes

[github-actions]

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

[PastaPastaPasta]

/self-reviewed

[github-actions]

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

[ZocoLini]

The fix itself looks right. I checked the cited Dash Core code (GetNonNullCoinbaseChainlock, GetHashModifierFromWorkBlock, BuildSimplifiedMNListDiff) and every masternode_lists mutation path, and found no correctness bug. The problem is scope: 984 added lines, about 170 of them production code, for a change that needs far less. Please limit this PR to the code that resolves the non-rotating part of #1262, plus tests for its new paths, using test_case.

Remove from production code

1. The retry_given_up_work_lists change. It is unrelated to #1262 and nothing in this fix depends on it. That means the manager.rs changes and their test, MockHeaderStorage::unreadable and the rest of the header_storage.rs changes, pub(crate) on scan_missing_work_block_lists, and the ..MockHeaderStorage::default() line. If it's worth having, it should be its own PR.
2. LLMQModifierType::from_work_block and its test. The existing constructor already does this: new_quorum_modifier_type(llmq_type, hash, height, chain_lock.unwrap_or(BLSSignature::from([0; 96])), network). The v20 check gives the same result, because a version 3 coinbase already implies a height at or above activation.
3. The quorumsCLSigs debug log in validate_quorum. It doesn't change behaviour.
4. The best_cl_height / best_cl_signature field docs. They are unrelated to the fix.
5. The u32 in chain_lock_candidate's return value. The only caller discards it, so return Option<BLSSignature>.

I applied 1 to 3 locally on 9b384c7. The sml_engine and sync::masternodes tests in dash-spv (99) and the quorum_modifier_type and coinbase tests in dashcore (18) all pass, including the PR's own remaining tests.

Reduce the tests

sync_manager.rs has 168 added lines and no production change, so it adds no new path to cover.

Remove:

• manager_asking_for_a_held_work_block_list, a_work_block_list_asked_for_from_its_own_block_is_applied and an_unanswered_work_block_list_from_its_own_block_is_given_up_and_retried_once. These exercise the #1258 path, and the request itself is already asserted at engine level.
• the_snapshot_leaves_the_coinbase_chain_locks_out. It tests something that wasn't added.
• a_diff_applied_twice_records_the_same_chain_lock.

Merge with test_case:

• a_restart_asks_for_the_work_block_coinbase_again, without_the_coinbase_chain_lock_a_quorum_is_skipped and a_persisted_invalid_quorum_verifies_once_its_coinbase_is_read into one test, with the persisted status and served signature as cases: skipped first, then verified after the self-diff.
• a_non_rotating_quorum_verifies_whatever_quorums_cl_sigs_serves and the_work_block_coinbase_chooses_the_members into one test, with the diff edit and expected status as cases.
• chain_lock_candidate_follows_get_non_null_coinbase_chainlock: one case per clause instead of the array, without near-duplicates.
• is_valid_follows_dash_core_is_valid: one case per input.

Keep a_self_diff_records_its_coinbase_chain_lock_only_once_proven and coinbase_chain_locks_go_with_their_lists.

That should leave roughly 440 added lines, around 300 of them tests.

[PastaPastaPasta]

Thanks for checking it against Core. I've applied all of it, and pushed 310a9d3 (rebased onto current dev). The diff is now +449/−25 over 6 files: 107 lines of production code and 342 of tests.

Production code

1. The retry_given_up_work_lists change is gone. manager.rs, sync_manager.rs and test_utils/header_storage.rs have no diff now, and the pub(crate) and ..MockHeaderStorage::default() changes are gone too. I won't file it separately: it only matters when a header read fails at the retry, and then the quorum just stays Skipped(MissedList) until the next QRInfo.
2. from_work_block and its test are removed. validate_quorum calls new_quorum_modifier_type(…, chain_lock.unwrap_or(BLSSignature::from([0; 96])), network). I checked your v20 argument at Core develop 15db47875, and it holds:
   • CheckCbTx rejects a version 3 payload unless v20 is active at its block (src/evo/cbtx.cpp:40-43). That is a buried height (src/deploymentstatus.h:14-18).
   • v20_activation_height matches Core's V20Height on mainnet and testnet (src/chainparams.cpp:194, :394).
3. The quorumsCLSigs debug log is removed.
4. The best_cl_* field docs are removed, along with the other doc rewrites that weren't needed.
5. chain_lock_candidate returns Option<BLSSignature>.

Tests

• The five you named are removed. sync_manager.rs has no diff.
• Restart group: merged into a_restored_quorum_waits_for_its_work_block_coinbase, with two cases. A persisted Unknown quorum is served the right signature, and a persisted Invalid one a wrong one. Each case is Skipped(MissingChainLock) and requests its work block's list from that block, then Verified after the self-diff. I dropped a third case because the served value is never read there, so it would have repeated one of these two.
• Issue group: merged into non_rotating_members_follow_the_work_block_coinbase_not_quorums_cl_sigs, with three cases. A zero signature and another block's ChainLock both give Verified; a different coinbase ChainLock gives Invalid. The first two fail on dev.
• chain_lock_candidate_follows_get_non_null_coinbase_chainlock has one test_case per cbtx.cpp:206-207 clause, plus the accepted case.
• is_valid_follows_dash_core_is_valid has one case per input. This adds test-case as a crypto dev-dependency. If chore!: general dependency maintenance #1289 lands first, I'll switch it to { workspace = true }.
• a_self_diff_records_its_coinbase_chain_lock_only_once_proven and coinbase_chain_locks_go_with_their_lists are unchanged.

Tests come to about 340 lines rather than ~300. Most of the difference is the two tests you asked to keep. cargo test -p dash-spv passes with the dashd integration tests.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[github-actions]

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]

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

[PastaPastaPasta]

/self-reviewed 310a9d3