fix: improve error messages in forest-cli state compute
#6073
Conversation
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
Contributor
WalkthroughAdds an explicit “root not found” error path in StateTree::new_from_root when the root CID is absent from the store; preserves existing unsupported-version behavior when the root exists but cannot be constructed. Wraps errors from apply_block_messages in compute_tipset_state_blocking with the tipset epoch for clearer context. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant StateTree
participant Store
Caller->>StateTree: new_from_root(root_cid)
StateTree->>Store: try construct as V4/V3/V2/V0
alt constructed
StateTree-->>Caller: Ok(StateTree)
else not constructed
StateTree->>Store: has(root_cid)?
alt not found
Note over StateTree: New explicit missing-root error path
StateTree-->>Caller: Err("No state tree exists for the root {cid}.")
else found
StateTree->>Store: read StateRoot & detect version
alt unsupported
StateTree-->>Caller: Err("Can't create a valid state tree... unsupported version")
else recognized
StateTree-->>Caller: Ok(StateTree)
end
end
end
sequenceDiagram
autonumber
participant Caller
participant StateManager
participant Executor as apply_block_messages
Caller->>StateManager: compute_tipset_state_blocking(tipset)
StateManager->>StateManager: epoch = tipset.epoch()
StateManager->>Executor: apply_block_messages(...)
alt success
Executor-->>StateManager: StateOutput
StateManager-->>Caller: Ok(StateOutput)
else error
Executor-->>StateManager: Err(e)
Note over StateManager: Wrap with "Failed to compute tipset state@{epoch}: {e}"
StateManager-->>Caller: Err(anyhow!(...))
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state_manager/mod.rs (1)
860-872: Preserve the 'replay_halt' sentinel — don’t wrap errors when a callback is presentWrapping the error here changes "replay_halt" into "Failed to compute tipset state@{epoch}: replay_halt" and breaks the exact equality check used in the replay path.
File: src/state_manager/mod.rs — change compute_tipset_state_blocking (≈lines 854–872). Sentinel defined/checked near line ~730 and the replay call at ~753–756.
pub fn compute_tipset_state_blocking( &self, tipset: Arc<Tipset>, callback: Option<impl FnMut(MessageCallbackCtx<'_>) -> anyhow::Result<()>>, enable_tracing: VMTrace, ) -> Result<StateOutput, Error> { - let epoch = tipset.epoch(); - Ok(apply_block_messages( + let epoch = tipset.epoch(); + let has_callback = callback.is_some(); + Ok(apply_block_messages( self.chain_store().genesis_block_header().timestamp, Arc::clone(&self.chain_store().chain_index), Arc::clone(&self.chain_config), self.beacon_schedule().clone(), &self.engine, tipset, callback, enable_tracing, - ) - .map_err(|e| anyhow::anyhow!("Failed to compute tipset state@{epoch}: {e}"))?) + ) + .map_err(|e| { + if has_callback { + e + } else { + anyhow::anyhow!("Failed to compute tipset state@{epoch}: {e}") + } + })?) }
🧹 Nitpick comments (1)
src/shim/state_tree.rs (1)
174-176: Good early-exit for missing state roots; consider adding an actionable hintThe explicit has-check cleanly distinguishes “root missing” from “unsupported version,” which aligns with fail-fast preferences. To better guide stateless-node users running
forest-cli state compute, consider appending a brief hint.Apply this minimal tweak to the error string:
- bail!("No state tree found with root {c}.") + bail!("No state tree found with root {c}. If running a stateless node, this operation is unsupported; use a recent snapshot or a stateful node.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/shim/state_tree.rs(1 hunks)src/state_manager/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
📚 Learning: 2025-08-11T14:00:47.060Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
Applied to files:
src/shim/state_tree.rs
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.
Applied to files:
src/state_manager/mod.rs
🧬 Code graph analysis (2)
src/shim/state_tree.rs (1)
src/shim/state_tree_v0.rs (1)
store(56-58)
src/state_manager/mod.rs (1)
src/blocks/tipset.rs (2)
epoch(306-308)epoch(543-545)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
sudo-shashank
previously approved these changes
Sep 12, 2025
akaladarshi
reviewed
Sep 12, 2025
src/shim/state_tree.rs
Outdated
| } else if let Ok(st) = super::state_tree_v0::StateTreeV0::new_from_root(store.clone(), c) { | ||
| Ok(StateTree::V0(st)) | ||
| } else if !store.has(c)? { | ||
| bail!("No state tree found with root {c}.") |
Collaborator
There was a problem hiding this comment.
bail!("No state tree exist for the root {c}.") ?
LesnyRumcajs
approved these changes
Sep 15, 2025
akaladarshi
approved these changes
Sep 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
New error message:
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6063
Other information and links
Change checklist
Summary by CodeRabbit