fix(dash): key rotation ChainLock signatures per quorum work block - #934
Draft
xdustinface wants to merge 6 commits into
Draft
fix(dash): key rotation ChainLock signatures per quorum work block#934xdustinface wants to merge 6 commits into
xdustinface wants to merge 6 commits into
Conversation
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.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #934 +/- ##
==========================================
+ Coverage 75.18% 75.31% +0.12%
==========================================
Files 328 328
Lines 78194 78487 +293
==========================================
+ Hits 58792 59110 +318
+ Misses 19402 19377 -25
|
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
1 participant
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.