Blocking tipset validation#6898
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCompute an explicit validation start epoch for snapshot import, offload range validation to a blocking worker via tokio::spawn_blocking, convert tipset outer-loop validation from parallel to sequential while keeping inner rayon parallelism, and add a CHANGELOG entry for issue Changes
Sequence Diagram(s)sequenceDiagram
participant Daemon
participant Tokio as "Tokio runtime\n(spawn_blocking)"
participant StateMgr as "StateManager\n(validate_range)"
Daemon->>Tokio: spawn_blocking(|| state_mgr.validate_range(range))
Tokio->>StateMgr: call validate_range(range)
StateMgr-->>Tokio: return Result<()>
Tokio-->>Daemon: join and return Result (propagate errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
✨ Simplify code
Comment |
3033c2d to
2c9b847
Compare
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 `@CHANGELOG.md`:
- Line 58: Update the CHANGELOG entry to reference the issue `#6893` instead of PR
`#6898`, correct the typo “occassional” to “occasional”, and replace “snapshot
validation” with “tipset validation” so the line reads using the issue link
format [`#6893`](https://github.com/ChainSafe/forest/issues/6893): Fixed
occasional lock contention during tipset validation.
In `@src/daemon/mod.rs`:
- Around line 179-184: The computed start value (assigned to start from
validate_from and current_height) must be clamped and validated: when
validate_from is negative compute start =
current_height.saturating_add(validate_from) but clamp to 0 (genesis) so it
cannot underflow, and when validate_from is positive ensure start <=
current_height and return or error if start > current_height so
validate_range(start..=current_height) is not a silent no-op; update the logic
around validate_from/current_height/start and add an explicit rejection path
(error/Exit) for start > current_height.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d857ec5d-aeab-49ee-bca6-ba84a0ea6826
📒 Files selected for processing (3)
CHANGELOG.mdsrc/daemon/mod.rssrc/state_manager/mod.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary of changes
Changes introduced in this pull request:
rayonparallel loop we introduced introduces a chance for wedges in the crates we're using; for example, infilecoin-proofswe end up with a deadlock on an SRS file. There's also arayon-heavy loop inbellperson. Giving up on parallelization incurs a small performance penalty (200 tipsets on mainnet, ~20% slower) but it gets rid of entire class of issues. On machines with fewer cores (my test was done on 32 cores), the penalty should be smaller or it might even get faster.ctrl_chandler 🤡Reference issue to close (if applicable)
Closes #6893
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit