fix(dash): classify missing infrastructure errors as Skipped#125
Open
xdustinface wants to merge 17 commits intov0.42-devfrom
Open
fix(dash): classify missing infrastructure errors as Skipped#125xdustinface wants to merge 17 commits intov0.42-devfrom
Skipped#125xdustinface wants to merge 17 commits intov0.42-devfrom
Conversation
|
Manki — Review complete Planner (23s) Review — 10 findings Judge — 6 kept · 0 dropped (18s) 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.
Reclassifying infrastructure errors as Skipped is the right call, but four reviewers independently flagged the same stale docstring on update_quorum_status and the conversion impl ships without any unit tests.
📊 6 findings (1 warning, 2 suggestion, 3 nitpick) · 64 lines · 149s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 148967,
"diffLines": 64,
"diffAdditions": 42,
"diffDeletions": 22,
"filesReviewed": 3,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 10,
"findingsKept": 6,
"findingsDropped": 4,
"severity": {
"blocker": 0,
"warning": 1,
"suggestion": 2,
"nitpick": 3
},
"verdict": "REQUEST_CHANGES",
"prNumber": 125,
"commitSha": "291174b67f2f02b9fffca2bdb957ffdc64a97ac6",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 2,
"responseLength": 2632
},
{
"name": "Architecture & Design",
"findingsRaw": 3,
"findingsKept": 3,
"responseLength": 3398
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 1,
"responseLength": 1157
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 5001
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 2,
"medium": 4,
"low": 0
},
"severityChanges": 6,
"mergedDuplicates": 4,
"defensiveHardeningCount": 1,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 3
},
"findingsPerFile": {
"dash/src/sml/quorum_entry/qualified_quorum_entry.rs": 1,
"dash/src/sml/llmq_entry_verification.rs": 4,
"dash/src/sml/masternode_list_engine/validation.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #125 +/- ##
=============================================
+ Coverage 70.62% 71.01% +0.39%
=============================================
Files 319 319
Lines 67590 68398 +808
=============================================
+ Hits 47734 48576 +842
+ Misses 19856 19822 -34
🚀 New features to boost your workflow:
|
`QuorumValidationError` carries both "the quorum data is bad" cases and "the caller didn't supply the data we need to verify". `update_quorum_status` and the rotated-quorum lookup in `validation.rs` mapped several of the latter to `LLMQEntryVerificationStatus::Invalid`, misreporting unverifiable quorums as bad. Add a `From<QuorumValidationError> for LLMQEntryVerificationStatus` impl that routes the infrastructure-data variants to `Skipped`, and let both call sites use `.into()`: - `RequiredBlockHeightNotPresent` becomes `Skipped(MissedList)`. - `RequiredSnapshotNotPresent` becomes `Skipped(UnknownBlock)`. - `RequiredRotatedChainLockSigsNotPresent` becomes a new `Skipped(MissingRotationChainLockSigs)` variant for the QRInfo case where a historical diff covers a range with no successful rotating DKG, so `feed_qr_info` can't populate the 4-sig tuple. All other variants still map to `Invalid`.
Addresses manki review comment on PR #125 #125 (comment)
Distinguish missing snapshot from missing block in `LLMQEntryVerificationSkipStatus` so downstream retry/back-off logic can target snapshot fetches separately. Mirrors the existing `MissingRotationChainLockSigs` pattern. Addresses manki review comment on PR #125 #125 (comment)
Adds unit tests for each match arm of the `LLMQEntryVerificationStatus` conversion impl so future rearrangements cannot silently mis-classify infrastructure errors as `Invalid` (or vice versa). Addresses manki review comment on PR #125 #125 (comment)
Adds From-impl arms for `RequiredRotatedChainLockSigNotPresent` (singular), `RequiredChainLockNotPresent`, and `VerifyingMasternodeListNotPresent`, all of which represent missing caller-provided infrastructure rather than bad quorum data. Without these arms they incorrectly fell through to `Invalid`, which would cause callers to treat infrastructure-missing situations as genuine quorum corruption. Introduces a singular `MissingRotationChainLockSig(u8, BlockHash)` skip variant mirroring the singular/plural pair on `QuorumValidationError`. The two emit sites carry genuinely different shapes (per-offset diff hash for the singular case, quorum hash for the plural), so collapsing them into one variant would be lossy. Addresses CodeRabbit review comment on PR #125 #125 (comment)
xdustinface
force-pushed
the
refactor/llmq-verification-status-classifier
branch
from
291174b to
5249339
Compare
|
Manki — Review complete Planner (22s) Review — 12 findings Judge — 8 kept · 0 dropped (27s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Two reviewers independently flagged the same real bug: when find_rotated_masternodes_for_quorums errors, the loop now overwrites pre-existing Invalid statuses with Skipped, so structurally bad quorums get reclassified as retryable. Several smaller findings about losing chain-lock specificity and stale docs are reasonable suggestions.
📊 8 findings (1 blocker, 4 suggestion, 3 nitpick) · 210 lines · 358s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 357874,
"diffLines": 210,
"diffAdditions": 187,
"diffDeletions": 23,
"filesReviewed": 3,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 12,
"findingsKept": 8,
"findingsDropped": 4,
"severity": {
"blocker": 1,
"warning": 0,
"suggestion": 4,
"nitpick": 3
},
"verdict": "REQUEST_CHANGES",
"prNumber": 125,
"commitSha": "524933990934baf0d608979153e7fa4ae28e2ccc",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 4332
},
{
"name": "Architecture & Design",
"findingsRaw": 2,
"findingsKept": 2,
"responseLength": 2380
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 2,
"responseLength": 2883
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 4897
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 5,
"medium": 3,
"low": 0
},
"severityChanges": 8,
"mergedDuplicates": 4,
"verdictReason": "required_present"
},
"fileMetrics": {
"fileTypes": {
".rs": 3
},
"findingsPerFile": {
"dash/src/sml/masternode_list_engine/validation.rs": 2,
"dash/src/sml/llmq_entry_verification.rs": 5,
"dash/src/sml/quorum_entry/qualified_quorum_entry.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}The bulk-overwrite path in `validate_rotation_cycle_quorums_validation_statuses` unconditionally replaced every quorum's status when `find_rotated_masternodes_for_quorums` errored, including entries the structure-validation pass had already marked as `Invalid`. Now that infrastructure errors map to `Skipped`, this would silently downgrade structurally-broken quorums from `Invalid` to `Skipped`, telling the caller to retry with more data instead of rejecting the quorum. Skip entries that are already `Invalid` in the overwrite loop. Addresses manki review comment on PR #125 #125 (comment)
…iant Map `RequiredChainLockNotPresent` to a dedicated `MissingChainLock` skip variant carrying both the expected height and the block hash, instead of folding it into `UnknownBlock` and discarding the height. This mirrors the already-added `MissingSnapshot` pattern and lets retry logic dispatch a chain-lock fetch instead of a block fetch. Addresses manki review comments on PR #125 #125 (comment) #125 (comment)
…tation-cycle statuses Adds a unit test that exercises `validate_rotation_cycle_quorums_validation_statuses` with two quorums: one structurally broken and one that triggers the `find_rotated_masternodes_for_quorums` infrastructure-error path. Asserts that the broken quorum keeps its `Invalid` status, and the infrastructure-error quorum surfaces as `Skipped(UnknownBlock(_))` rather than being misclassified as `Invalid`. This locks in both the behavioral change introduced by this PR and the bulk-overwrite guard. Addresses manki review comment on PR #125 #125 (comment)
|
Manki — Review complete Planner (24s) Review — 13 findings Judge — 10 kept · 0 dropped (53s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Bulk-overwrite guard now preserves pre-existing Invalid statuses, but the per-quorum quorum.validate() error arm still bypasses the new From classification — same misclassification bug the PR set out to fix, just in the sibling code path.
📊 10 findings (2 warning, 3 suggestion, 5 nitpick) · 311 lines · 418s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 417592,
"diffLines": 311,
"diffAdditions": 288,
"diffDeletions": 23,
"filesReviewed": 3,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 13,
"findingsKept": 10,
"findingsDropped": 3,
"severity": {
"blocker": 0,
"warning": 2,
"suggestion": 3,
"nitpick": 5
},
"verdict": "REQUEST_CHANGES",
"prNumber": 125,
"commitSha": "651ae16073ca4578b5a4cc0582dc6447b3f52518",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 3,
"findingsKept": 3,
"responseLength": 3469
},
{
"name": "Architecture & Design",
"findingsRaw": 3,
"findingsKept": 3,
"responseLength": 4452
},
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 3,
"responseLength": 3575
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4906
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 6,
"medium": 4,
"low": 0
},
"severityChanges": 10,
"mergedDuplicates": 3,
"defensiveHardeningCount": 1,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 3
},
"findingsPerFile": {
"dash/src/sml/llmq_entry_verification.rs": 5,
"dash/src/sml/masternode_list_engine/validation.rs": 3,
"dash/src/sml/quorum_entry/qualified_quorum_entry.rs": 2
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}…sifier The per-quorum `validate()` Err branch in `validate_rotation_cycle_quorums_validation_statuses` mapped errors directly to `LLMQEntryVerificationStatus::Invalid(e)`, bypassing the `From<QuorumValidationError>` classifier that the bulk-overwrite path uses. Infrastructure errors surfaced by `quorum.validate()` itself (e.g. a missing chain-lock sig) would therefore be misclassified as bad quorum data instead of `Skipped`. Convert through `.into()` so both error paths classify consistently. Addresses manki review comment on PR #125 #125 (comment)
The existing structure-broken assertion was tied to `InsufficientSigners` specifically, but the test's intent is just that any pre-existing `Invalid` status is preserved by the bulk-overwrite guard. A change in `validate_structure`'s check order would silently flip the assertion into a misleading failure. Match `Invalid(_)` instead. Also adds a second scenario covering the case where every quorum reaches the bulk-overwrite path with no pre-existing `Invalid`. This catches any off-by-one in the guard logic that would skip entries it shouldn't. Addresses manki review comments on PR #125 #125 (comment) #125 (comment)
Adds direct tests on `QualifiedQuorumEntry::update_quorum_status` so a regression that breaks the `From<QuorumValidationError>` delegation (e.g. an accidental `Self::Invalid(e)` fallback) gets caught at the method boundary rather than only via the indirect `From`-impl tests. Covers an infrastructure-error path (`RequiredSnapshotNotPresent` -> `Skipped`), a bad-data path (`InvalidQuorumPublicKey` -> `Invalid`), and the `Ok` path (`Verified`). Also refreshes the now-stale docstring on `update_quorum_status`, which still claimed only block/masternode-list errors became `Skipped`. Addresses manki review comment on PR #125 #125 (comment)
The reclassification of `VerifyingMasternodeListNotPresent` from `Invalid` to `Skipped` was a behavioral change carried by the `From` impl rewrite but not called out anywhere in the diff. Add a short rationale so a future reader (or code-review pass) does not have to reverse-engineer why the verifying-list variant is grouped with the other height-based caller- infrastructure errors. Addresses manki review comments on PR #125 #125 (comment) #125 (comment)
|
Manki — Review complete Planner (23s) Review — 12 findings Judge — 7 kept · 0 dropped (67s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Test brittleness around Skipped(UnknownBlock(_)) is the recurring theme — two reviewers independently flagged it. Open threads on validate_structure bypassing the From impl and the missing per-quorum validate path test remain unaddressed.
📊 7 findings (4 suggestion, 3 nitpick) · 420 lines · 390s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 389909,
"diffLines": 420,
"diffAdditions": 385,
"diffDeletions": 35,
"filesReviewed": 3,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 12,
"findingsKept": 7,
"findingsDropped": 5,
"severity": {
"blocker": 0,
"warning": 0,
"suggestion": 4,
"nitpick": 3
},
"verdict": "APPROVE",
"prNumber": 125,
"commitSha": "6a36650d6c4de0e037078576980a99dc3a1da93f",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 0,
"responseLength": 3416
},
{
"name": "Architecture & Design",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 4269
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 1,
"responseLength": 2530
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 3693
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 7,
"medium": 0,
"low": 0
},
"severityChanges": 7,
"mergedDuplicates": 0,
"verdictReason": "only_nit_or_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 3
},
"findingsPerFile": {
"dash/src/sml/masternode_list_engine/validation.rs": 3,
"dash/src/sml/llmq_entry_verification.rs": 4
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}…ven form Nine near-identical `#[test]` functions all built one error variant, called `.into()`, and asserted the resulting `LLMQEntryVerificationStatus`. Replace them with a single test driving a table of `(error, expected_status)` pairs so adding a new arm in the future is a one-row change.
The three per-case tests duplicated the `From<QuorumValidationError>` table-driven coverage. Keep one test that exercises both an `Ok` and an `Err` path through `update_quorum_status` so a regression breaking the delegation is still caught at this layer, and let the `From` impl tests cover the classification matrix.
The two `validate_structure` Err branches at the top of `validate_rotation_cycle_quorums_validation_statuses` constructed `LLMQEntryVerificationStatus::Invalid(e)` directly, bypassing the canonical `From<QuorumValidationError>` classifier that the rest of the function uses. If a structure-validation error variant is ever reclassified as infrastructure-missing, the direct construction would silently preserve the old behavior. Funnel both sites through `.into()` so all classification flows through the single canonical path. Addresses manki review comment on PR #125 #125 (comment)
Both rotation-cycle status tests asserted `Skipped(UnknownBlock(_))`, which is the specific sub-variant produced by `MasternodeListEngine::default()` today. If the empty engine ever errors with a different infrastructure variant (e.g. `RequiredMasternodeListNotPresent` -> `MissedList`), the assertion would fail misleadingly even though the intent of the test (classification as `Skipped`, not `Invalid`) is still satisfied. Match `Skipped(_)` so the assertion verifies the classification boundary the test is actually about. Addresses manki review comments on PR #125 #125 (comment) #125 (comment)
Adds a table-driven test asserting the formatted output of `MissingSnapshot`,
`MissingChainLock`, `MissingRotationChainLockSigs`, and
`MissingRotationChainLockSig` so a typo in any of the format strings (in
particular the `h - {offset}` style for the singular rotation-sig case) is
caught at unit-test time rather than only by an observant log reader.
Addresses manki review comment on PR #125
#125 (comment)
|
Manki — Review complete Planner (26s) Review — 9 findings Judge — 3 kept · 1 dropped (107s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
All 10 previous threads addressed cleanly — .into() is used everywhere, tests use wildcard match against Skipped/Invalid instead of pinning variants, and MissingRotationChainLockSigs now carries QuorumHash. Remaining nits are all about the new test helper using BlockHash for QuorumHash slots (silently OK because QuorumHash is a type alias for BlockHash) and shallow coverage on the Invalid branch.
📊 3 findings (2 suggestion, 1 nitpick) · 393 lines · 618s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 618405,
"diffLines": 393,
"diffAdditions": 348,
"diffDeletions": 45,
"filesReviewed": 3,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 9,
"findingsKept": 3,
"findingsDropped": 6,
"severity": {
"blocker": 0,
"warning": 0,
"suggestion": 2,
"nitpick": 1
},
"verdict": "APPROVE",
"prNumber": 125,
"commitSha": "48d06628cc702184d7a9d4a4912e38112408a08b",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 0,
"responseLength": 3361
},
{
"name": "Architecture & Design",
"findingsRaw": 2,
"findingsKept": 1,
"responseLength": 3325
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 0,
"responseLength": 1534
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 2,
"responseLength": 4398
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 4,
"medium": 0,
"low": 0
},
"severityChanges": 4,
"mergedDuplicates": 0,
"verdictReason": "only_nit_or_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 3
},
"findingsPerFile": {
"dash/src/sml/llmq_entry_verification.rs": 2,
"dash/src/sml/quorum_entry/qualified_quorum_entry.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}
This was referenced May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
QuorumValidationErrorcarries both "the quorum data is bad" cases and "the caller didn't supply the data we need to verify".update_quorum_statusand the rotated-quorum lookup invalidation.rsmapped several of the latter toLLMQEntryVerificationStatus::Invalid, misreporting unverifiable quorums as bad.Add a
From<QuorumValidationError> for LLMQEntryVerificationStatusimpl that routes the infrastructure-data variants toSkipped, and let both call sites use.into():RequiredBlockHeightNotPresentbecomesSkipped(MissedList).RequiredSnapshotNotPresentbecomesSkipped(UnknownBlock).RequiredRotatedChainLockSigsNotPresentbecomes a newSkipped(MissingRotationChainLockSigs)variant for the QRInfo case where a historical diff covers a range with no successful rotating DKG, sofeed_qr_infocan't populate the 4-sig tuple.All other variants still map to
Invalid.