fix(dash): make feed_qr_info resilient to missing rotation CL sigs

[xdustinface]

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.

Summary by CodeRabbit

• New Features

  • Per-call quorum-rotation observability: counts of rotated quorums, newly qualified quorums, and fully-verified cycles with optional stored-cycle height.
  • Improved handling of rotated quorum signatures across cycle boundaries, with conditional storage only when a full cycle is verified and graceful skipping when signatures are incomplete.
  • Feed operation now returns detailed QR info when enabled.

• Tests

  • Expanded fixture-driven tests covering missing-signature behavior, storage gating, re-validation, and rejection paths.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b8ff4b2c-b59f-4830-b03d-6a5034256682

📥 Commits

Reviewing files that changed from the base of the PR and between 447a6e5 and 25c3347.

📒 Files selected for processing (1)

• dash/src/sml/masternode_list_engine/mod.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• dash/src/sml/masternode_list_engine/mod.rs

---

📝 Walkthrough

Walkthrough

Adds rotation-ChainLock signature slot tracking, per-call QRInfo observability, and a previous-cycle validation/storage path. feed_qr_info now returns an optional QRInfoFeedResult; rotated-quorum cycles are stored only when every entry is Verified and rotation signatures for all four slots are present. Tests and fixtures updated.

Changes

QRInfo Rotation Cycle Validation & Storage

Layer / File(s)	Summary
Data Shape dash/src/sml/masternode_list_engine/mod.rs	Added RotationChainLockSignatureSlot enum with Display and QRInfoFeedResult struct (rotated_quorum_count, newly_qualified_count, fully_verified_count, stored_cycle_height) + all_fully_verified() helper.
Rotation Signature Assembly dash/src/sml/masternode_list_engine/mod.rs	Added collect_rotation_sigs to assemble four rotation CL sig slots (h-3c,h-2c,h-c,h) and per-diff extraction; logs missing slots and returns None when incomplete.
Previous-Cycle Loading dash/src/sml/masternode_list_engine/mod.rs	Added try_load_previous_cycle_entries to extract candidate previous-cycle rotated quorum entries from the MN list at the work block when alignment requires previous-cycle handling.
Validation & Mirroring dash/src/sml/masternode_list_engine/mod.rs	Added validate_and_store_previous_cycle_quorums to validate previous-cycle rotated quorums, mirror verification statuses into quorum_statuses and the MN list, and prepare cycle storage only when every entry is Verified.
Storage Gate Helpers dash/src/sml/masternode_list_engine/mod.rs	Added is_cycle_fully_verified and store_cycle_if_fully_verified to prevent partial/downgrade storage and return optional stored cycle height.
Feed Logic & API dash/src/sml/masternode_list_engine/mod.rs	Changed MasternodeListEngine::feed_qr_info signature to Result<Option<QRInfoFeedResult>, QuorumValidationError>; added signature-alignment (current vs previous cycle), rotating-sig attachment for newly qualified quorums, counters for rotated/newly_qualified/fully_verified, and conditional cycle storage.
Tests & Fixtures dash/src/sml/masternode_list_engine/mod.rs (tests)	Added load_qrinfo_2240504_fixture helper; refactored/added tests covering missing-signature behavior, storage-gate short-circuiting, re-validation of stored rotated quorums, and updated QRInfo rejection/storage assertions.

Sequence Diagram

sequenceDiagram
    participant Test as Test Harness
    participant Engine as MasternodeListEngine
    participant QRInfo as QRInfo Input
    participant MNList as MN List State
    participant RotSigs as RotationSig Collector
    participant Validator as Quorum Validator
    participant Storage as RotatedQuorums Storage

    Test->>Engine: feed_qr_info(qr_info, work_block, diffs)
    activate Engine

    Engine->>MNList: apply snapshots & diffs
    MNList-->>Engine: updated MN list state

    Engine->>RotSigs: collect rotation sigs (h-3c, h-2c, h-c, h)
    RotSigs-->>Engine: signatures tuple or missing info

    alt signatures_present
        Engine->>Engine: determine alignment (current vs previous cycle)
        alt previous_cycle_alignment
            Engine->>Engine: try_load_previous_cycle_entries(work_block)
            Engine->>Validator: validate_and_store_previous_cycle_quorums(entries, sigs)
            Validator->>Validator: validate each rotated quorum
            Validator->>Storage: store cycle if all entries Verified
            Validator-->>Engine: validation results
        else current_cycle_alignment
            Engine->>Validator: validate current-cycle rotated quorums with sigs
            Validator-->>Engine: validation results
        end

        Engine->>Engine: compute counts (rotated, newly_qualified, fully_verified)
        Engine->>Storage: optionally store verified cycle
        Engine-->>Test: Ok(Some(QRInfoFeedResult{...}))
    else missing_signatures
        Engine-->>Test: Ok(Some(QRInfoFeedResult{... stored_cycle_height: None })) or Err(QuorumValidationError)
    end

    deactivate Engine

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I fetch four sigs from h through h‑3c,
I count and check each rotated canopy.
If every seal is Verified, I store the round;
else I skip and log the slots not found.
A rabbit cheers when cycles all sound.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: making feed_qr_info resilient to missing rotation chainlock signatures, which is the core objective of this PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/qr-info-rotation-quorums

---

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 89.28571% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.50%. Comparing base (cab6d7d) to head (25c3347).
⚠️ Report is 3 commits behind head on v0.42-dev.

Files with missing lines	Patch %	Lines
dash/src/sml/masternode_list_engine/mod.rs	89.28%	36 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           v0.42-dev     #736      +/-   ##
=============================================
+ Coverage      71.44%   71.50%   +0.05%     
=============================================
  Files            320      320              
  Lines          68825    69078     +253     
=============================================
+ Hits           49171    49393     +222     
- Misses         19654    19685      +31     

Flag	Coverage Δ
core	76.68% <89.28%> (+0.16%)	⬆️
ffi	46.25% <ø> (+0.01%)	⬆️
rpc	20.00% <ø> (ø)
spv	87.43% <ø> (-0.08%)	⬇️
wallet	70.07% <ø> (ø)

Files with missing lines	Coverage Δ
dash/src/sml/masternode_list_engine/mod.rs	82.38% <89.28%> (+2.44%)	⬆️

... and 5 files with indirect coverage changes

[coderabbitai]

Actionable comments posted: 3

🤖 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/mod.rs`:
- Line 123: Replace the panic-causing expect in the Some(...) expression by
propagating the optional values instead: instead of Some(slots.map(|(sig, _)|
sig.expect("all slots checked above"))), map the tuple iterator to the
Option<Sig> (| (sig, _) | sig) and use Option/Iterator helpers (e.g.,
collect::<Option<Vec<_>>>() or Iterator::transpose/Option::transpose) to convert
the sequence of Option<Sig> into an Option of a collection and return/propagate
None when any slot is missing; update the surrounding code to work with that
Option result rather than unwrapping with expect (locate the expression
referencing slots in masternode_list_engine::mod.rs and replace the expect usage
accordingly).
- Around line 1080-1103: The code currently writes into
rotated_quorums_per_cycle even when verify_rotated_quorums == false and not all
entries are Verified; change the insertion guard so you only call
build_cycle_quorum_map and store into self.rotated_quorums_per_cycle when the
incoming entries are fully verified. Concretely, before calling
build_cycle_quorum_map and assigning to
self.rotated_quorums_per_cycle.entry(cycle_key), require that
qualified_last_commitment_per_index.iter().all(|q| matches!(q.verified,
LLMQEntryVerificationStatus::Verified)) (or compare fully_verified_count ==
qualified_last_commitment_per_index.len()), and only proceed when that condition
is true and already_fully_verified is false; otherwise skip the insertion so
existing upgrade/early-return logic remains valid.
- Around line 556-560: The code currently derives cycle_hash via
entries.first().map(|q| q.quorum_entry.quorum_hash) which picks the first entry
in hash order and can mis-key cycles; instead locate the quorum whose
quorum_entry.quorum_index == Some(0) and use that entry's
quorum_entry.quorum_hash as the cycle key. Replace the entries.first() call with
something like entries.iter().find(|e| e.quorum_entry.quorum_index ==
Some(0)).map(|q| q.quorum_entry.quorum_hash) and keep the existing
rotated_quorums_per_cycle.contains_key(&cycle_hash) and return semantics
unchanged so previous-cycle quorums aren’t stored under the wrong key.

🪄 Autofix (Beta)

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

Run ID: b76549b6-bef9-4b10-abfc-1f4a3403d7c8

📥 Commits

Reviewing files that changed from the base of the PR and between d4b22bf and 239da59.

📒 Files selected for processing (1)

• dash/src/sml/masternode_list_engine/mod.rs

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

dash/src/sml/masternode_list_engine/mod.rs (1)

937-938: ⚡ Quick win

cycle_key derived via .first() — use quorum_index == Some(0) explicitly.

Both the verify_rotated_quorums path (line 937) and the else path (line 1084) derive the cycle boundary key from .first(), relying on the wire order of last_commitment_per_index. If a peer sends entries out of order, the wrong hash is used as the cycle key and subsequent IS-lock lookups silently fail. The same concern was raised for try_load_previous_cycle_entries and is already fixed there with an explicit index-0 find.

🛠️ Proposed fix

-        let cycle_key =
-            qualified_last_commitment_per_index.first().map(|q| q.quorum_entry.quorum_hash);
+        let cycle_key = qualified_last_commitment_per_index
+            .iter()
+            .find(|q| q.quorum_entry.quorum_index == Some(0))
+            .map(|q| q.quorum_entry.quorum_hash);

Apply the same change to the else if let Some(cycle_key) = ... arm at line 1084.

Also applies to: 1084-1085

🤖 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 937 - 938, The
cycle_key is currently taken with .first() from
qualified_last_commitment_per_index which can be wrong if entries are
out-of-order; change both places where cycle_key is derived (the binding named
cycle_key in the verify_rotated_quorums path and the other else/if arm) to
locate the index-0 entry explicitly by using .iter().find(|q|
q.quorum_entry.quorum_index == Some(0)).map(|q| q.quorum_entry.quorum_hash)
(mirroring the fix used in try_load_previous_cycle_entries) so the cycle
boundary uses the quorum_index == Some(0) entry rather than the first vector
element.

🤖 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/mod.rs`:
- Around line 1865-1867: The test
feed_qr_info_rejects_post_v20_with_missing_chainlock_signatures calls the helper
load_qrinfo_2240504_fixture which is gated by #[cfg(feature =
"quorum_validation")], causing compilation failures when the feature is
disabled; fix by adding the same #[cfg(feature = "quorum_validation")] gate to
the test (or conditionally skip the call by guarding inside the test) so the
test and helper have matching feature gating and compilation succeeds,
referencing the test function
feed_qr_info_rejects_post_v20_with_missing_chainlock_signatures and the helper
load_qrinfo_2240504_fixture.
- Around line 801-823: The variables maybe_sig_h_minus_3c, work_block_hash_h,
and maybe_sig_tip are always bound but only used under #[cfg(feature =
"quorum_validation")], causing unused-variable warnings when that feature is
off; fix this by moving their let bindings behind the same #[cfg(feature =
"quorum_validation")] guard (or splitting the surrounding code so the apply_diff
calls that produce maybe_sig_h_minus_3c and maybe_sig_tip and the assignment to
work_block_hash_h are only compiled when quorum_validation is enabled),
referencing the existing symbols (apply_diff, mn_list_diff_at_h_minus_3c,
mn_list_diff_h, mn_list_diff_tip, maybe_sig_h_minus_3c, work_block_hash_h,
maybe_sig_tip) so the bindings are only created in the same cfg scope where they
are consumed.

---

Nitpick comments:
In `@dash/src/sml/masternode_list_engine/mod.rs`:
- Around line 937-938: The cycle_key is currently taken with .first() from
qualified_last_commitment_per_index which can be wrong if entries are
out-of-order; change both places where cycle_key is derived (the binding named
cycle_key in the verify_rotated_quorums path and the other else/if arm) to
locate the index-0 entry explicitly by using .iter().find(|q|
q.quorum_entry.quorum_index == Some(0)).map(|q| q.quorum_entry.quorum_hash)
(mirroring the fix used in try_load_previous_cycle_entries) so the cycle
boundary uses the quorum_index == Some(0) entry rather than the first vector
element.

🪄 Autofix (Beta)

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

Run ID: 45f1e5d2-442c-41bc-8f5e-6dee370f4a6d

📥 Commits

Reviewing files that changed from the base of the PR and between 239da59 and 7c6e2b7.

📒 Files selected for processing (1)

• dash/src/sml/masternode_list_engine/mod.rs

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (5)

dash/src/sml/masternode_list_engine/mod.rs (5)

866-871: 💤 Low value

Field/order alignment between sigs (3-tuple) and the 4-slot rotation set is load-bearing — consider a brief comment.

sigs = [s2, s1, s0] here is the [h-2c, h-c, h] triple consumed by apply_diff(mn_list_diff_tip, ..., sigs) as previous_chain_lock_sigs: Option<[BLSSignature; 3]> for tip-rotation validation, and is independent of the 4-slot rotation_sigs assembled at lines 900–933. The two ordering conventions live a few lines apart and use very similar names; a one-line comment here noting "3-sig tuple for tip apply_diff; the separate 4-sig tuple at line 900 is for last_commitment_per_index alignment" would prevent future accidental reuse/swap.

🤖 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 866 - 871, Add a
one-line clarifying comment next to the construction of sigs (the Some([s2, s1,
s0]) from maybe_sig_h_minus_2c, maybe_sig_h_minus_c, maybe_sig_h) explaining
that this 3-element array represents [h-2c, h-c, h] and is passed as
previous_chain_lock_sigs into apply_diff (used for tip-rotation validation), and
that it is distinct from the 4-slot rotation_sigs / last_commitment_per_index
structure assembled later (lines ~900–933) so the order/size must not be
confused or reused interchangeably.

---

988-989: 💤 Low value

Invalid early-return short-circuits status mirroring for sibling entries.

When any rotated quorum returns Invalid, feed_qr_info returns immediately, so:

• quorum_statuses is not updated for any sibling rotated quorum in this last_commitment_per_index (the mirror loop at 994–1013 never runs).
• masternode_lists updates collected from prior loops are also discarded.
• The previous-cycle path at 882–885 already ran and is preserved (this is consistent with the test at 2025–2032).

This is defensible — an Invalid signal in any rotated quorum should poison the whole cycle — but it also means observability for the rejected cycle is asymmetric (some entries get statuses mirrored, others don't, depending on iteration order). If the intent is "reject the whole cycle atomically", consider mirroring Skipped(InvalidSiblingInCycle) (or similar) for the remaining entries before returning, so the engine's view is consistent. Otherwise a brief comment noting the asymmetric mirror-on-error would help future readers.

🤖 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 988 - 989,
feed_qr_info currently returns immediately when any rotated quorum yields
Invalid, which leaves quorum_statuses and masternode_lists inconsistent for
sibling entries in last_commitment_per_index; update feed_qr_info so that before
returning on an Invalid from a rotated quorum it mirrors a consistent status for
the remaining sibling entries (e.g. set their quorum_statuses to
Skipped(InvalidSiblingInCycle) or a similar sentinel) and apply the same
masternode_lists updates so the cycle is rejected atomically, or if you prefer
not to change behavior add a clear comment in feed_qr_info explaining the
asymmetric mirroring on early-return and why it’s acceptable.

---

2079-2092: 💤 Low value

Synthetic sibling quorums use all-zero pubkeys/sigs — depending on validation order, they may resolve to Unknown/Skipped, not exercise the gate.

The siblings (indices 1..active_count) carry zero pubkey, zero all_commitment_aggregated_signature, and a constructed quorum_hash whose work block is unknown to the engine. Whether validate_rotation_cycle_quorums_validation_statuses returns Skipped (missing context) or Invalid (signature mismatch) for them determines whether feed_qr_info reaches the storage gate at line 1124 or returns early at line 988. Either path keeps the existing cycle intact, but only the Skipped path actually exercises the store_cycle_if_fully_verified downgrade-protection — the Invalid path returns before storage runs.

Combined with the let _ = at line 2144, this test could pass without ever exercising the gate. Asserting the call's success (or asserting qualified_last_commitment_per_index siblings settle as Skipped/Unknown rather than Invalid) would tighten the coverage.

🤖 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 2079 - 2092, The
test creates sibling QuorumEntry items in last_commitment_per_index with
all-zero pubkeys/signatures and unknown quorum_hash, which can yield either
Skipped or Invalid and may let the test skip exercising
store_cycle_if_fully_verified; fix by making the synthetic siblings
deterministically validate as Skipped or valid: either (A) populate their
quorum_public_key and all_commitment_aggregated_signature with non-zero dummy
values and ensure quorum_hash maps to a known work block so
validate_rotation_cycle_quorums_validation_statuses returns a non-Invalid
status, or (B) explicitly assert the result of feed_qr_info (remove the `let _
=` usage) and/or assert that qualified_last_commitment_per_index siblings have
the expected statuses (Skipped/Unknown) before proceeding so the test actually
exercises store_cycle_if_fully_verified and the downgrade-protection gate.

---

1117-1129: 💤 Low value

verify_rotated_quorums == false storage path now only stores when entries are already cached as Verified — confirm this is the intended trust path.

In this branch no validation runs; fully_verified_count only counts entries that arrived already-Verified via known_qualified_quorum_entry. Combined with store_cycle_if_fully_verified's all_entries_verified gate, this means a cycle gets stored from a verify_rotated_quorums=false call only when every rotated entry was previously verified in a prior call. That's a sound trust property (no unverified writes), but it's a behavior change from before: a false call no longer ever inserts a fresh cycle. Worth confirming this matches the intent expressed in the PR objectives ("cycle is stored only when every entry is Verified").

🤖 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 1117 - 1129, The
branch taken when verify_rotated_quorums == false only counts entries already
marked LLMQEntryVerificationStatus::Verified (via known_qualified_quorum_entry)
and then calls store_cycle_if_fully_verified(cycle_key,
qualified_last_commitment_per_index, rotation_quorum_type), so a cycle will be
stored only if every rotated entry was previously Verified; to fix, either (A)
explicitly run the same verification path used when verify_rotated_quorums ==
true before counting/storing (i.e. validate entries here and update their status
so fully_verified_count reflects newly-verified entries), or (B) make the
storage call accept partial verification by changing
store_cycle_if_fully_verified semantics, or (preferred) add an explicit
comment/doc and a test asserting that verify_rotated_quorums == false never
inserts fresh cycles; update code around qualified_last_commitment_per_index,
fully_verified_count, known_qualified_quorum_entry, and
store_cycle_if_fully_verified to implement the chosen approach.

---

768-773: ⚡ Quick win

Update the sync_manager.rs caller to act on the new QRInfoFeedResult fields.

The feed_qr_info call at dash-spv/src/sync/masternodes/sync_manager.rs:238 discards the success-side Option<QRInfoFeedResult> and only retries when a hard tip-level ChainLock error occurs. Two follow-ups are needed to realize the observability benefit:

• Check all_fully_verified() to drive the existing qrinfo_retry_count logic when a QRInfo settles as "applied but not fully verified". Currently only RequiredRotatedChainLockSigNotPresent(0, _) (tip offset) triggers retry; partial-verification states are silently accepted.
• Surface fully_verified_count, rotated_quorum_count, and stored_cycle_height in metrics or logs so incomplete verification cycles are visible operationally.

Without this caller update, cycles may remain unstored on the SPV side with no feedback loop.

🤖 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 768 - 773, The
caller of MasternodeListEngine::feed_qr_info (in sync_manager.rs around the
current callsite) ignores the success-side QRInfoFeedResult; update that caller
to inspect the returned Option<QRInfoFeedResult> and (1) use
QRInfoFeedResult::all_fully_verified() to decide whether to increment/trigger
the existing qrinfo_retry_count path (i.e., treat "applied but not fully
verified" the same as the current RequiredRotatedChainLockSigNotPresent(0, _)
retry condition) instead of silently accepting it, and (2) extract and emit/log
the QRInfoFeedResult fields fully_verified_count, rotated_quorum_count, and
stored_cycle_height to metrics or logging so incomplete verification cycles are
observable operationally. Ensure you reference feed_qr_info, QRInfoFeedResult,
all_fully_verified(), and the qrinfo_retry_count retry logic when making the
change.

🤖 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/mod.rs`:
- Around line 2143-2154: The test currently swallows the result of
engine.feed_qr_info(thin_qr_info, false, true) which can mask failures and
weaken the anti-clobber assertion; update the test to explicitly assert the call
succeeds (e.g., unwrap/expect the Ok variant) so that the subsequent check of
engine.rotated_quorums_per_cycle.get(&cycle_key) against original_cycle truly
verifies the gate prevented a downgrade, or, if an Err is acceptable in some
configs, add an explicit comment and assert that rotated_quorums_per_cycle was
unchanged even on Err; target the engine.feed_qr_info call, the
rotated_quorums_per_cycle lookup by cycle_key, original_cycle comparison, and
any behavior around store_cycle_if_fully_verified in your change.

---

Nitpick comments:
In `@dash/src/sml/masternode_list_engine/mod.rs`:
- Around line 866-871: Add a one-line clarifying comment next to the
construction of sigs (the Some([s2, s1, s0]) from maybe_sig_h_minus_2c,
maybe_sig_h_minus_c, maybe_sig_h) explaining that this 3-element array
represents [h-2c, h-c, h] and is passed as previous_chain_lock_sigs into
apply_diff (used for tip-rotation validation), and that it is distinct from the
4-slot rotation_sigs / last_commitment_per_index structure assembled later
(lines ~900–933) so the order/size must not be confused or reused
interchangeably.
- Around line 988-989: feed_qr_info currently returns immediately when any
rotated quorum yields Invalid, which leaves quorum_statuses and masternode_lists
inconsistent for sibling entries in last_commitment_per_index; update
feed_qr_info so that before returning on an Invalid from a rotated quorum it
mirrors a consistent status for the remaining sibling entries (e.g. set their
quorum_statuses to Skipped(InvalidSiblingInCycle) or a similar sentinel) and
apply the same masternode_lists updates so the cycle is rejected atomically, or
if you prefer not to change behavior add a clear comment in feed_qr_info
explaining the asymmetric mirroring on early-return and why it’s acceptable.
- Around line 2079-2092: The test creates sibling QuorumEntry items in
last_commitment_per_index with all-zero pubkeys/signatures and unknown
quorum_hash, which can yield either Skipped or Invalid and may let the test skip
exercising store_cycle_if_fully_verified; fix by making the synthetic siblings
deterministically validate as Skipped or valid: either (A) populate their
quorum_public_key and all_commitment_aggregated_signature with non-zero dummy
values and ensure quorum_hash maps to a known work block so
validate_rotation_cycle_quorums_validation_statuses returns a non-Invalid
status, or (B) explicitly assert the result of feed_qr_info (remove the `let _
=` usage) and/or assert that qualified_last_commitment_per_index siblings have
the expected statuses (Skipped/Unknown) before proceeding so the test actually
exercises store_cycle_if_fully_verified and the downgrade-protection gate.
- Around line 1117-1129: The branch taken when verify_rotated_quorums == false
only counts entries already marked LLMQEntryVerificationStatus::Verified (via
known_qualified_quorum_entry) and then calls
store_cycle_if_fully_verified(cycle_key, qualified_last_commitment_per_index,
rotation_quorum_type), so a cycle will be stored only if every rotated entry was
previously Verified; to fix, either (A) explicitly run the same verification
path used when verify_rotated_quorums == true before counting/storing (i.e.
validate entries here and update their status so fully_verified_count reflects
newly-verified entries), or (B) make the storage call accept partial
verification by changing store_cycle_if_fully_verified semantics, or (preferred)
add an explicit comment/doc and a test asserting that verify_rotated_quorums ==
false never inserts fresh cycles; update code around
qualified_last_commitment_per_index, fully_verified_count,
known_qualified_quorum_entry, and store_cycle_if_fully_verified to implement the
chosen approach.
- Around line 768-773: The caller of MasternodeListEngine::feed_qr_info (in
sync_manager.rs around the current callsite) ignores the success-side
QRInfoFeedResult; update that caller to inspect the returned
Option<QRInfoFeedResult> and (1) use QRInfoFeedResult::all_fully_verified() to
decide whether to increment/trigger the existing qrinfo_retry_count path (i.e.,
treat "applied but not fully verified" the same as the current
RequiredRotatedChainLockSigNotPresent(0, _) retry condition) instead of silently
accepting it, and (2) extract and emit/log the QRInfoFeedResult fields
fully_verified_count, rotated_quorum_count, and stored_cycle_height to metrics
or logging so incomplete verification cycles are observable operationally.
Ensure you reference feed_qr_info, QRInfoFeedResult, all_fully_verified(), and
the qrinfo_retry_count retry logic when making the change.

🪄 Autofix (Beta)

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

Run ID: c81e136a-1295-4232-ae9c-97c7f9d19bc6

📥 Commits

Reviewing files that changed from the base of the PR and between 7c6e2b7 and a0a0c19.

📒 Files selected for processing (1)

• dash/src/sml/masternode_list_engine/mod.rs

[coderabbitai]

Actionable comments posted: 1

🤖 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/mod.rs`:
- Around line 82-86: The field doc for fully_verified_count incorrectly states
it is "Always 0 when verify_rotated_quorums == false"; update the comment to
reflect that cached entries can contribute to the count: explain that when
verify_rotated_quorums is false the code still computes fully_verified_count
from qualified_last_commitment_per_index which may include prior Verified
statuses from known_qualified_quorum_entry (cache hits), so fully_verified_count
can be > 0 in that case. Mention the involved symbols fully_verified_count,
verify_rotated_quorums, qualified_last_commitment_per_index, and
known_qualified_quorum_entry so readers can locate the logic.

🪄 Autofix (Beta)

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

Run ID: 2e1b215a-f739-4515-b03f-10721d053158

📥 Commits

Reviewing files that changed from the base of the PR and between a0a0c19 and 447a6e5.

📒 Files selected for processing (1)

• dash/src/sml/masternode_list_engine/mod.rs

[xdustinface]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[QuantumExplorer]

Spent some time looking at this one, seems good, but it would be very hard for me to know if it is correct without spending a lot more time on it.