fix(blocks): validate all beacon entries for all covered epochs on quicknet - #7441
fix(blocks): validate all beacon entries for all covered epochs on quicknet #7441EclesioMeloJunior wants to merge 14 commits into
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 unchained beacon validation now requires one ascending entry per epoch between the parent and block epochs. Each entry must use that epoch’s maximum beacon round. Public test helpers and coverage for contiguous and null-round epochs were added. ChangesQuicknet beacon validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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 |
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 `@CHANGELOG.md`:
- Line 44: Update the changelog entry for Quicknet block validation to reference
issue `#7413` instead of `#7412`, including the corresponding issue URL, while
preserving the existing description.
In `@src/blocks/header.rs`:
- Around line 188-223: Update src/beacon/drand.rs in
BeaconSchedule::beacon_entries_for_block to generate one unchained beacon entry
for every epoch in parent_epoch + 1..=epoch, matching the validator’s expected
count and rounds. In src/blocks/header.rs:188-223, retain validation against
that full range; in src/blocks/header.rs:481-517, construct the null-epoch
header using BeaconSchedule::beacon_entries_for_block so the test exercises the
producer-validator contract.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9cb33ea-85fe-4045-b24e-403c651a75fd
📒 Files selected for processing (4)
CHANGELOG.mdsrc/beacon/mod.rssrc/beacon/tests/drand.rssrc/blocks/header.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
nit: there are some partially covered lines in the coverage you might want to cover (they're fairly trivial to reason about, so not a huge deal). I'll approve once CI is green. |
Summary of changes
This PR fixes the
RawBlockHeader::validate_block_drandmethod by checking correctly, for quicknet, if the current block header contains the beacon entries fromparent_epoch + 1tillself.epoch, meaning that a block built afterNnull-epoch should have a beacon entry per skipped epochChanges introduced in this pull request:
src/blocks/header.rs:validate_block_drandnow checks, for unchained beacons only, that the header carries exactlyepoch - parent_epochentries and that entryiis atmax_beacon_round_for_epoch(parent_epoch + i + 1)Reference issue to close (if applicable)
Closes #7413
Other information and links
MinerGetBaseInforeturns the correctBeaconEntries#7427 fixes the producing side, whereFilecoin.MinerGetBaseInforeturned too few entries for the same reason. This PR is the validating side.Change checklist
Outside contributions
Summary by CodeRabbit