Impl state fork handler in state manager call_raw#7061
Open
sudo-shashank wants to merge 13 commits into
Open
Conversation
Contributor
WalkthroughForest now detects and rejects RPC calls that would trigger expensive protocol upgrade migrations. Network configuration marks which protocol heights are expensive via ChangesExpensive Fork Handling in State Simulation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
sudo-shashank
changed the title
prepare_parent_state in state manager call_raw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/state_manager/message_simulation.rs`:
- Around line 42-44: The call to exec.prepare_parent_state(genesis_timestamp,
VMTrace::NotTraced, &mut no_cb)? lacks any error context; change it to use
anyhow::Context so failures include call-site info (e.g., use
exec.prepare_parent_state(...).context(format!("failed to prepare parent state
for genesis_timestamp={}", genesis_timestamp))?), ensuring the returned
(state_cid, _, _) bind remains the same and still handles the Result from
prepare_parent_state with context attached.
🪄 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: 418976d3-f7ec-4592-a04e-233e6206d71f
📒 Files selected for processing (1)
src/state_manager/message_simulation.rs
sudo-shashank
changed the title
prepare_parent_state in state manager call_raw
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/networks/mod.rs`:
- Around line 542-550: The has_expensive_fork_between function incorrectly
includes the parent epoch in its search window causing a migration at parent to
make the next tipset appear to require migration; update the lower bound check
so it excludes parent by using info.epoch > parent (instead of info.epoch >=
parent) when scanning self.height_infos.values() for info.expensive, i.e. change
the predicate in has_expensive_fork_between to info.expensive && info.epoch >
parent && info.epoch < height so parent-side migrations are not double-counted.
In `@src/state_manager/errors.rs`:
- Around line 15-17: The error message for the ExpensiveFork variant is not
self-contained because it mentions "at epoch" but the variant carries no epoch;
fix by either (A) adding an epoch field to the variant (e.g., change
ExpensiveFork to ExpensiveFork { epoch: u64 } or similar type and update the
#[error(...)] to include the epoch placeholder like #[error("refusing explicit
call due to state fork at epoch {epoch}")] and then update all places that
construct ExpensiveFork to pass the epoch, or (B) remove the dangling suffix by
changing the #[error(...)] text to a self-contained message such as
#[error("refusing explicit call due to state fork")]; touch the ExpensiveFork
variant and its usages accordingly to keep types/constructors consistent.
In `@src/state_manager/message_simulation.rs`:
- Around line 30-56: The code currently performs the expensive-fork check
(chain_config.has_expensive_fork_between) and runs migration prep even when the
caller supplied a state (state_cid is Some), which can reject or re-mutate a
caller-provided post-fork state; in call_on_state, guard the expensive-fork
rejection and run_state_migrations() so they only execute when
state_cid.is_none() (i.e., when we derive state from a tipset), by moving the
chain_config.has_expensive_fork_between checks and the run_state_migrations()
calls inside the state_cid.is_none() branch; apply the same change for the other
occurrence around the code referenced at 58-79 and keep references to
chain_index().load_required_tipset, has_expensive_fork_between,
run_state_migrations, and call_on_state to locate the spots to change.
🪄 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: 8082d114-8347-4a81-b4bb-2fa3b521a995
📒 Files selected for processing (3)
src/networks/mod.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rs
sudo-shashank
force-pushed
the
shashank/fix-call-raw
branch
from
c2ff120 to
23e2128
Compare
sudo-shashank
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/networks/mod.rs (1)
731-737: ⚡ Quick winPin the inclusive lower-bound case in this regression test.
This currently locks in the exclusive upper bound only. Adding an assertion like
assert!(cfg.has_expensive_fork_between(shark, shark + 1));would protect the intended[parent, height)behavior from regressing again.Based on learnings:
ChainConfig::has_expensive_fork_between(parent, height)intentionally uses an inclusive lower bound matching Lotus’s[parent, height)semantics.🤖 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/networks/mod.rs` around lines 731 - 737, Update the test has_expensive_fork_between_matches_upgrade_epochs to pin the inclusive lower-bound case by adding an assertion that the fork exists at the lower bound: insert assert!(cfg.has_expensive_fork_between(shark, shark + 1)); inside the test (the test uses ChainConfig::mainnet(), variable shark = cfg.epoch(Height::Shark)) so update that function to include this new assertion that calls ChainConfig::has_expensive_fork_between with (shark, shark + 1).
🤖 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 `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 2450-2456: The code computing expensive_fork_epoch uses
.max().expect(...) which can panic; update this in the helper that returns
anyhow::Result to return an error instead of aborting: replace the
.max().expect(...) pattern on
chain_config.height_infos.values().filter(...).map(...).max() with
.ok_or_else(|| anyhow::anyhow!("calibnet must define at least one expensive
fork"))? so the function returns Err(...) when no epoch is found, propagating
the anyhow::Result; keep references to expensive_fork_epoch, chain_config,
heaviest_tipset, and height_infos so the change is localized.
---
Nitpick comments:
In `@src/networks/mod.rs`:
- Around line 731-737: Update the test
has_expensive_fork_between_matches_upgrade_epochs to pin the inclusive
lower-bound case by adding an assertion that the fork exists at the lower bound:
insert assert!(cfg.has_expensive_fork_between(shark, shark + 1)); inside the
test (the test uses ChainConfig::mainnet(), variable shark =
cfg.epoch(Height::Shark)) so update that function to include this new assertion
that calls ChainConfig::has_expensive_fork_between with (shark, shark + 1).
🪄 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: 4821a8ea-e361-4550-aaaf-255a36880eac
📒 Files selected for processing (6)
src/networks/mod.rssrc/rpc/methods/eth.rssrc/rpc/methods/state.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2456-2456: ⚡ Quick winEnhance error message with diagnostic context.
The change from
.expect()to.ok_or_else()correctly addresses the past review feedback. However, the error message could be more diagnostic by including the heaviest tipset epoch, as suggested in the original review comment.📋 Suggested improvement for diagnostics
- .ok_or_else(|| anyhow::anyhow!("calibnet must define at least one expensive fork"))?; + .ok_or_else(|| { + anyhow::anyhow!( + "no expensive fork epoch <= heaviest tipset epoch {} for calibnet snapshot", + heaviest_tipset.epoch() + ) + })?;🤖 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/tool/subcommands/api_cmd/api_compare_tests.rs` at line 2456, The error created by .ok_or_else currently uses a generic message "calibnet must define at least one expensive fork"; update the closure to include diagnostic context by appending the heaviest tipset epoch (e.g., heaviest_tipset_epoch or heaviest_tipset.epoch()) so the error shows the epoch value when no expensive fork is found; locate the call site where .ok_or_else(|| anyhow::anyhow!(...)) is used and interpolate the existing heaviest tipset variable into the anyhow message to produce a clearer, actionable error.
🤖 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/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Line 2456: The error created by .ok_or_else currently uses a generic message
"calibnet must define at least one expensive fork"; update the closure to
include diagnostic context by appending the heaviest tipset epoch (e.g.,
heaviest_tipset_epoch or heaviest_tipset.epoch()) so the error shows the epoch
value when no expensive fork is found; locate the call site where .ok_or_else(||
anyhow::anyhow!(...)) is used and interpolate the existing heaviest tipset
variable into the anyhow message to produce a clearer, actionable error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3494d18e-f244-4632-b2a5-c3084f080270
📒 Files selected for processing (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs
hanabi1224
reviewed
May 20, 2026
| pub const fn is_expensive_migration(height: Height) -> bool { | ||
| matches!( | ||
| height, | ||
| Height::Assembly |
Contributor
There was a problem hiding this comment.
How about *Fix migrations on calibnet?
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
2 participants
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.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6446
Closes #5965
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Tests