fix(dash): key rotation ChainLock signatures per quorum work block - #934
Merged
xdustinface merged 36 commits intoAug 12, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRotated quorum QRInfo processing now supports per-quorum reconstruction and validation, inferred signatures, partial cycle storage, optional capture, and expanded feed observability. New statuses classify inferred-signature and low-cycle-base conditions. ChangesRotated quorum QRInfo validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QRInfoPeer
participant MasternodeListEngine
participant RotatedQuorumConstruction
participant CycleStorage
QRInfoPeer->>MasternodeListEngine: submit QRInfo diffs
MasternodeListEngine->>RotatedQuorumConstruction: reconstruct quorum members and quarter signatures
RotatedQuorumConstruction-->>MasternodeListEngine: return per-quorum validation results
MasternodeListEngine->>CycleStorage: merge verified indexed entries
CycleStorage-->>MasternodeListEngine: return stored cycle state
MasternodeListEngine-->>QRInfoPeer: return QRInfoFeedResult fields
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #934 +/- ##
==========================================
+ Coverage 75.62% 76.33% +0.71%
==========================================
Files 329 329
Lines 79258 79940 +682
==========================================
+ Hits 59939 61024 +1085
+ Misses 19319 18916 -403
|
This was referenced Aug 10, 2026
Open
When `DASH_SML_DUMP_QRINFO_DIR` is set, `feed_qr_info` writes the pre-feed block container and the raw `QRInfo` to that directory before processing. Validation failures against live chain state are otherwise unreproducible once the rotation cycle advances, so the capture preserves the failing input as a test fixture. Inert without the environment variable.
Core attaches each rotated quorum's ChainLock signature to that quorum's own work block (`quorumsCLSigs`, cycle base minus 8), but `feed_qr_info` collapsed them to one signature per diff, taken from the first rotating quorum encountered, and stamped the same 4-signature tuple onto every entry of a batch. The active rotation set can span several cycles at once, because a failed DKG leaves the previous cycle's quorum in place at that index, so every such straggler got quarter modifiers built from another cycle's signatures. The reconstructed member sets then disagreed with the real signers and `feed_qr_info` hard-failed the whole QRInfo with `AllCommitmentAggregatedSignatureNotValid` on the first fresh-sync response, permanently wedging masternode sync, ChainLock validation and InstantSend verification. `feed_qr_info` now builds a work-height-to-signature map from every diff's per-quorum `quorumsCLSigs` data and each rotated quorum looks up the four quarter signatures of its own cycle. Groups whose quorum hash heights are unknown are keyed by elimination oldest diff first. Entries whose signatures are missing settle as `Skipped` individually instead of poisoning the batch, and `find_rotated_masternodes_for_quorums` returns a per-quorum `Result` for the same reason. The cycle key for `rotated_quorums_per_cycle` is now derived from the newest cycle base among the entries instead of the first entry's quorum hash, which stored the set under the old cycle's hash whenever quorum index 0 was a straggler and broke IS lock cycle lookups. `qr_info_referenced_block_hashes` includes rotating quorum hashes from all diffs so callers feed the heights the signature map needs. Regression fixture `qrinfo_2518986` was captured from a live mainnet fresh sync at tip 2518986, where index 0 of the active set carried the previous cycle's commitment and reproduced the failure deterministically.
…inLock signatures A work block whose coinbase carries no ChainLock reaches the wire as an all-zero signature in `quorumsCLSigs`. Core then derives the quorum modifier from the work block hash, but `new_quorum_modifier_type` always used the signature form post-v20, so member scores diverged from Core and rotated quorum validation failed with an invalid aggregated signature whenever a quarter's work block lacked a coinbase ChainLock.
Core fills all rotated quorum quarters from one shared cursor that wraps back to the front of the sorted combined masternode list, reusing masternodes when fewer than `quorum_count * quarter_size` are available. The `chunks` based fill produced short or missing quarters in that case, which silently diverged from Core's member sets on networks with few masternodes. Also mirror Core's empty-list early return, which additionally guards the `SkipFirst` arm's modulo arithmetic against an empty list.
…fo feed Previous-cycle validation is best-effort enrichment for IS lock verification, but an `Invalid` entry there propagated as a hard error out of `feed_qr_info`, discarding the entire QRInfo including the fully valid current cycle. An invalid previous-cycle entry now only prevents that cycle from being stored, logged at warn level, while corrupt current-cycle data in `lastCommitmentPerIndex` still rejects the feed.
…s cycle The previous rotation cycle's active set can carry a straggler from a cycle older than the QRInfo's diff range, whose quarter masternode lists are never shipped, so that one entry cannot possibly verify. The all-or-nothing storage gate then dropped the entire otherwise-verified cycle and every IS lock referencing it failed with a missing cycle. The previous-cycle path now stores the verified entries and leaves unverifiable or invalid ones out individually. `build_cycle_quorum_map` accepts partial sets, which is safe because IS lock verification selects the quorum by index and fails cleanly with `QuorumIndexNotFound` when a lock selects an absent one, so only locks signed by exactly the missing quorum are affected.
…ing the cycle base `QuorumEntry::quorum_index` is an `Option<i16>` straight off the wire that nothing bounds, so the open-coded `quorum_block_height - quorum_index as u32` in `find_rotated_masternodes_for_quorum` and `find_rotated_masternodes_for_quorums` sign-extended a negative index into a huge `u32` and then panicked under overflow checks or wrapped in release. Both paths are reachable from `feed_qr_info` on peer data. The checked arithmetic already living in `rotated_quorum_cycle_base` now sits in a shared `rotated_cycle_base_height` helper that both it and the two construction sites call, so there is one implementation and no unchecked copies. Each site keeps its existing error shape and reports `InvalidQuorumIndex` for an index the cycle base cannot absorb. The quarter work block arithmetic in `required_cl_sig_heights` and `masternode_list_entry_members_for_rotated_quorum` underflowed for any cycle within a few DKG intervals of genesis, which is reachable on regtest and devnet. The former now skips work blocks below genesis, the latter reports `CycleBaseHeightTooLow`.
`rotation_cl_sigs_by_work_height` keys a signature group whose quorum heights are all unknown by elimination, presuming it belongs to the diff's newest cycle. A wrong presumption yields a wrong quorum modifier, a wrong reconstructed member set and a failing aggregate check, which `feed_qr_info` then turned into an `Err` that aborted the entire feed. A caller that under-supplied block heights could therefore wedge masternode sync permanently. The keying now reports which work heights it inferred, and a current-cycle entry whose quarter signatures rest on one settles as `Skipped` instead. Corrupt data on an exactly keyed signature still rejects hard, and a `Skipped` entry still cannot enter `rotated_quorums_per_cycle` because the storage gate requires every entry to be `Verified`. The elimination pass also declined a group whose signature value already keyed another work height, treating the repeat as evidence of an older cycle. Core deliberately merges work blocks that share a signature, which is routine while those blocks carry no ChainLock and the signature is all zeroes, so the rule now applies only to non-zero signatures that genuinely identify one block. `rotation_cl_sigs_by_work_height` had no targeted coverage. It gains a unit test over synthetic diffs for exact keying, elimination, ambiguity, the zeroed merged group and the non-zero repeat, plus a fixture-driven test that an inferred key failing validation settles `Skipped` rather than aborting the feed.
…comment `validate_and_store_previous_cycle_quorums` looks each quorum's quarter signatures up by work height rather than aligning them positionally, so the `h` height is no longer load-bearing for signature alignment. The reason it must stay `h`, that the h-c cycle's quorums live on `masternode_lists[h]`, is unchanged.
…ture `bincode` is a default feature, so the capture hook shipped compiled in for every downstream release build and a stray `DASH_SML_DUMP_QRINFO_DIR` in the environment would have it write roughly 640KB per QRInfo to an arbitrary path. The new `qrinfo-capture` feature is off by default and pulls `bincode` in when enabled. An unresolvable tip height used to fall back to `0`, which collapsed every such capture onto one pair of file names where each write overwrote the last. Skip the capture instead, and document the environment variable and the file layout on the method.
…sh preview `qr_info_referenced_block_hashes` tells callers which block heights to feed before `feed_qr_info` needs them, so the two must walk exactly the same set of diffs. Both built that list inline, leaving the invariant to be maintained by hand in two places.
…he wire `rotation_quorum_type` drives the masternode list lookups at `h` and the index range `build_cycle_quorum_map` enforces, so deriving it from the first entry of the peer-supplied `last_commitment_per_index` let a peer pick which type the engine reasons about. The surrounding rotation paths already resolve it through `Network::isd_llmq_type`, and the empty-list case already fell back to it.
…feed `validate_and_store_previous_cycle_quorums` is documented as best-effort enrichment, yet the index keying it does at the end still propagated `RequiredQuorumIndexNotPresent`, `InvalidQuorumIndex` and the duplicate-index `CorruptedCodeExecution` out of `feed_qr_info`. A duplicate `quorum_index` across two otherwise verified entries would fail the whole feed from the one path that promises not to. The cycle is now logged and left unstored instead, which drops the function's last failure mode and with it its `Result`. The authoritative `lastCommitmentPerIndex` path still rejects hard.
`is_cycle_fully_verified` treated any non-empty all-`Verified` map as complete, so a cycle stored with part of its set (the fixture at 2518986 legitimately stores 31 of 32) looked finished and permanently blocked further writes to that cycle key. The missing entries are exactly the ones whose context the storing QRInfo did not carry, which is what a later QRInfo is expected to supply. Requiring one entry per active rotation slot costs nothing on the authoritative current-cycle path, where `lastCommitmentPerIndex` always covers the full set, and re-opens the previous-cycle path for completion.
…aller A previous-cycle rotated quorum that fails validation is logged and left out of the stored cycle, so a peer serving forged rotated commitments still got `Ok(...)` back with nothing in the result to act on. `QRInfoFeedResult` now carries the count. `all_fully_verified` is deliberately untouched: it is the trust gate for the current cycle, which the enrichment path does not feed, and dash-spv uses it to decide whether the cycle needs another QRInfo. The count is surfaced in the QRInfo log line and in the `MasternodeStateUpdated` display.
Fold the repeated bincode decode boilerplate into one `decode_fixture` helper, and derive both hand-rolled cycle bases through `rotated_quorum_cycle_base` instead of recomputing height minus index inline. The mainnet DKG interval literal now comes from the network params that name it. `feed_qr_info_degrades_previous_cycle_on_corrupt_aggregate_signature` zeroed the signature, which trips the structural zero check in `validate_structure` and never reaches aggregate verification, so it did not test what its name says. It now substitutes another quorum's signature, which is structurally sound and fails the aggregate check, and asserts the surviving entry count is exactly one lower than the rotated set at `h`, so degradation dropping more than the corrupted quorum would fail.
…h preview `qr_info_referenced_block_hashes` now collects rotating commitment hashes from every diff rather than only from `mn_list_diff_h`, which is what lets the caller resolve each quorum's own cycle base. Nothing in the workspace exercised the widening, and its only consumer is dash-spv. The 2518986 fixture's previous-cycle straggler sits in the h-c diff, so asserting the preview lists it fails against the narrower behaviour.
The hook reached for `bincode::encode_to_vec` and `bincode::config::standard` through fully qualified paths where the module can import them.
`last_commitment_per_index` is peer controlled and its length is not validated, so storing a cycle by whole-map replacement let a peer serving one genuine commitment from an already-stored cycle drop the other indices and break InstantSend verification for the rest of that cycle. Verified entries now merge per quorum index at both storage sites, with the incoming entry winning a collision.
Insufficient history for a rotation reconstruction is a missing-context condition like every sibling `Required*NotPresent` error, so it must degrade the one quorum rather than abort the whole feed as `Invalid` does.
`OtherContext` carries a prose string that retry logic cannot dispatch on, unlike every sibling variant. `InferredRotationChainLockSigs` names the condition and carries the quorum hash, and is appended last so existing bincode discriminants keep their values.
The `[B-3c-8, B-2c-8, B-c-8, B-8]` arithmetic had four copies. `cycle_quarter_work_heights` returns the quarters as options so each caller keeps its own reading of an underflow: the required-height survey takes what exists, `quarter_work_heights` needs all four, and the reconstruction reports `CycleBaseHeightTooLow`. The cycle base in `required_cl_sig_heights` stays derived from the height modulo the interval rather than from `quorum_index`, because the index-based form skips a quorum that carries no index where the modulo form still contributes its heights.
The presumed height of an unresolved rotated quorum is derived from a wire-supplied quorum index and work height, the one unchecked addition in a function that is otherwise checked throughout. An overflow now skips the hypothesis check for that quorum, like an unusable index already does.
The `MissingRotationChainLockSigs` doc still described the deleted per-diff rotation signature, and `qr_info_referenced_block_hashes` claimed to name every hash the feed needs while a straggler's cycle base block is not carried in the message at all. `feed_qr_info_does_not_store_cycle_when_rotation_sigs_missing` derived its expected cycle key from the index-0 commitment, which only coincidentally matches the key production computes, and the two rotation-status tests in `validation.rs` had converged on the same assertions after the batch error path was removed.
Rewords three log messages that used a semicolon as a clause separator, routes two tests through `MasternodeListEngine::feed_block_height` instead of reaching into the block container, and drops the block hash alongside the height in the inferred-signature test so it simulates a state the container can actually be in.
`all_fully_verified` compared the verified count against the number of rotated quorums a peer served, and that length is peer-controlled. A peer serving a truncated but genuine `lastCommitmentPerIndex` therefore passed the trust gate, which lets a caller record the cycle as done and stop asking for the rest of it, leaving InstantSend locks from the omitted indices unverifiable for the remainder of the cycle. `QRInfoFeedResult` now carries `expected_rotated_quorum_count`, the active quorum count of the network's rotation quorum type, and the gate additionally requires the served set to cover it. Storage keeps merging verified entries per quorum index, so a later QRInfo carrying the missing indices still completes the cycle.
A fully verified active set whose cycle base no entry resolves was dropped without a trace: the storage call sat behind an `if let` with no else, so nothing was stored while the result still reported every quorum verified. The two states need different responses, a blocked storage gate wants better data from the peer while an unresolvable key wants more block heights from the caller, and neither was distinguishable. `feed_qr_info` now resolves the cycle key once for both branches, warns when the served set yields none, and reports it as `cycle_key_unresolved` on `QRInfoFeedResult`.
Rotation ChainLock signatures are keyed by the work block of the cycle a quorum belongs to, and that cycle base is derived as the commitment block height minus the `quorum_index` the peer sent. Nothing tied the index to the block, so a shifted one moved the base into another cycle and let one crafted entry replace the genuine signature keyed at that cycle's work block, which rebuilds the wrong member set for every quorum of the cycle it hit. `rotated_quorum_cycle_base` now accepts an index only while it names a slot of the active set and lands the base on a DKG interval boundary, which together leave no shift that resolves. An entry failing the check contributes no exact key and falls back to the elimination pass like any other unresolvable one.
The per-cycle reconstruction cache was filled by whichever quorum of a cycle came first, including when that quorum carried no rotation ChainLock signatures at all. The resulting `RequiredRotatedChainLockSigsNotPresent` was then handed to every sibling of the cycle, so one entry served without signatures made the whole cycle unverifiable even though its siblings carried everything their own verification needs. Only a successful reconstruction is cached now. A quorum that carries no signatures fails on its own account and leaves the cycle open.
A rotation ChainLock signature whose work block was keyed by elimination can belong to a different cycle, which reconstructs the wrong member set and fails the aggregate check on genuine data. The current-cycle path already degrades such a quorum to `Skipped(InferredRotationChainLockSigs)`, but the previous-cycle path counted every failure into `previous_cycle_invalid_count`, the one signal a caller has for judging the peer that served the commitments. On a fresh sync the oldest quarters of the previous cycle are routinely keyed by elimination, so the count reported our own missing context as corruption. `validate_and_store_previous_cycle_quorums` now takes the inferred work heights and settles a failure resting on one as skipped without counting it.
… wire `feed_qr_info` keys the active set by the network's configured rotation quorum type, which is right everywhere the deployed type is fixed. A devnet is deployed with whichever DIP24 type it was launched with, and without `set_devnet_isd_type` the configured type is only a guess: `build_cycle_quorum_map` then rejects every quorum index the guessed type has no slot for and the whole feed aborts. `rotation_quorum_type` keeps configuration authoritative on mainnet, testnet, regtest and on a devnet that was told its type, and falls back to the served commitments only on a devnet that was not. Only a rotating type is adopted.
Every `feed_qr_info` call walked all of the QRInfo's diffs, keyed each `quorumsCLSigs` group to a work block and ran the elimination pass over what was left, including on a re-feed where the engine already holds a qualified quorum for every entry of the active set and no h-4c share opens the previous cycle. The map is unread in that case. The keying is gated on either of those needing it, which is what the per-diff signature collection it replaced used to do.
`find_rotated_masternodes_for_quorum` walked the same four-step resolution as the loop body of `find_rotated_masternodes_for_quorums`, height lookup, quorum index, cycle base and rotating signatures, and built the same errors from it. It delegates to the plural now, which also takes its per-cycle reconstruction cache along. The plural no longer ties the slice it reads to the lifetime of the members it returns, since those borrow the engine rather than the slice.
Both loops of `required_cl_sig_heights` resolved a quorum's height, derived its cycle base by modulo and extended the set with the cycle's four quarter work heights, differing only in the context string of the missing-height error. That body lives in `extend_with_quarter_work_heights` now and both loops call it.
The block hash preview told callers what to feed by pointing at what one particular downstream crate does. Comments do not get to reference their callers, so the contract stands on its own now.
xdustinface
force-pushed
the
fix/rotated-quorum-cl-sig-alignment
branch
from
August 12, 2026 11:09
bfa9a0e to
2abdfc8
Compare
xdustinface
marked this pull request as ready for review
August 12, 2026 11:09
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs (1)
544-555: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove hardcoded test network values.
Line 549 hardcodes a network address. Line 550 hardcodes a key value. Use a reusable test fixture source instead.
As per coding guidelines,
**/*.rs: “Never hardcode network parameters, addresses, or keys.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs` around lines 544 - 555, Update the test helper function entry to obtain the service address and operator public key from the existing reusable test fixture source instead of hardcoding 127.0.0.1:9999 and a byte-filled BLSPublicKey. Preserve the generated entry’s remaining fields and ensure the fixture provides valid network and key values.Source: Coding guidelines
dash/src/sml/masternode_list_engine/mod.rs (1)
612-624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the resolved rotation quorum type for the previous-cycle path.
validate_and_store_previous_cycle_quorumstakesisd_typefromself.network.isd_llmq_type(). The current-cycle path now resolves the type throughrotation_quorum_type(), which can adopt the served rotating type on an unconfigured devnet. On such a devnet the two paths can disagree, somn_list.quorums.get(&isd_type)finds no entry and the previous-cycle enrichment is skipped silently.is_cycle_fully_verifiedat Line 779 uses the same configured type for the expected count.The effect is limited to devnet enrichment, so this is not urgent. Passing the resolved type into this function (and into
is_cycle_fully_verified) would keep both paths on one type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash/src/sml/masternode_list_engine/mod.rs` around lines 612 - 624, Update validate_and_store_previous_cycle_quorums and is_cycle_fully_verified to use the resolved rotation quorum type from rotation_quorum_type() instead of self.network.isd_llmq_type(). Resolve the type once in the surrounding current-cycle flow and pass it through both paths so previous-cycle enrichment and expected-count verification use the same quorum type, including unconfigured devnets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs`:
- Around line 724-747: Extend skip_first_decodes_relative_skip_list_entries to
use offsets [3, 2, 2] and assert skipped indexes 3, 5, and 7. Update the
SkipFirst decoder’s processed_skip_list accumulation so each subsequent offset
is added to the previously accumulated skipped index, including after the second
entry.
- Around line 193-196: Update the required-heights construction around
quorum_block_height to derive cycle_base_height with
rotated_cycle_base_height(quorum_block_height, quorum_index) instead of manually
rounding by cycle_length. Reuse the helper’s validation behavior, then pass the
resulting base to cycle_quarter_work_heights while preserving the existing
required_heights extension.
---
Nitpick comments:
In `@dash/src/sml/masternode_list_engine/mod.rs`:
- Around line 612-624: Update validate_and_store_previous_cycle_quorums and
is_cycle_fully_verified to use the resolved rotation quorum type from
rotation_quorum_type() instead of self.network.isd_llmq_type(). Resolve the type
once in the surrounding current-cycle flow and pass it through both paths so
previous-cycle enrichment and expected-count verification use the same quorum
type, including unconfigured devnets.
In `@dash/src/sml/masternode_list_engine/rotated_quorum_construction.rs`:
- Around line 544-555: Update the test helper function entry to obtain the
service address and operator public key from the existing reusable test fixture
source instead of hardcoding 127.0.0.1:9999 and a byte-filled BLSPublicKey.
Preserve the generated entry’s remaining fields and ensure the fixture provides
valid network and key values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd10d249-7146-4eb8-b58b-21d9513d2cef
⛔ Files ignored due to path filters (2)
dash/tests/data/test_DML_diffs/block_container_2518986.datis excluded by!**/*.datdash/tests/data/test_DML_diffs/qrinfo_2518986.datis excluded by!**/*.dat
📒 Files selected for processing (9)
dash-spv/src/sync/events.rsdash-spv/src/sync/masternodes/sync_manager.rsdash/Cargo.tomldash/src/sml/llmq_entry_verification.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/masternode_list_engine/rotated_quorum_construction.rsdash/src/sml/masternode_list_engine/validation.rsdash/src/sml/quorum_entry/quorum_modifier_type.rsdash/src/sml/quorum_validation_error.rs
…index Addresses CodeRabbit review comment on PR dashpay#934 dashpay#934 (comment)
ZocoLini
approved these changes
Aug 12, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A fresh mainnet sync deterministically fails on the first QRInfo response.
MasternodeListEngine::feed_qr_inforejects it withAllCommitmentAggregatedSignatureNotValid("invalid signature"), whichdash-spvtreats as fatal, permanently wedging masternode sync, ChainLock validation and InstantSend verification for the rest of the session. The same failure was reported on testnet in #907.Root cause
Core keys each rotated quorum's ChainLock signature to that quorum's own work block (
quorumsCLSigs, work block is the cycle base minus 8, seeCSimplifiedMNListDiff::BuildQuorumChainlockInfo). The engine collapsed that mapping to one signature per diff, taken from the first rotating quorum it encountered, and stamped the same 4-signature tuple onto every quorum in a batch.The active rotation set is the latest commitment per quorum index regardless of cycle. Whenever an index fails its DKG, the previous cycle's quorum stays in the set at that index. At the captured mainnet state (tip 2518986, cycle 2518848) quorum index 0 carried the commitment from cycle 2518560. Such stragglers got quarter modifiers built from another cycle's signatures, the reconstructed member sets disagreed with the real signers, and the BLS aggregate check correctly failed, killing the whole feed.
Two more divergences from Core were found in the same audit and are fixed in separate commits, both chain-state dependent drivers of the intermittency seen in #907:
quorum_count * quarter_sizemasternodes existFix
feed_qr_infobuilds a work-height-to-signature map from every diff's per-quorumquorumsCLSigsdata, and each rotated quorum looks up the four quarter signatures of its own cycle. Signature groups whose quorum hash heights are unknown are keyed by elimination, walking diffs oldest first.Skippedindividually instead of poisoning the batch.find_rotated_masternodes_for_quorumsreturns a per-quorumResultfor the same reason.rotated_quorums_per_cycleis derived from the newest cycle base among the entries instead of the first entry's quorum hash. The old key stored the active set under the previous cycle's hash whenever index 0 was a straggler, which broke InstantSend lock cycle lookups.new_quorum_modifier_typefalls back to the block-hash modifier for zeroed signatures.Reproduction and testing
The failing chain state was captured live from mainnet via a new env-gated hook (
DASH_SML_DUMP_QRINFO_DIR) and committed as fixtureqrinfo_2518986with its block container. The new regression testvalidate_first_qr_info_on_fresh_engine_with_mixed_cycle_quorumsreplays production cold start offline: it failed with the exact production error before the fix and now asserts all 32 quorums verify and the set is stored under the current cycle hash.New unit tests pin the skip-list strategies (NoSkipping wrap-around, SkipFirst relative decoding, empty-list behavior), the modifier selection, and the degraded previous-cycle error contract.
Verified: full
dashcoreanddash-spvsuites including the dashd regtest integration tests, clippy with all features, and a live mainnet fresh sync that now processes the first QRInfo cleanly (rotated_quorum_count=32, fully_verified_count=32, stored_cycle_height=2518848).Closes #907.
Follow-ups (out of scope)
dash-spvhas no retry path whenfeed_qr_infoerrors: one failed QRInfo permanently stalls masternode sync until restart. That area is being rewritten in refactor(dash-spv): network manager refactor and sync pipelines optimized #902, so the retry fix should land on top of it.Summary by CodeRabbit