Skip to content

Commit dd7937a

Browse files
authored
fix(dash): make feed_qr_info resilient to missing rotation CL sigs (#736)
* fix(dash): make `feed_qr_info` resilient to missing rotation CL sigs QRInfos that span a block range with no successful rotating DKG arrive with per-diff CL sigs missing. Previously this hard-errored with `RequiredRotatedChainLockSigNotPresent`. Now affected entries land as `Skipped(MissingRotationChainLockSigs)` and the cycle is left unstored, deferring IS lock verification for that cycle until a later QRInfo carries complete sigs. Tighten the `rotated_quorums_per_cycle` insertion gate: store a cycle only when every entry is `Verified`, and never overwrite an already-fully-verified cycle (a thin follow-up QRInfo must not downgrade prior trust). Add a previous-cycle storage path (`validate_and_store_previous_cycle_quorums`) that uses the 4 historical sigs `[h-3c, h-2c, h-c, h]` to validate and store the rotated quorums living on `masternode_lists[h]` under their cycle boundary hash. This enables IS lock verification for the previous cycle on a fresh sync, where `lastCommitmentPerIndex` only covers the current cycle. Change `feed_qr_info` to return `Result<Option<QRInfoFeedResult>>` so callers can observe rotated quorum counts, fully-verified counts, and which cycle (if any) was stored. New tests cover the soft-skip storage gate, corrupt aggregate-signature rejection, and the anti-clobber guarantee. The existing `validate_from_qr_info_and_mn_list_diffs` test is extended to assert both cycles are stored and to re-validate every stored quorum. * refactor(dash): drop `expect()` from `collect_rotation_sigs` Replace the provably safe but rule-violating `expect()` with `?` propagation on a destructured array. Behavior is unchanged: the missing-slot debug log still fires before the unwrap path, and `?` on already-checked `Some` values short-circuits cleanly. Addresses CodeRabbit review comment on PR #736 #736 (comment) * fix(dash): key previous-cycle storage by index-0 quorum hash `try_load_previous_cycle_entries` derived its `cycle_hash` via `entries.first()` from a `BTreeMap<QuorumHash, _>`, so the key was the hash-order minimum rather than the cycle-boundary quorum. Dash Core defines `cycleHash` as the block hash at the cycle-base height, which equals the `quorum_hash` of the index-0 quorum, and IS lock verification looks up `rotated_quorums_per_cycle` with that key. Mis-keyed storage caused silent IS-lock verification failures for the previous cycle. Find the index-0 entry's `quorum_hash` instead, and short-circuit the already-stored check before allocating the `entries` vector. Addresses CodeRabbit review comment on PR #736 #736 (comment) * fix(dash): gate `verify_rotated_quorums == false` storage on all-verified The `else if` branch in `feed_qr_info` skipped verification but still wrote into `rotated_quorums_per_cycle` whenever `already_fully_verified` was false. That contradicted the documented invariant ("a cycle is only stored when every entry is `Verified`") and could poison IS-lock BLS verification with unverified `quorum_public_key` values. Mirror the verified-only gate from the `verify_rotated_quorums == true` path: only store when every entry is `Verified` (which can still hold via cache hits from `known_qualified_quorum_entry`). Addresses CodeRabbit review comment on PR #736 #736 (comment) * fix(dash): silence unused-variable warnings without `quorum_validation` Gate the per-cycle rotation-sig bindings, the `RotationChainLockSignatureSlot` enum, its `Display` impl, and the `core::fmt` import behind `#[cfg(feature = "quorum_validation")]` so default-feature builds stay warning-free. Addresses CodeRabbit review comment on PR #736 #736 (comment) * fix(dash): gate `feed_qr_info_rejects_post_v20_with_missing_chainlock_signatures` test on `quorum_validation` The test calls `load_qrinfo_2240504_fixture` which is gated on `quorum_validation`. Without the matching gate the test fails to compile when the feature is off. Addresses CodeRabbit review comment on PR #736 #736 (comment) * refactor(dash): extract `store_cycle_if_fully_verified` from `feed_qr_info` Both the `verify_rotated_quorums == true` and `== false` branches in `feed_qr_info` had identical logic for gating storage of a rotation cycle into `rotated_quorums_per_cycle`: check that every entry is `Verified`, refuse to overwrite a cycle that is already fully verified, then build the cycle map and record the stored height. Move that logic into a single helper so the two call sites stay aligned. * refactor(dash): unify cycle-already-verified check via `is_cycle_fully_verified` `store_cycle_if_fully_verified` and `try_load_previous_cycle_entries` both decide whether to skip work based on what is already in `rotated_quorums_per_cycle`, but the predicates had drifted. The storage gate required a non-empty cycle with every entry `Verified`, while the previous-cycle loader only checked `contains_key`. The two now share a single `is_cycle_fully_verified` helper so the previous-cycle path also refuses to short-circuit on a partial entry. * docs(dash): align `try_load_previous_cycle_entries` short-circuit comment The `None`-return condition is now \"already fully verified\", not bare \"already stored\", to match the underlying `is_cycle_fully_verified` predicate. * test(dash): replace `feed_qr_info_does_not_downgrade_already_verified_cycle` with direct gate test The old integration-style test claimed to exercise the storage gate's anti-clobber behavior, but actually never reached the gate. The synthetic fake quorums in `last_commitment_per_index` had a single-element `signers` bitvector, so `validate_structure` rejected them with `InsufficientSigners` and `feed_qr_info` returned `Err` before `store_cycle_if_fully_verified` was ever called. The post-call assertion that `rotated_quorums_per_cycle[K]` was unchanged passed for a trivial reason: nothing wrote to it. The named invariant ("does not downgrade already-verified cycle") was not under test. Replace it with a focused unit test that calls `store_cycle_if_fully_verified` directly and covers both short-circuit branches: (a) the cycle is already fully `Verified`, (b) at least one input entry is not `Verified`. The test asserts the gate returns `Ok(None)` and does not mutate `rotated_quorums_per_cycle` in either case. Net change: -115/+44 lines, 1 of 2 unused test imports also removed. Addresses CodeRabbit review comment on PR #736 #736 (comment) * docs(dash): correct `fully_verified_count` doc on `verify_rotated_quorums == false` path The comment claimed `fully_verified_count` is always `0` when `verify_rotated_quorums == false`, but the else branch at the `feed_qr_info` storage step counts every entry of `qualified_last_commitment_per_index` whose `.verified` is `Verified`. Cache hits returned by `known_qualified_quorum_entry` retain their prior `Verified` status (the gate only stores fully-Verified cycles), so the count can be non-zero even with verification disabled. Update the doc to describe the actual behavior. Addresses CodeRabbit review comment on PR #736 #736 (comment)
1 parent 68dff90 commit dd7937a

1 file changed

Lines changed: 663 additions & 101 deletions

File tree

  • dash/src/sml/masternode_list_engine

0 commit comments

Comments
 (0)