refactor: derive rotation cycle key from the cycle boundary block#100
refactor: derive rotation cycle key from the cycle boundary block#100xdustinface wants to merge 1 commit intofix/rotated-quorums-indexfrom
Conversation
Previously the per-cycle map was keyed by the quorum hash of whichever entry happened to be first in the incoming commitment list. In practice this produces the cycle boundary block hash because the wire format is ordered by `quorum_index` and the quorum at index 0 has its DKG start block at the cycle boundary itself, so the old and new keys should match in theory. Derive the key directly from the work block in `mn_list_diff_h` (at `cycle_boundary - WORK_DIFF_DEPTH`) and look up the cycle boundary block hash in the block container. This expresses the intent at the call site, removes the implicit dependency on wire ordering, and fails loudly when the block container does not yet have the cycle boundary block.
|
Manki — Review failed Planner (27s) Review — 8 findings Judge — 0 kept · 0 dropped |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/rotated-quorums-index #100 +/- ##
=============================================================
+ Coverage 67.86% 67.88% +0.02%
=============================================================
Files 318 318
Lines 67832 67846 +14
=============================================================
+ Hits 46036 46060 +24
+ Misses 21796 21786 -10
🚀 New features to boost your workflow:
|
|
@manki review |
|
Manki — Review complete Planner (22s) Review — 14 findings Judge — 8 kept · 0 dropped (121s) Review metadataConfig:
Judge decisions:
Timing:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
The #[cfg(feature = "quorum_validation")] gate covers only the binding of cycle_boundary_hash but not its downstream uses — a guaranteed compile error in non-validation builds. Behind that: removing both empty-list guards means a diff with zero rotation commitments now silently inserts a phantom cycle entry that downstream lookups may treat as fully populated.
📊 8 findings (1 required, 5 suggestion, 2 nit) · 54 lines · 606s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 606281,
"diffLines": 54,
"diffAdditions": 39,
"diffDeletions": 15,
"filesReviewed": 1,
"agents": [
"Correctness & Logic",
"Security & Safety",
"Testing & Coverage",
"Architecture & Design"
],
"findingsRaw": 14,
"findingsKept": 8,
"findingsDropped": 6,
"severity": {
"required": 1,
"suggestion": 5,
"nit": 2
},
"verdict": "REQUEST_CHANGES",
"prNumber": 100,
"commitSha": "7c8d7e50ba64e167c552a71a9945c1c4e0192d05",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 1,
"responseLength": 3496
},
{
"name": "Security & Safety",
"findingsRaw": 3,
"findingsKept": 2,
"responseLength": 3733
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4200
},
{
"name": "Architecture & Design",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4904
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 3,
"medium": 5,
"low": 0
},
"severityChanges": 8,
"mergedDuplicates": 6
},
"fileMetrics": {
"fileTypes": {
".rs": 1
},
"findingsPerFile": {
"dash/src/sml/masternode_list_engine/mod.rs": 8
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-sonnet-4-6"
}| } | ||
| 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; |
There was a problem hiding this comment.
💡 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
| *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"
}| let cycle_boundary_height = h_height + WORK_DIFF_DEPTH; | ||
| *self.block_container.get_hash(&cycle_boundary_height).ok_or( | ||
| QuorumValidationError::RequiredBlockNotPresent( | ||
| BlockHash::all_zeros(), |
There was a problem hiding this comment.
💡 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
| 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"
}| // 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 = { |
There was a problem hiding this comment.
💡 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"
}| #[cfg(feature = "quorum_validation")] | ||
| let cycle_boundary_hash = { | ||
| let cycle_boundary_height = h_height + WORK_DIFF_DEPTH; | ||
| *self.block_container.get_hash(&cycle_boundary_height).ok_or( |
There was a problem hiding this comment.
💡 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
| *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"
}| @@ -859,12 +865,10 @@ impl MasternodeListEngine { | |||
| } | |||
There was a problem hiding this comment.
💡 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
| } | |
| 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"
}| *self.rotated_quorums_per_cycle.entry(cycle_boundary_hash).or_default() = cycle_map; | ||
|
|
||
| // Apply collected updates after iteration to avoid borrow conflicts | ||
| for (heights, quorum_type, quorum_hash, new_status) in updates { |
There was a problem hiding this comment.
🚫 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
| 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"
}2 participants
Previously the per-cycle map was keyed by the quorum hash of whichever entry happened to be first in the incoming commitment list. In practice this produces the cycle boundary block hash because the wire format is ordered by
quorum_indexand the quorum at index 0 has its DKG start block at the cycle boundary itself, so the old and new keys should match in theory.Derive the key directly from the work block in
mn_list_diff_h(atcycle_boundary - WORK_DIFF_DEPTH) and look up the cycle boundary block hash in the block container. This expresses the intent at the call site, removes the implicit dependency on wire ordering, and fails loudly when the block container does not yet have the cycle boundary block.