fix: resolve just-retired quorum by walking earlier masternode lists#801
Open
xdustinface wants to merge 4 commits into
Open
fix: resolve just-retired quorum by walking earlier masternode lists#801xdustinface wants to merge 4 commits into
xdustinface wants to merge 4 commits into
Conversation
`get_quorum_at_height` and the FFI `ffi_dash_spv_get_quorum_public_key` resolved a quorum only through the single nearest masternode list at or below the lookup height. A Platform signing quorum is selected at a lagged height, so by the proof's `core_chain_locked_height` it can already have retired out of the active set. `apply_diff` drops a retired quorum from every later list, so the nearest list misses and the lookup fails with `Quorum not found`, surfacing downstream as `InvalidQuorum`. The failure is intermittent, firing only at the retirement edge. The full entry is not actually gone: `apply_diff` clones the base list without mutating earlier ones, and old per-height lists are retained, so the quorum still lives in every list from its mint height up to retirement. Add `MasternodeListEngine::quorum_entry_for_hash_at_or_before_height`, which walks `masternode_lists` backward from the lookup height and returns the first list still holding the quorum, skipping `Invalid` entries. Both consumer sites route through it, so the real full `QualifiedQuorumEntry` is returned with no synthesized data, no new storage, and no API change. The walk is floored at a few active windows below the height (derived from the type's `signing_active_quorum_count` and DKG interval): a legitimately referenced signing quorum cannot be older than that, so a miss scans a bounded span rather than every accumulated list. Closes #800.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds MasternodeListEngine::quorum_entry_for_hash_at_or_before_height() with active-window bounded backward search, and updates dash-spv client and the FFI layer to use this API to resolve quorum entries by hash/height. ChangesQuorum hash-based lookup refactor
Sequence DiagramsequenceDiagram
participant FFI
participant Client
participant MasternodeListEngine
participant RetainedMasternodeLists
FFI->>MasternodeListEngine: quorum_entry_for_hash_at_or_before_height(type, hash, locked_height)
Client->>MasternodeListEngine: quorum_entry_for_hash_at_or_before_height(type, hash, height)
MasternodeListEngine->>RetainedMasternodeLists: iterate lists from floor..=height (descending)
RetainedMasternodeLists-->>MasternodeListEngine: return list (contains quorums map)
MasternodeListEngine->>MasternodeListEngine: select first matching quorum where status != Invalid
MasternodeListEngine-->>FFI: Some((list_height, &QualifiedQuorumEntry)) or None
MasternodeListEngine-->>Client: Some((list_height, &QualifiedQuorumEntry)) or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Comment |
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/client/queries.rs`:
- Line 78: The tracing::warn!(message) call treats message as a structured
field; change the tracing::warn! invocation that references the variable message
to use a format string (e.g., tracing::warn!("{}", message)) or the formatter
shorthand (tracing::warn!(%message)) so the text is emitted as the event message
to match the tracing::debug(...) style; update the invocation where
tracing::warn! is called with the local variable message.
In `@dash/src/sml/masternode_list_engine/helpers.rs`:
- Around line 96-101: Remove the redundant QuorumHash import in the test module:
the tests module already brings QuorumHash into scope via use super::*, so
update the use crate::{BlockHash, QuorumHash}; line (inside #[cfg(test)] mod
tests) to only import BlockHash (e.g., use crate::BlockHash;) or otherwise omit
QuorumHash, leaving functions/structs that reference QuorumHash untouched;
reference the test module and the use statement near the top of helpers.rs to
locate and edit the import.
🪄 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: 3fa92136-d9bf-4610-a80f-1de727fbf6c4
📒 Files selected for processing (3)
dash-spv-ffi/src/platform_integration.rsdash-spv/src/client/queries.rsdash/src/sml/masternode_list_engine/helpers.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #801 +/- ##
==========================================
+ Coverage 72.67% 72.69% +0.01%
==========================================
Files 322 323 +1
Lines 71363 71838 +475
==========================================
+ Hits 51866 52224 +358
- Misses 19497 19614 +117
|
…rum lookup Addresses CodeRabbit review comment on PR #801 #801 (comment)
xdustinface
added a commit
that referenced
this pull request
Jun 4, 2026
Addresses CodeRabbit review comment on PR #801 #801 (comment)
Addresses CodeRabbit review comment on PR #801 #801 (comment)
xdustinface
force-pushed
the
fix/quorum-lookup-retired-quorum
branch
from
a241cc3 to
57ae73d
Compare
The public `quorum_entry_for_hash_at_or_before_height` doc linked to the private `QUORUM_WALK_BACK_ACTIVE_WINDOWS` constant, which rustdoc rejects under `-D rustdoc::private-intra-doc-links`. Demote the link to a plain code span so the Documentation CI job passes.
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
1 participant
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.
get_quorum_at_heightand the FFIffi_dash_spv_get_quorum_public_keyresolved a quorum only through the single nearest masternode list at or below the lookup height. A Platform signing quorum is selected at a lagged height, so by the proof'score_chain_locked_heightit can already have retired out of the active set.apply_diffdrops a retired quorum from every later list, so the nearest list misses and the lookup fails withQuorum not found, surfacing downstream asInvalidQuorum. The failure is intermittent, firing only at the retirement edge.The full entry is not actually gone:
apply_diffclones the base list without mutating earlier ones, and old per-height lists are retained, so the quorum still lives in every list from its mint height up to retirement.Add
MasternodeListEngine::quorum_entry_for_hash_at_or_before_height, which walksmasternode_listsbackward from the lookup height and returns the first list still holding the quorum, skippingInvalidentries. Both consumer sites route through it, so the real fullQualifiedQuorumEntryis returned with no synthesized data, no new storage, and no API change. The walk is floored at a few active windows below the height (derived from the type'ssigning_active_quorum_countand DKG interval): a legitimately referenced signing quorum cannot be older than that, so a miss scans a bounded span rather than every accumulated list.Closes #800.
Summary by CodeRabbit