fix: ensure the result state tree is loadable in ForestStateCompute#6871
fix: ensure the result state tree is loadable in ForestStateCompute#6871hanabi1224 merged 7 commits intomainfrom
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 ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughForest.StateCompute RPC gains an optional Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant RPC as Forest.StateCompute
participant SM as StateManager
participant VM as VM / compute_tipset_state
participant ST as StateTree
CLI->>RPC: request(epoch, n_epochs, force_recompute)
alt force_recompute == false
RPC->>SM: load_executed_tipset(ts)
SM-->>RPC: ExecutedTipset(state_root) / None
alt received state_root
RPC->>ST: StateTree::new_from_root(state_root)
alt valid
ST-->>RPC: OK
RPC-->>CLI: return cached state_root
else invalid
ST-->>RPC: Err
RPC->>VM: compute_tipset_state(ts, NO_CALLBACK, NotTraced)
VM-->>RPC: computed_state_root
RPC->>ST: StateTree::new_from_root(computed_state_root)
ST-->>RPC: OK
RPC-->>CLI: return recomputed state_root
end
else no cached
RPC->>VM: compute_tipset_state(ts, NO_CALLBACK, NotTraced)
VM-->>RPC: computed_state_root
RPC->>ST: StateTree::new_from_root(computed_state_root)
ST-->>RPC: OK
RPC-->>CLI: return recomputed state_root
end
else force_recompute == true
RPC->>VM: compute_tipset_state(ts, NO_CALLBACK, NotTraced)
VM-->>RPC: computed_state_root
RPC->>ST: StateTree::new_from_root(computed_state_root)
ST-->>RPC: OK
RPC-->>CLI: return recomputed state_root
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/state.rs (1)
1592-1603: Sound defensive logic for handling incomplete state trees.The approach of validating the cached state tree and falling back to recomputation is appropriate for the stated objective of handling bad/wrong diff snapshot imports.
Two minor suggestions:
Add error context on line 1601 — per coding guidelines, errors should include context for debuggability.
Optional: Add a debug/info log when fallback occurs — this would help diagnose when incomplete state trees are encountered in production.
🔧 Proposed refinement
let ExecutedTipset { state_root, .. } = ctx.state_manager.load_executed_tipset(&ts).await?; let state_root = if StateTree::new_from_root(ctx.store_owned(), &state_root).is_ok() { state_root } else { + tracing::debug!(epoch = ts.epoch(), "Cached state tree unloadable, recomputing"); let ExecutedTipset { state_root, .. } = ctx .state_manager .compute_tipset_state(ts, NO_CALLBACK, VMTrace::NotTraced) .await?; - _ = StateTree::new_from_root(ctx.store_owned(), &state_root)?; + _ = StateTree::new_from_root(ctx.store_owned(), &state_root) + .context("recomputed state tree is not loadable")?; state_root };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state.rs` around lines 1592 - 1603, The code validates a cached state root by calling StateTree::new_from_root and falls back to ctx.state_manager.compute_tipset_state; add contextual error messages and a fallback log: when the second StateTree::new_from_root(...)? is used, wrap its error with context (e.g., include the tipset id/ts and mention "failed validating recomputed state root") so the propagated error includes debug info, and insert a debug/info log right before calling compute_tipset_state(...) indicating fallback due to invalid cached state (referencing ExecutedTipset, StateTree::new_from_root, compute_tipset_state, ctx.state_manager, ctx.store_owned, NO_CALLBACK, VMTrace::NotTraced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rpc/methods/state.rs`:
- Around line 1592-1603: The code validates a cached state root by calling
StateTree::new_from_root and falls back to
ctx.state_manager.compute_tipset_state; add contextual error messages and a
fallback log: when the second StateTree::new_from_root(...)? is used, wrap its
error with context (e.g., include the tipset id/ts and mention "failed
validating recomputed state root") so the propagated error includes debug info,
and insert a debug/info log right before calling compute_tipset_state(...)
indicating fallback due to invalid cached state (referencing ExecutedTipset,
StateTree::new_from_root, compute_tipset_state, ctx.state_manager,
ctx.store_owned, NO_CALLBACK, VMTrace::NotTraced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e85008e9-5e61-4800-878b-649245822ab1
📒 Files selected for processing (1)
src/rpc/methods/state.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@hanabi1224 Shouldn't this API always re-compute regardless if the state root is present or not? Also can you add some documentation for the |
@akaladarshi description added. This one is typically used by Update: |
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 `@src/rpc/methods/state.rs`:
- Around line 1532-1534: Update the DESCRIPTION constant string to accurately
reflect actual behavior: state cache is only reused when
StateTree::new_from_root(...) succeeds (make explicit that cache reuse depends
on successful state tree reconstruction), and explain that this Forest-specific
range recomputation uses on-chain messages from the tipsets rather than
caller-supplied messages (contrast with Filecoin.StateCompute). Edit the
DESCRIPTION const to include both points and reference StateTree::new_from_root
and Filecoin.StateCompute so readers understand the difference.
- Around line 1597-1604: When rebuilding the state root fails, wrap the errors
from StateTree::new_from_root and state_manager.compute_tipset_state with
context including the tipset epoch so callers know which epoch failed; modify
the branch that currently calls StateTree::new_from_root(ctx.store_owned(),
&state_root) and the call to ctx.state_manager.compute_tipset_state(ts,
NO_CALLBACK, VMTrace::NotTraced).await to use .context(format!("... epoch {}",
ts.epoch())) (or similar) on the Result so both the StateTree::new_from_root and
compute_tipset_state failures include the epoch in their anyhow::Error context.
🪄 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: e1c0551a-c12c-4a25-a1ef-e0e13b1ca916
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/rpc/methods/state.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/subcommands/state_cmd.rs`:
- Around line 37-38: The force field in the CLI struct (field name: force) is
currently an Option<bool> and missing the #[arg(long)] attribute so clap treats
it as a positional; update the declaration for the force field by adding the
#[arg(long)] attribute and make it a simple bool (false by default, true when
--force is passed) to behave as a proper --force flag used by the state command
handlers (look for the struct that defines force in state_cmd.rs and its
usages).
🪄 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: 3c3265c3-deb9-4696-b4df-afe6cb30618e
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
CHANGELOG.mdsrc/cli/subcommands/state_cmd.rssrc/rpc/methods/state.rs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
3 participants
Summary of changes
There's a chance that an incomplete state tree presents (e.g. from bad/wrong diff snapshot import).
ForestStateComputeshould verify the result state tree is loadable and recompute when needed.Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes