fix(dash): classify missing infrastructure errors as Skipped#721
fix(dash): classify missing infrastructure errors as Skipped#721xdustinface wants to merge 1 commit intov0.42-devfrom
Skipped#721Conversation
📝 WalkthroughWalkthroughCentralizes LLMQ validation error classification by extending skip-reasons in ChangesLLMQ error classification & call-site simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #721 +/- ##
=============================================
+ Coverage 70.57% 70.99% +0.41%
=============================================
Files 319 319
Lines 68211 68398 +187
=============================================
+ Hits 48142 48561 +419
+ Misses 20069 19837 -232
|
|
@CodeRabbit review! |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash/src/sml/llmq_entry_verification.rs (1)
13-25:⚠️ Potential issue | 🔴 CriticalInserting
MissingRotationChainLockSigsbeforeOtherContextshifts its bincode discriminant — critical serialization break for persisted data.
MasternodeListEngine(which includesLLMQEntryVerificationStatusinquorum_statuses) is serialized and persisted to disk asMasternodeState.engine_state. Enum variants are encoded with discriminants assigned in declaration order; with no explicit#[bincode(variant_index = N)]overrides,OtherContextmoves from discriminant3to4. Any previously persistedSkipped(OtherContext(...))will be silently misread asSkipped(MissingRotationChainLockSigs(...))or fail to decode after deserialization, causing silent data corruption.The safe fix is to append the new variant at the end of the enum rather than inserting it before
OtherContext:🛡️ Proposed fix — append variant to preserve discriminants
pub enum LLMQEntryVerificationSkipStatus { NotMarkedForVerification, MissedList(CoreBlockHeight), UnknownBlock(BlockHash), - /// The quorum entry came through without an attached - /// `VerifyingChainLockSignaturesType::Rotating`. Typically happens when - /// a QRInfo's historical diff covers a block range in which no rotating - /// DKG successfully committed, so `apply_diff` extracts no - /// `rotation_sig` and `feed_qr_info` can't populate the 4-sig tuple for - /// the quorums in `lastCommitmentPerIndex`. - MissingRotationChainLockSigs(BlockHash), OtherContext(String), + /// The quorum entry came through without an attached + /// `VerifyingChainLockSignaturesType::Rotating`. Typically happens when + /// a QRInfo's historical diff covers a block range in which no rotating + /// DKG successfully committed, so `apply_diff` extracts no + /// `rotation_sig` and `feed_qr_info` can't populate the 4-sig tuple for + /// the quorums in `lastCommitmentPerIndex`. + MissingRotationChainLockSigs(BlockHash), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/sml/llmq_entry_verification.rs` around lines 13 - 25, LLMQEntryVerificationSkipStatus was changed so inserting MissingRotationChainLockSigs before OtherContext shifts bincode discriminants and breaks persisted MasternodeState.engine_state; fix by moving the MissingRotationChainLockSigs variant to the end of the LLMQEntryVerificationSkipStatus enum (after OtherContext) so existing discriminant ordering for LLMQEntryVerificationStatus/MasternodeListEngine remains unchanged; ensure no other reordering occurs (or alternatively add explicit bincode variant_index attributes if reordering is unavoidable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/src/sml/llmq_entry_verification.rs`:
- Around line 62-86: Add match arms in the From<QuorumValidationError> for the
three missing-infrastructure variants so they map to Skipped instead of falling
through to Invalid: handle
QuorumValidationError::RequiredRotatedChainLockSigNotPresent(index, block_hash)
->
Self::Skipped(LLMQEntryVerificationSkipStatus::MissingRotationChainLockSig(index,
block_hash)); QuorumValidationError::RequiredChainLockNotPresent(height,
block_hash) ->
Self::Skipped(LLMQEntryVerificationSkipStatus::UnknownBlock(block_hash)); and
QuorumValidationError::VerifyingMasternodeListNotPresent(height) ->
Self::Skipped(LLMQEntryVerificationSkipStatus::MissedList(height)); keep the
existing arms (e.g. RequiredRotatedChainLockSigsNotPresent,
RequiredBlockNotPresent, RequiredSnapshotNotPresent) unchanged.
---
Outside diff comments:
In `@dash/src/sml/llmq_entry_verification.rs`:
- Around line 13-25: LLMQEntryVerificationSkipStatus was changed so inserting
MissingRotationChainLockSigs before OtherContext shifts bincode discriminants
and breaks persisted MasternodeState.engine_state; fix by moving the
MissingRotationChainLockSigs variant to the end of the
LLMQEntryVerificationSkipStatus enum (after OtherContext) so existing
discriminant ordering for LLMQEntryVerificationStatus/MasternodeListEngine
remains unchanged; ensure no other reordering occurs (or alternatively add
explicit bincode variant_index attributes if reordering is unavoidable).
🪄 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: f9e48bcb-72b8-4a4f-9e90-edf9996ff24d
📒 Files selected for processing (3)
dash/src/sml/llmq_entry_verification.rsdash/src/sml/masternode_list_engine/validation.rsdash/src/sml/quorum_entry/qualified_quorum_entry.rs
Several `QuorumValidationError` variants represent missing caller-supplied infrastructure (unknown block, unloaded masternode list, missing snapshot, chain lock, rotation sig) rather than bad quorum data, but the call sites mapped them to `LLMQEntryVerificationStatus::Invalid`, pushing callers to reject quorums they should refetch. Add a `From<QuorumValidationError> for LLMQEntryVerificationStatus` impl as the single canonical classifier, route every construction site through `.into()`, and add the missing `Skipped` sub-variants needed to preserve context. Also guard the bulk-overwrite path so structurally-broken quorums keep `Invalid` when an infrastructure error fires.
xdustinface
force-pushed
the
fix/llmq-verification-status-classifier
branch
from
218cdcd to
7300318
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash/src/sml/masternode_list_engine/validation.rs (1)
135-141: 💤 Low valueConsider using
.into()for consistency.This error path explicitly constructs
Invalid(e.clone())while other sites now use.into(). Both are correct sinceCorruptedCodeExecutionfalls through toInvalidanyway, but using.into()would maintain consistency with the rest of the function.♻️ Optional consistency fix
Err(e) => { - return_statuses.insert( - quorum.quorum_entry.quorum_hash, - LLMQEntryVerificationStatus::Invalid(e.clone()), - ); + return_statuses.insert(quorum.quorum_entry.quorum_hash, e.into()); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/sml/masternode_list_engine/validation.rs` around lines 135 - 141, Replace the explicit construction LLMQEntryVerificationStatus::Invalid(e.clone()) with using .into() for consistency with other paths: locate the insert into return_statuses keyed by quorum.quorum_entry.quorum_hash and change the value expression to e.into() (or e.clone().into() if a clone is still required) so the error type converts into LLMQEntryVerificationStatus via Into instead of directly constructing the Invalid variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash/src/sml/masternode_list_engine/validation.rs`:
- Around line 135-141: Replace the explicit construction
LLMQEntryVerificationStatus::Invalid(e.clone()) with using .into() for
consistency with other paths: locate the insert into return_statuses keyed by
quorum.quorum_entry.quorum_hash and change the value expression to e.into() (or
e.clone().into() if a clone is still required) so the error type converts into
LLMQEntryVerificationStatus via Into instead of directly constructing the
Invalid variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 931befd2-a67d-4d16-8dad-f3aa34b2d920
📒 Files selected for processing (3)
dash/src/sml/llmq_entry_verification.rsdash/src/sml/masternode_list_engine/validation.rsdash/src/sml/quorum_entry/qualified_quorum_entry.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash/src/sml/quorum_entry/qualified_quorum_entry.rs
1 participant
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.Summary by CodeRabbit