fix: index rotated quorums by quorum_index and rebuild per cycle#637
Open
xdustinface wants to merge 2 commits intov0.42-devfrom
Open
fix: index rotated quorums by quorum_index and rebuild per cycle#637xdustinface wants to merge 2 commits intov0.42-devfrom
quorum_index and rebuild per cycle#637xdustinface wants to merge 2 commits intov0.42-devfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRotated quorum storage changed from per-cycle Vec to per-cycle BTreeMap keyed by quorum_index; lookup switched from linear search to direct map access. New validation builds per-cycle maps (rejecting missing/invalid/duplicate indices and wrong counts). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash/src/sml/masternode_list_engine/mod.rs (1)
747-751: Consider replacing.unwrap()with safer alternative.While the
unwrap()at line 748 is safe because the loop only executes whenqualified_last_commitment_per_indexis non-empty (which guarantees theOptionisSomefrom the.first().map()at lines 724-729), the coding guidelines discourageunwrap()in library code.♻️ Suggested safer alternative
- insert_cycle_quorum_by_index( - qualified_rotated_quorums_per_cycle.as_mut().unwrap(), - rotated_quorum.clone(), - rotation_quorum_type, - )?; + if let Some(map) = qualified_rotated_quorums_per_cycle.as_mut() { + insert_cycle_quorum_by_index( + map, + rotated_quorum.clone(), + rotation_quorum_type, + )?; + }As per coding guidelines: "/src//*.rs: Avoid
unwrap()andexpect()in library code; use proper error types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/sml/masternode_list_engine/mod.rs` around lines 747 - 751, Replace the direct .unwrap() on qualified_rotated_quorums_per_cycle with a safe pattern: check the Option (e.g., if let Some(ref mut qualified_rotated_quorums) = qualified_rotated_quorums_per_cycle) and call insert_cycle_quorum_by_index(qualified_rotated_quorums, rotated_quorum.clone(), rotation_quorum_type) inside that branch, otherwise return an Err with an appropriate error variant; reference the surrounding logic that sets qualified_rotated_quorums_per_cycle from qualified_last_commitment_per_index (the .first().map() earlier) to craft a meaningful error message if the Option is None.
🤖 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/mod.rs`:
- Around line 747-751: Replace the direct .unwrap() on
qualified_rotated_quorums_per_cycle with a safe pattern: check the Option (e.g.,
if let Some(ref mut qualified_rotated_quorums) =
qualified_rotated_quorums_per_cycle) and call
insert_cycle_quorum_by_index(qualified_rotated_quorums, rotated_quorum.clone(),
rotation_quorum_type) inside that branch, otherwise return an Err with an
appropriate error variant; reference the surrounding logic that sets
qualified_rotated_quorums_per_cycle from qualified_last_commitment_per_index
(the .first().map() earlier) to craft a meaningful error message if the Option
is None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a849f286-b84f-4ddd-b04e-9fc98bf65d33
📒 Files selected for processing (4)
dash/src/sml/masternode_list_engine/message_request_verification.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/quorum_validation_error.rsdash/tests/data/test_DML_diffs/masternode_list_engine.hex
xdustinface
force-pushed
the
fix/rotated-quorums-index
branch
from
31f7258 to
a82c35c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #637 +/- ##
=============================================
+ Coverage 67.48% 67.90% +0.42%
=============================================
Files 246 318 +72
Lines 49280 67832 +18552
=============================================
+ Hits 33256 46063 +12807
- Misses 16024 21769 +5745
|
xdustinface
force-pushed
the
fix/rotated-quorums-index
branch
from
a82c35c to
7d9305a
Compare
The `Vec<QualifiedQuorumEntry>` had no size bound and no uniqueness check on `quorum_index`. Combined with QRInfo still being requested on every new block (will change soon), repeated `feed_qr_info` calls within a single rotation cycle accumulated duplicate entries that the lookup then returned as stale matches. - Switch the inner container to `BTreeMap<u16, QualifiedQuorumEntry>` keyed by `quorum_index` so insertion replaces by key, lookup is direct, and re-feeds of the same cycle are not causing wrong behaviour anymore. - Introduce `build_cycle_quorum_map` to centralize map construction with index validation and count checks. Both branches of `feed_qr_info` use it to rebuild the per-cycle map from scratch. - Migrates `tests/data/test_DML_diffs/masternode_list_engine.hex` to the new format.
xdustinface
force-pushed
the
fix/rotated-quorums-index
branch
from
7d9305a to
9c9835f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/masternode_list_engine/mod.rs`:
- Around line 722-724: The code currently sets cycle_key by taking the first
element of qualified_last_commitment_per_index (cycle_key =
qualified_last_commitment_per_index.first().map(|q|
q.quorum_entry.quorum_hash)), which is wrong if the vector is out of order;
change the lookup to find the entry whose quorum_entry.quorum_index == 0 and use
its quorum_hash (i.e., use .iter().find(|q| q.quorum_entry.quorum_index ==
0).map(|q| q.quorum_entry.quorum_hash)) so the cycle is always keyed by index 0;
make this change in both the verifying branch (where cycle_key is currently set)
and the non‑verifying branch (the analogous assignment mentioned for Lines
856–861 and 759–765) so both branches derive cycle_key from quorum_index == 0.
- Around line 225-232: The current validation only checks map.len() against
expected (rotation_quorum_type.active_quorum_count()), which allows missing or
non-zero-based quorum_index keys; update validation to ensure the full
quorum_index range 0..expected-1 exists by iterating each expected index and
verifying map.get(index) is Some (or map.contains_key(index)) and return
QuorumValidationError::CorruptedCodeExecution with a clear message if any index
is missing; reference the existing symbols rotation_quorum_type, expected, map
and quorum_index when adding this explicit per-index check so downstream code
that does direct get(index) lookups can safely assume index 0..expected-1 are
present.
🪄 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: 3a114604-c50c-4d7f-be19-ba011eb95168
📒 Files selected for processing (4)
dash/src/sml/masternode_list_engine/message_request_verification.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/quorum_validation_error.rsdash/tests/data/test_DML_diffs/masternode_list_engine.hex
🚧 Files skipped from review as they are similar to previous changes (1)
- dash/src/sml/quorum_validation_error.rs
The existing count check allowed a set like `{1, 2, ..., expected}` to pass - distinct indices with the right total, but missing index 0 and containing one out-of-range index. Downstream code assumes contiguous `[0, expected)` coverage and breaks on direct `get(&i)` lookups. Add a per-entry upper-bound guard inside the loop so the offending index is reported precisely; combined with the existing no-duplicates and count checks, this guarantees `keys == {0, 1, ..., expected - 1}` exactly.
Addresses CodeRabbit review comment on PR #637
#637 (comment)
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Apr 13, 2026
The existing count check allowed a set like `{1, 2, ..., expected}` to pass - distinct indices with the right total, but missing index 0 and containing one out-of-range index. Downstream code assumes contiguous `[0, expected)` coverage and breaks on direct `get(&i)` lookups. Add a per-entry upper-bound guard inside the loop so the offending index is reported precisely; combined with the existing no-duplicates and count checks, this guarantees `keys == {0, 1, ..., expected - 1}` exactly.
Addresses CodeRabbit review comment on PR dashpay#637
dashpay#637 (comment)
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Apr 13, 2026
The existing count check allowed a set like `{1, 2, ..., expected}` to pass - distinct indices with the right total, but missing index 0 and containing one out-of-range index. Downstream code assumes contiguous `[0, expected)` coverage and breaks on direct `get(&i)` lookups. Add a per-entry upper-bound guard inside the loop so the offending index is reported precisely; combined with the existing no-duplicates and count checks, this guarantees `keys == {0, 1, ..., expected - 1}` exactly.
Addresses CodeRabbit review comment on PR dashpay#637
dashpay#637 (comment)
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Apr 13, 2026
The existing count check allowed a set like `{1, 2, ..., expected}` to pass - distinct indices with the right total, but missing index 0 and containing one out-of-range index. Downstream code assumes contiguous `[0, expected)` coverage and breaks on direct `get(&i)` lookups. Add a per-entry upper-bound guard inside the loop so the offending index is reported precisely; combined with the existing no-duplicates and count checks, this guarantees `keys == {0, 1, ..., expected - 1}` exactly.
Addresses CodeRabbit review comment on PR dashpay#637
dashpay#637 (comment)
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Apr 13, 2026
The existing count check allowed a set like `{1, 2, ..., expected}` to pass - distinct indices with the right total, but missing index 0 and containing one out-of-range index. Downstream code assumes contiguous `[0, expected)` coverage and breaks on direct `get(&i)` lookups. Add a per-entry upper-bound guard inside the loop so the offending index is reported precisely; combined with the existing no-duplicates and count checks, this guarantees `keys == {0, 1, ..., expected - 1}` exactly.
Addresses CodeRabbit review comment on PR dashpay#637
dashpay#637 (comment)
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.
The
Vec<QualifiedQuorumEntry>had no size bound and no uniqueness check onquorum_index. Combined with QRInfo still being requested on every new block (will change soon), repeatedfeed_qr_infocalls within a single rotation cycle accumulated duplicate entries that the lookup then returned as stale matches.BTreeMap<u16, QualifiedQuorumEntry>keyed byquorum_indexso insertion replaces by key, lookup is direct, and re-feeds of the same cycle are not causing wrong behaviour anymore.insert_cycle_quorum_by_indexas helper to centralize inserts.feed_qr_infonow.clear()the per-cycle map before refilling it and clearing is enforced via a debug assert.tests/data/test_DML_diffs/masternode_list_engine.hexto the new format.Summary by CodeRabbit
Performance Improvements
Reliability & Stability
Tests