fix: store historical rotating quorums by cycle boundary hash

[xdustinface]

This fixes issues in InstantSend lock verification where we miss the selected quorums. It adds infrastructure for storing historical rotating quorums keyed by cycle boundary hash instead of quorum hash. This enables InstantLock verification for previous cycles by:

• Adding WORK_DIFF_DEPTH constant (8 blocks offset to cycle boundary)
• Adding store_rotating_quorums_for_cycle() to extract and store quorums
• Collecting work block hashes from QRInfo diffs (h-4c, h-3c, h-2c, h-c)
• Using cycle boundary hash as the key for rotated_quorums_per_cycle

Summary by CodeRabbit

• Bug Fixes
  • Improved rotating quorum management and storage logic to ensure quorums are properly organized by cycle and retrieved correctly during validation.
  • Refined quorum validation sequencing to ensure masternode lists contain all necessary data before quorum processing.
  • Enhanced cycle boundary handling for more reliable quorum cycling operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

A public constant WORK_DIFF_DEPTH is introduced alongside logic to store rotating quorums per cycle, keyed by cycle boundary hash. The implementation computes cycle boundaries, extracts quorums from the masternode list, and integrates rotating quorum storage into the QRInfo processing and quorum validation workflows.

Changes

Cohort / File(s)	Summary
Rotating Quorum Storage Per Cycle dash/src/sml/masternode_list_engine/mod.rs	Added public constant WORK_DIFF_DEPTH (value 8). Implemented per-cycle rotating quorum storage keyed by cycle boundary hash, including cycle boundary computation, quorum extraction from masternode list, and integration into QRInfo processing flow. Refined quorum validation to use cycle boundary hash directly and added verification status propagation updates.

Sequence Diagram

sequenceDiagram
    participant QRInfo as QRInfo Processing
    participant MNList as Masternode List Engine
    participant CycleBound as Cycle Boundary Calculator
    participant QRStorage as Rotating Quorum Storage
    participant VerifyStatus as Verification Status

    QRInfo->>MNList: Apply QRInfo diffs (series)
    MNList->>MNList: Update masternode state
    QRInfo->>CycleBound: Compute cycle boundary hash<br/>(from work block height)
    CycleBound-->>QRInfo: Return cycle boundary hash
    QRInfo->>MNList: Extract rotating quorums<br/>(at work block height)
    MNList-->>QRInfo: Return rotating quorums
    QRInfo->>QRStorage: Store rotating quorums<br/>(keyed by cycle boundary hash)
    QRStorage->>QRStorage: Check if cycle already exists<br/>(skip if present)
    QRStorage->>QRStorage: Store non-empty quorum set
    QRStorage-->>QRInfo: Storage complete
    QRInfo->>VerifyStatus: Propagate verification status<br/>(across heights)
    VerifyStatus-->>QRInfo: Status update applied

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 The cycles spin, the quorums dance,
Stored by hash with measured stance,
Eight blocks deep the work blocks fall,
Per-cycle wisdom holds them all! 🔄✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 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: storing historical rotating quorums by cycle boundary hash instead of quorum hash, which directly addresses the core fix implemented in the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/store-historical-cycles

---

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.}

[PastaPastaPasta]

@coderabbitai review

Ensure you take into consideration this DIP https://github.com/dashpay/dips/blob/master/dip-0022.md and it's prior work, to understand system design.

[coderabbitai]

🧩 Analysis chain

@PastaPastaPasta, I'll review the PR with DIP-0022 in mind. Let me first fetch the DIP specification and examine the changes.

---

🏁 Script executed:

#!/bin/bash

# Fetch DIP-0022 content
echo "=== Fetching DIP-0022 ==="
curl -s https://raw.githubusercontent.com/dashpay/dips/master/dip-0022.md

echo -e "\n\n=== Fetching DIP-0006 (Long-Living Masternode Quorums) ==="
curl -s https://raw.githubusercontent.com/dashpay/dips/master/dip-0006.md

echo -e "\n\n=== Checking for DIP-0024 (mentioned in learnings) ==="
curl -s https://raw.githubusercontent.com/dashpay/dips/master/dip-0024.md

Length of output: 84079

---

🏁 Script executed:

#!/bin/bash

# Get the list of changed files in the PR
echo "=== Changed files in PR ==="
gh pr view 409 --json files --jq '.files[].path'

echo -e "\n\n=== PR diff ==="
gh pr diff 409

Length of output: 8821

---

🏁 Script executed:

#!/bin/bash

# Find relevant files related to rotating quorums and InstantSend
echo "=== Finding rotating quorum related files ==="
fd -e rs | rg -i "quorum|instant"

Length of output: 812

---

@PastaPastaPasta, I'll review this PR with DIP-0022 and DIP-0024 specifications in mind. Let me examine the implementation details.

---

🏁 Script executed:

#!/bin/bash

# Examine the rotated_quorums_per_cycle usage and structure
echo "=== Examining rotated_quorums_per_cycle structure ==="
rg -n -A5 -B5 "rotated_quorums_per_cycle" dash/src/sml/masternode_list_engine/mod.rs | head -100

echo -e "\n\n=== Check InstantLock verification code ==="
cat dash-spv/src/validation/instantlock.rs

Length of output: 12201

---

🏁 Script executed:

#!/bin/bash

# Look at QRInfo structure to understand the diff components
echo "=== Finding QRInfo structure definition ==="
rg -n "struct.*QRInfo" --type rust -A20

echo -e "\n\n=== Check cycle boundary and work block usage ==="
rg -n "cycle.*boundary|WORK_DIFF_DEPTH" --type rust -C3

Length of output: 10580

---

🏁 Script executed:

#!/bin/bash

# Examine how qualified quorum entries are used
echo "=== Check QualifiedQuorumEntry structure ==="
rg -n "struct.*QualifiedQuorumEntry" --type rust -A10

echo -e "\n\n=== Check masternode list quorums structure ==="
rg -n "pub quorums:" --type rust -C5

Length of output: 3144

---

🏁 Script executed:

#!/bin/bash

# Check how verify_is_lock uses the stored quorums
echo "=== Find verify_is_lock implementation ==="
rg -n "fn verify_is_lock" --type rust -A30

echo -e "\n\n=== Check InstantLock structure and cyclehash field ==="
rg -n "struct InstantLock|pub cyclehash" --type rust -C3

Length of output: 5282

---

🏁 Script executed:

#!/bin/bash

# Check how quorums are retrieved by cycle hash for verification
echo "=== Check quorum retrieval by cycle hash ==="
rg -n "rotated_quorums_per_cycle.*get\(|get.*cyclehash|cycle.*hash.*get" --type rust -C5

echo -e "\n\n=== Check if there are any tests for this functionality ==="
rg -n "#\[test\]|#\[cfg\(test\)\]" dash/src/sml/masternode_list_engine/mod.rs -A3 | head -50

Length of output: 4839

---

🏁 Script executed:

#!/bin/bash

# Look for any documentation or comments about the cycle boundary vs work block distinction
echo "=== Check comments about cycle boundary and work block ==="
rg -n "cycle.boundary|work.block|h-[0-9]c" --type rust -C2 dash/src/sml/masternode_list_engine/mod.rs

echo -e "\n\n=== Check the old implementation (what was it keyed by before?) ==="
git diff HEAD~1 dash/src/sml/masternode_list_engine/mod.rs | grep -A5 -B5 "rotated_quorums_per_cycle"

Length of output: 8707

---

@PastaPastaPasta, I've completed my review of PR #409 with DIP-0022 and DIP-0024 in mind. This is an excellent fix that properly implements cycle-based quorum storage for historical InstantLock verification.

✅ Correct Implementation of DIP-0022 & DIP-0024

The PR correctly addresses the requirement that InstantLock messages must be verified using cycleHash rather than quorumHash. Here's what I found:

Key Strengths

1. WORK_DIFF_DEPTH Constant - Correctly implements the 8-block offset between cycle boundary and work block as specified in DIP-0024

2. Cycle Boundary Hash as Key - The critical change from using quorum_entry.quorum_hash to cycle_boundary_hash aligns perfectly with DIP-0022's requirement that "any node can follow the steps required to determine the appropriate quorumHash" using the cycleHash

3. Historical Quorum Storage - Storing quorums from h-c, h-2c, h-3c, and h-4c enables verification of InstantLocks from previous cycles, which is essential for Platform State Transition validation as described in DIP-0022

4. Dual Storage Strategy:

   • Historical cycles: Extracted from masternode lists at work block heights
   • Current cycle: Populated from lastCommitmentPerIndex with validation status

   This distinction is important because lastCommitmentPerIndex contains "the most recent commitment for each quorumIndex" which may include older quorums if DKG sessions failed

Architecture Observations

Cycle Boundary Calculation

let cycle_boundary_height = work_block_height + WORK_DIFF_DEPTH;

This correctly maps the work block (where masternode lists are selected) to the cycle boundary (where the cycle begins), matching DIP-0024's requirement that "the masternode list at this height is used 8 blocks prior to the current height"

Duplicate Prevention

• contains_key() check prevents re-storing historical cycles
• clear() call allows current cycle updates on repeated feed_qr_info() calls
• This prevents accumulation while allowing state updates

Minor Suggestions

1. Enhanced Documentation: Consider adding a comment explaining why historical and current quorums use different storage paths (masternode list extraction vs lastCommitmentPerIndex validation)

2. Test Coverage: Verify tests exist for:

   • InstantLock verification using historical cycle hashes
   • Correct cycle boundary calculation from work blocks
   • Quorum retrieval by cycle hash for different DKG failure scenarios

3. Feature Flag Clarity: The removal of #[cfg(feature = "quorum_validation")] from the NetworkLLMQExt import is correct since isd_llmq_type() is now used in the non-feature-gated store_rotating_quorums_for_cycle() method

Questions

1. Edge Case: What happens if feed_qr_info() is called multiple times for the same height h? The clear() prevents accumulation for the current cycle, but are there any performance implications?

2. Masternode List Availability: The code gracefully handles missing masternode lists with early returns. Are there scenarios where a work block's masternode list might legitimately be absent after applying its diff?

Summary

This PR correctly implements the critical infrastructure needed for deterministic InstantSend lock verification per DIP-0022 and DIP-0024. The shift from quorum hash to cycle hash as the storage key is essential because "any quorum could sign any islock even if it were not responsible to do so for that quorum cycle" under the old approach. The new implementation ensures that "nodes can follow the steps required to determine the appropriate quorumHash and verify the signature" for InstantLocks from any cycle.

---

🧠 Learnings used

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Maintain backward compatibility for serialized wallets and monitor for new DIPs affecting wallet structure when updating derivation paths

---

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T14:54:24.111Z
Learning: This codebase is not intended for consensus-critical validation; do not rely on exact Dash Core consensus behavior

---

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Prepare support for future Dash upgrades including Schnorr/Taproot support, descriptor wallets, multi-signature account types, and Lightning Network payment channels

---

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T14:53:56.708Z
Learning: This library should NOT be used for consensus code and cannot replicate exact consensus behavior of Dash Core

---

Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 56
File: dash/src/sml/masternode_list_engine/message_request_verification.rs:98-99
Timestamp: 2025-02-27T05:44:42.338Z
Learning: In the Dash codebase, quorum selection for InstantLock verification uses a bit-shifting operation with (64 - n - 1) to extract n bits starting from the second-highest bit of a 64-bit selection hash. The exact reasoning for the extra "-1" isn't documented, but it matches the original C++ implementation for DIP-24 quorum selection.

✅ 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.

[PastaPastaPasta]

@coderabbitai 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.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@dash/src/sml/masternode_list_engine/mod.rs`:
- Around line 708-713: The current call to get_hash(...) returns an Err
constructed with
QuorumValidationError::RequiredBlockNotPresent(BlockHash::all_zeros(), ...)
which uses an all-zero sentinel that is misleading; update the error to either
add and use a new variant (e.g., RequiredBlockHeightNotPresent(CoreBlockHeight,
String)) or pass a meaningful related hash (for example work_block_hash) instead
of BlockHash::all_zeros(), and update the match arms that handle
QuorumValidationError::RequiredBlockNotPresent (or add handling for the new
variant) so downstream code sees the real context (reference symbols:
block_container.get_hash, cycle_boundary_height,
QuorumValidationError::RequiredBlockNotPresent, BlockHash::all_zeros(),
work_block_hash).

🧹 Nitpick comments (3)

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

412-449: Silent early returns may mask data inconsistencies.

All four early-return paths silently discard failures. While this is reasonable for historical cycles where data might genuinely be absent, it can make debugging harder when the block container or masternode list should have been populated but wasn't (e.g., a prior apply_diff failed silently or the block height wasn't fed).

Consider returning a bool (or logging at trace/debug level) so callers or operators can distinguish "nothing to store" from "unexpected missing data."

---

671-675: Nit: work_block_hash and mn_list_diff_at_h_block_hash hold the same value.

Both capture mn_list_diff_h.block_hash under #[cfg(feature = "quorum_validation")]. This is fine for readability since they serve different semantic roles, but worth noting they're always identical.

---

1208-1213: Consider adding targeted tests for the new cycle boundary logic.

The existing tests implicitly exercise store_rotating_quorums_for_cycle through feed_qr_info, but there's no direct test for:

• Cycle boundary height calculation (work_block_height + WORK_DIFF_DEPTH)
• The contains_key guard preventing duplicate storage
• Behavior when historical masternode lists or block hashes are absent

A focused unit test for store_rotating_quorums_for_cycle would improve confidence in edge cases (e.g., missing block mappings, empty quorum lists, repeated calls).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

BlockHash::all_zeros() in error is semantically misleading.

RequiredBlockNotPresent expects the missing block's hash, but here the hash is what's missing. Any error handler pattern-matching on the hash value would see the genesis-like all-zeros sentinel instead of meaningful data. The format string mitigates this for logs, but downstream match arms could be confused.

Consider introducing a dedicated error variant (e.g., RequiredBlockHeightNotPresent(CoreBlockHeight, String)) or passing work_block_hash with a message noting the derived height wasn't found, since the work block is the closest related known hash.

🤖 Prompt for AI Agents

In `@dash/src/sml/masternode_list_engine/mod.rs` around lines 708 - 713, The
current call to get_hash(...) returns an Err constructed with
QuorumValidationError::RequiredBlockNotPresent(BlockHash::all_zeros(), ...)
which uses an all-zero sentinel that is misleading; update the error to either
add and use a new variant (e.g., RequiredBlockHeightNotPresent(CoreBlockHeight,
String)) or pass a meaningful related hash (for example work_block_hash) instead
of BlockHash::all_zeros(), and update the match arms that handle
QuorumValidationError::RequiredBlockNotPresent (or add handling for the new
variant) so downstream code sees the real context (reference symbols:
block_container.get_hash, cycle_boundary_height,
QuorumValidationError::RequiredBlockNotPresent, BlockHash::all_zeros(),
work_block_hash).