feat: add QRInfoFeedResult and return it from feed_qr_info#686
feat: add QRInfoFeedResult and return it from feed_qr_info#686xdustinface wants to merge 2 commits intov0.42-devfrom
QRInfoFeedResult and return it from feed_qr_info#686Conversation
For now just add logging for the result. This will also be used more in follow up PRs.
|
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 51 minutes and 13 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)
📝 WalkthroughWalkthroughThe changes enhance QRInfo processing by capturing and logging metadata about rotated quorum validation counts, while preserving existing retry logic for signature validation errors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 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}
There was a problem hiding this comment.
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/masternode_list_engine/mod.rs (1)
618-662:⚠️ Potential issue | 🟡 Minor
freshly_validated_countincrements regardless ofverify_rotated_quorums.The counter is bumped inside the
elsebranch of theknown_qualified_quorum_entrylookup (line 627), which runs unconditionally for every entry inlast_commitment_per_index. However, the actual validation (validate_rotation_cycle_quorums_validation_statuses) only runs insideif verify_rotated_quorumsfurther down. So when a caller passesverify_rotated_quorums = false,freshly_validated_countreports quorums that went through the signature-attachment path but were never actually validated — which contradicts the docstring wording "Rotated quorums that went through the fresh-validation path this call".In the current PR the only caller (
sync_manager.rs) always passestrue, so this is a latent concern rather than an active bug. Consider either:
- renaming to something like
freshly_qualified_count/newly_seen_rotated_quorum_count, or- gating the increment on
verify_rotated_quorumsso the field's name matches its semantics.🤖 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 618 - 662, Summary: freshly_validated_count is incremented even when verify_rotated_quorums is false, so it misreports quorums that weren’t actually validated. Fix: in the mapping that builds qualified_last_commitment_per_index (the closure over last_commitment_per_index where known_qualified_quorum_entry is checked and the else branch attaches VerifyingChainLockSignaturesType::Rotating), only increment freshly_validated_count when verify_rotated_quorums is true; alternatively, if you prefer semantics that count newly seen rotated quorums regardless of validation, rename freshly_validated_count to freshly_qualified_count and update any uses (including where validate_rotation_cycle_quorums_validation_statuses is called) to reflect the new name. Reference symbols: freshly_validated_count, known_qualified_quorum_entry, verify_rotated_quorums, validate_rotation_cycle_quorums_validation_statuses, QualifiedQuorumEntry, VerifyingChainLockSignaturesType::Rotating.
🧹 Nitpick comments (2)
dash/src/sml/masternode_list_engine/mod.rs (1)
574-581: Nit:stored_cycle_heightis resolved before the diffs are applied.
stored_cycle_heightis computed here, but theapply_diffcalls below can themselves feed new block heights intoself.block_container(seeapply_diff→feed_block_heightpath). In practice this is fine because callers are contractually required to pre-populate heights for all hashes returned byqr_info_referenced_block_hashes(which includeslast_commitment_per_index[*].quorum_hash), so the lookup succeeds either way. Still, computing it alongsidecycle_keyafter the diffs are applied would make the relationship to what actually gets stored inrotated_quorums_per_cyclemore obvious and avoids wasted work when an earlierapply_diff?bails out.Purely a readability/robustness nit — no functional bug.
dash-spv/src/sync/masternodes/sync_manager.rs (1)
304-311: Consider a structuredinfo!event and including the tip height for correlation.The current format string is fine, but since this is currently the main consumer of the new summary and is stated in the PR description to be the primary near-term use, a couple of small improvements would pay off later:
- Emit structured fields instead of interpolating into the message, which plays better with log aggregation:
♻️ Proposed tweak
- if let Some(summary) = summary { - tracing::info!( - "QRInfo processed: stored_cycle_height={:?}, rotated_quorum_count={}, freshly_validated_count={}", - summary.stored_cycle_height, - summary.rotated_quorum_count, - summary.freshly_validated_count, - ); - } + if let Some(summary) = summary { + tracing::info!( + stored_cycle_height = ?summary.stored_cycle_height, + rotated_quorum_count = summary.rotated_quorum_count, + freshly_validated_count = summary.freshly_validated_count, + "QRInfo processed" + ); + }
- Consider logging at
debug!if this fires on every QRInfo message in steady-state incremental updates, to avoid noisyinfo!output after initial sync. Alternatively, only log atinfo!whenfreshly_validated_count > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/masternodes/sync_manager.rs` around lines 304 - 311, The current tracing::info! call interpolates values into the message and omits the chain tip, so change the log to emit structured fields and include the tip height for correlation: replace the formatted string usage around the QRInfo summary (the block that inspects summary and logs stored_cycle_height, rotated_quorum_count, freshly_validated_count) with a structured event that passes those values as named fields (e.g., stored_cycle_height=summary.stored_cycle_height, rotated_quorum_count=summary.rotated_quorum_count, freshly_validated_count=summary.freshly_validated_count, tip_height=<obtain tip value>) and reduce noise by logging at debug level or only using info! when summary.freshly_validated_count > 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash/src/sml/masternode_list_engine/mod.rs`:
- Around line 618-662: Summary: freshly_validated_count is incremented even when
verify_rotated_quorums is false, so it misreports quorums that weren’t actually
validated. Fix: in the mapping that builds qualified_last_commitment_per_index
(the closure over last_commitment_per_index where known_qualified_quorum_entry
is checked and the else branch attaches
VerifyingChainLockSignaturesType::Rotating), only increment
freshly_validated_count when verify_rotated_quorums is true; alternatively, if
you prefer semantics that count newly seen rotated quorums regardless of
validation, rename freshly_validated_count to freshly_qualified_count and update
any uses (including where validate_rotation_cycle_quorums_validation_statuses is
called) to reflect the new name. Reference symbols: freshly_validated_count,
known_qualified_quorum_entry, verify_rotated_quorums,
validate_rotation_cycle_quorums_validation_statuses, QualifiedQuorumEntry,
VerifyingChainLockSignaturesType::Rotating.
---
Nitpick comments:
In `@dash-spv/src/sync/masternodes/sync_manager.rs`:
- Around line 304-311: The current tracing::info! call interpolates values into
the message and omits the chain tip, so change the log to emit structured fields
and include the tip height for correlation: replace the formatted string usage
around the QRInfo summary (the block that inspects summary and logs
stored_cycle_height, rotated_quorum_count, freshly_validated_count) with a
structured event that passes those values as named fields (e.g.,
stored_cycle_height=summary.stored_cycle_height,
rotated_quorum_count=summary.rotated_quorum_count,
freshly_validated_count=summary.freshly_validated_count, tip_height=<obtain tip
value>) and reduce noise by logging at debug level or only using info! when
summary.freshly_validated_count > 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6146bb62-3e6d-4074-abe1-7ca25d185872
📒 Files selected for processing (2)
dash-spv/src/sync/masternodes/sync_manager.rsdash/src/sml/masternode_list_engine/mod.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #686 +/- ##
=============================================
+ Coverage 70.22% 70.24% +0.02%
=============================================
Files 319 319
Lines 66686 66774 +88
=============================================
+ Hits 46830 46907 +77
- Misses 19856 19867 +11
|
1 participant
For now just add logging for the result. This will also be used more in follow up PRs.
Summary by CodeRabbit