fix MinerGetBaseInfo returns the correct BeaconEntries - #7427
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughQuicknet beacon entry generation now returns one entry for each epoch from the parent epoch through the current epoch. A regression test verifies per-epoch rounds when null beacon rounds occur. ChangesBeacon entry coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/beacon/drand.rs (1)
117-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve epoch and round context when fetching entries.
The new per-epoch fetch propagates
entryerrors without identifying which covered epoch failed. Add context before?so null-round failures remain diagnosable.Proposed fix
- out.push(curr_beacon.entry(round).await?); + out.push( + curr_beacon + .entry(round) + .await + .context(format!( + "failed to fetch beacon entry for epoch {covered_epoch}, round {round}" + ))?, + );As per coding guidelines, Rust operations should use
anyhow::Result<T>for most operations and add context with.context()when errors occur.🤖 Prompt for 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. In `@src/beacon/drand.rs` around lines 117 - 120, Update the per-epoch fetch loop in the drand flow to add anyhow context to the `curr_beacon.entry(round)` operation before propagating its error. Include both the `covered_epoch` and resolved `round` in the context so null-round failures identify the exact epoch and round that failed, while preserving the existing `out.push` behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/beacon/drand.rs`:
- Around line 117-120: Update the per-epoch fetch loop in the drand flow to add
anyhow context to the `curr_beacon.entry(round)` operation before propagating
its error. Include both the `covered_epoch` and resolved `round` in the context
so null-round failures identify the exact epoch and round that failed, while
preserving the existing `out.push` behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8a326c6-9c36-43a3-8c0d-c4cb35cd9434
📒 Files selected for processing (2)
src/beacon/drand.rssrc/beacon/tests/drand.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Aside from some nits, looks good! Let's have a changelog entry. This is not a transparent change.
|
@EclesioMeloJunior There's also another thing that'd be great to tackle, basically answering a question "why wasn't it caught before"; the RPC parity tests should've eventually caught it. Perhaps we're missing some test cases there? forest/src/tool/subcommands/api_cmd/api_compare_tests.rs Lines 1129 to 1133 in 81f6cba |
…inSafe/forest into fix/beacon-entries-after-null-rounds
Summary of changes
After the quicknet upgrade Forest was returning the incorrect set of
BeaconEntriesas it was hard-coded to return a single entry, while when a null-round happens a Filecoin block should be able to cover every epoch since its parent and one beacon entry per covered epochChanges introduced in this pull request:
src/beacon/drand.rs: basicaly on the quicknet "unchained" branch fetch themax_beacon_roundfrom(parent_epoch + 1)tillepochReference issue to close (if applicable)
Closes #7412
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Tests
Documentation