Skip to content

refactor: derive rotation cycle key from the cycle boundary block #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xdustinface wants to merge 1 commit into
base: fix/rotated-quorums-index
Choose a base branch
from
Open

refactor: derive rotation cycle key from the cycle boundary block #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 39 additions & 15 deletions dash/src/sml/masternode_list_engine/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,19 @@ impl MasternodeListEngine {
.map(|quorum_entry| quorum_entry.llmq_type)
.unwrap_or(self.network.isd_llmq_type());

// IS locks reference the cycle boundary block hash as their `cyclehash` field,
// so `rotated_quorums_per_cycle` must be keyed by that hash.
#[cfg(feature = "quorum_validation")]
let cycle_boundary_hash = {
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion [medium confidence]: cycle_boundary_hash lookup now fails the call even when verify_rotated_quorums is false

The cycle_boundary_hash block is evaluated unconditionally (before the if verify_rotated_quorums branch), so any caller that passes verify_rotated_quorums: false — e.g. during initial sync when the cycle boundary block may not yet be in the container — will now receive RequiredBlockNotPresent where previously they would have succeeded with a no-op insertion. The PR description frames this as intentional ('fails loudly'), but it silently tightens the precondition of feed_qr_info without a corresponding update to documentation or caller sites. At minimum, document that cycle_boundary_height must be present in block_container regardless of the verify_rotated_quorums flag.

AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 637,
  "severity": "suggestion",
  "confidence": "medium",
  "flaggedBy": [
    "Security & Safety"
  ],
  "title": "cycle_boundary_hash lookup now fails the call even when verify_rotated_quorums is false"
}

let cycle_boundary_height = h_height + WORK_DIFF_DEPTH;
*self.block_container.get_hash(&cycle_boundary_height).ok_or(
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion [medium confidence]: No test for missing cycle boundary block (error path)

The new cycle_boundary_hash lookup at h_height + WORK_DIFF_DEPTH returns QuorumValidationError::RequiredBlockNotPresent when the block is absent, but there is no test exercising this failure path. This is the primary behavioral change introduced by the PR — previously the code silently skipped insertion when no first entry existed; now it hard-fails. Without a test, a regression that re-introduces silent skipping or changes the error variant would go undetected.

Suggested fix
Suggested change
*self.block_container.get_hash(&cycle_boundary_height).ok_or(
#[test]
fn feed_qr_info_returns_error_when_cycle_boundary_block_missing() {
// Build engine whose block_container does NOT contain the cycle boundary
// block, then assert that feed_qr_info returns RequiredBlockNotPresent.
let mut engine = build_engine_without_cycle_boundary_block();
let result = engine.feed_qr_info::<fn(&BlockHash) -> Result<u32, _>>(
qr_info, true, true, None,
);
assert!(matches!(
result,
Err(QuorumValidationError::RequiredBlockNotPresent(..))
));
}
AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 639,
  "severity": "suggestion",
  "confidence": "medium",
  "flaggedBy": [
    "Testing & Coverage"
  ],
  "title": "No test for missing cycle boundary block (error path)",
  "fix": "#[test]\nfn feed_qr_info_returns_error_when_cycle_boundary_block_missing() {\n    // Build engine whose block_container does NOT contain the cycle boundary\n    // block, then assert that feed_qr_info re"
}

QuorumValidationError::RequiredBlockNotPresent(
BlockHash::all_zeros(),
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion [high confidence]: BlockHash::all_zeros() sentinel breaks RequiredBlockNotPresent contract

All existing call sites of QuorumValidationError::RequiredBlockNotPresent pass the actual missing block hash as the first argument (e.g. lines 574-576 in this file, lines 37-39 in rotated_quorum_construction.rs). The new lookup is by height, so the hash is unavailable — but BlockHash::all_zeros() is passed instead. update_quorum_status in qualified_quorum_entry.rs:70 pattern-matches on this error and stores the extracted hash into LLMQEntryVerificationStatus::Skipped(UnknownBlock(block_hash)), so callers observing the skipped status will see the null sentinel rather than the actual missing block identity, making diagnostics misleading. The right fix is a separate error variant, e.g. RequiredBlockAtHeightNotPresent(u32, String), so callers can differentiate hash-based from height-based lookup failures.

Suggested fix
Suggested change
BlockHash::all_zeros(),
// In QuorumValidationError:
#[error("Required block at height {0} not present: {1}")]
RequiredBlockAtHeightNotPresent(u32, String),
// At the call site:
QuorumValidationError::RequiredBlockAtHeightNotPresent(
cycle_boundary_height,
format!("cycle boundary at height {cycle_boundary_height}"),
)
AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 641,
  "severity": "suggestion",
  "confidence": "high",
  "flaggedBy": [
    "Security & Safety",
    "Architecture & Design"
  ],
  "title": "BlockHash::all_zeros() sentinel breaks RequiredBlockNotPresent contract",
  "fix": "// In QuorumValidationError:\n#[error(\"Required block at height {0} not present: {1}\")]\nRequiredBlockAtHeightNotPresent(u32, String),\n\n// At the call site:\nQuorumValidationError::RequiredBlockAtHeightN"
}

format!("cycle boundary at height {cycle_boundary_height}"),
),
)?
};

if let Some((quorum_snapshot_at_h_minus_4c, mn_list_diff_at_h_minus_4c)) =
quorum_snapshot_and_mn_list_diff_at_h_minus_4c
{
Expand Down Expand Up @@ -725,9 +738,6 @@ impl MasternodeListEngine {
LLMQEntryVerificationStatus,
)> = Vec::new();

let cycle_key =
qualified_last_commitment_per_index.first().map(|q| q.quorum_entry.quorum_hash);

for rotated_quorum in qualified_last_commitment_per_index.iter_mut() {
tracing::debug!(
" Current cycle quorum: hash={}, raw_quorum_index={:?}, map_key={:?}",
Expand Down Expand Up @@ -762,13 +772,9 @@ impl MasternodeListEngine {
*status = rotated_quorum.verified.clone();
}

if let Some(key) = cycle_key {
let cycle_map = build_cycle_quorum_map(
qualified_last_commitment_per_index,
rotation_quorum_type,
)?;
*self.rotated_quorums_per_cycle.entry(key).or_default() = cycle_map;
}
let cycle_map =
build_cycle_quorum_map(qualified_last_commitment_per_index, rotation_quorum_type)?;
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion [medium confidence]: Removing empty-list guard creates spurious empty entries in rotated_quorums_per_cycle

The old code guarded both the if-block write (if let Some(key) = cycle_key { ... }) and the else if write with qualified_last_commitment_per_index.first().map(...), silently skipping the write when the list is empty. Both guards are now gone: build_cycle_quorum_map is called unconditionally and an entry is always written under cycle_boundary_hash. If qualified_last_commitment_per_index is ever empty (e.g., a diff that carries no rotation commitments), rotated_quorums_per_cycle now gets an entry with an empty map, whereas previously no entry existed. Consumers that distinguish contains_key from !is_empty() on the value will observe different behaviour. Verify that build_cycle_quorum_map is correct on empty input and that an empty-map entry is semantically equivalent to an absent entry, or restore the guard.

Suggested fix
Suggested change
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
// Option 1 – guard the whole block (mirrors old semantics)
if !qualified_last_commitment_per_index.is_empty() {
let cycle_map = build_cycle_quorum_map(qualified_last_commitment_per_index, rotation_quorum_type)?;
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
}
AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 777,
  "severity": "suggestion",
  "confidence": "medium",
  "flaggedBy": [
    "Correctness & Logic",
    "Architecture & Design"
  ],
  "title": "Removing empty-list guard creates spurious empty entries in rotated_quorums_per_cycle",
  "fix": "// Option 1 – guard the whole block (mirrors old semantics)\nif !qualified_last_commitment_per_index.is_empty() {\n    let cycle_map = build_cycle_quorum_map(qualified_last_commitment_per_index, rotatio"
}


// Apply collected updates after iteration to avoid borrow conflicts
for (heights, quorum_type, quorum_hash, new_status) in updates {
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 Required [high confidence]: cycle_boundary_hash used outside its #[cfg(feature = "quorum_validation")] guard

The variable cycle_boundary_hash is defined only under #[cfg(feature = "quorum_validation")] (lines 634-648), but it is used unconditionally inside both branches of the if let Some(...) { ... } else { ... } block (lines ~781 and ~875). When the crate is compiled without the quorum_validation feature the compiler will not emit the let binding but will still encounter the bare identifier, resulting in a "cannot find value cycle_boundary_hash in this scope" compilation error. Either wrap the entire if/else block in the same feature gate, or extract the cycle-boundary lookup into a shared helper that is available regardless of features.

Suggested fix
Suggested change
for (heights, quorum_type, quorum_hash, new_status) in updates {
#[cfg(feature = "quorum_validation")]
{
let cycle_boundary_hash = { /* ... */ };
if let Some((snap, diff)) = quorum_snapshot_and_mn_list_diff_at_h_minus_4c {
/* ... */
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
} else {
let cycle_map = build_cycle_quorum_map(qualified_last_commitment_per_index, rotation_quorum_type)?;
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
}
}
#[cfg(not(feature = "quorum_validation"))]
{ /* existing non-validation path */ }
AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 781,
  "severity": "required",
  "confidence": "high",
  "flaggedBy": [
    "Architecture & Design"
  ],
  "title": "`cycle_boundary_hash` used outside its `#[cfg(feature = \"quorum_validation\")]` guard",
  "fix": "#[cfg(feature = \"quorum_validation\")]\n{\n    let cycle_boundary_hash = { /* ... */ };\n    if let Some((snap, diff)) = quorum_snapshot_and_mn_list_diff_at_h_minus_4c {\n        /* ... */\n        *self.ro"
}

Expand Down Expand Up @@ -859,12 +865,10 @@ impl MasternodeListEngine {
}
Copy link
Copy Markdown

@manki-review manki-review Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion [medium confidence]: New else-branch calls build_cycle_quorum_map unconditionally; no test for empty qualified_last_commitment_per_index

Previously the else-branch was guarded by if let Some(cycle_key) = qualified_last_commitment_per_index.first().map(...), so an empty slice was silently skipped. The refactored code removes that guard and always calls build_cycle_quorum_map. The behavior of build_cycle_quorum_map on an empty slice is not tested, and if it returns an empty map or an error, that is now a new observable change with no regression coverage.

Suggested fix
Suggested change
}
Add a unit test that feeds a `qr_info` whose `qualified_last_commitment_per_index` is empty (no quorum-snapshot branch) and asserts the expected outcome — either an empty map entry is created or an error is returned, whichever is correct per the DIP spec.
AI context 
{
  "file": "dash/src/sml/masternode_list_engine/mod.rs",
  "line": 865,
  "severity": "suggestion",
  "confidence": "medium",
  "flaggedBy": [
    "Testing & Coverage"
  ],
  "title": "New else-branch calls build_cycle_quorum_map unconditionally; no test for empty qualified_last_commitment_per_index",
  "fix": "Add a unit test that feeds a `qr_info` whose `qualified_last_commitment_per_index` is empty (no quorum-snapshot branch) and asserts the expected outcome — either an empty map entry is created or an er"
}

}
}
} else if let Some(cycle_key) =
qualified_last_commitment_per_index.first().map(|q| q.quorum_entry.quorum_hash)
{
} else {
let cycle_map =
build_cycle_quorum_map(qualified_last_commitment_per_index, rotation_quorum_type)?;
*self.rotated_quorums_per_cycle.entry(cycle_key).or_default() = cycle_map;
*self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map;
}

#[cfg(not(feature = "quorum_validation"))]
Expand Down Expand Up @@ -1187,7 +1191,7 @@ mod tests {
};
use crate::sml::masternode_list::MasternodeList;
use crate::sml::masternode_list_engine::{
MasternodeListEngine, MasternodeListEngineBlockContainer,
MasternodeListEngine, MasternodeListEngineBlockContainer, WORK_DIFF_DEPTH,
};
use crate::sml::quorum_entry::qualified_quorum_entry::{
QualifiedQuorumEntry, VerifyingChainLockSignaturesType,
Expand Down Expand Up @@ -1437,6 +1441,8 @@ mod tests {
.expect("expected to apply diff");
}

let h_work_block_hash = qr_info.mn_list_diff_h.block_hash;

masternode_list_engine
.feed_qr_info::<fn(&BlockHash) -> Result<u32, ClientDataRetrievalError>>(
qr_info, true, true, None,
Expand All @@ -1452,6 +1458,24 @@ mod tests {
.1,
&[Llmqtype400_85, Llmqtype50_60, Llmqtype400_60],
);

// Verify the cycle map is keyed by the cycle boundary hash, not a quorum hash.
let h_work_height = masternode_list_engine
.block_container
.get_height(&h_work_block_hash)
.expect("h work block must be in container after feed_qr_info");
let expected_cycle_boundary_hash = masternode_list_engine
.block_container
.get_hash(&(h_work_height + WORK_DIFF_DEPTH))
.copied()
.expect("cycle boundary hash must be in container after feed_qrinfo_heights_to_engine");
assert!(
masternode_list_engine
.rotated_quorums_per_cycle
.contains_key(&expected_cycle_boundary_hash),
"rotated_quorums_per_cycle should be keyed by the cycle boundary hash {}",
expected_cycle_boundary_hash
);
}

#[test]
Expand Down
Loading