Skip to content

fix: fix invalid conversion when syncing from genesis on calibnet#6431

Merged
LesnyRumcajs merged 1 commit intomainfrom
fix-panic-from-genesis-calibnet
Jan 16, 2026
Merged

fix: fix invalid conversion when syncing from genesis on calibnet#6431
LesnyRumcajs merged 1 commit intomainfrom
fix-panic-from-genesis-calibnet

Conversation

@LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Jan 16, 2026

Summary of changes

Changes introduced in this pull request:

  • it's a workaround but it should be fine for now. Long term solution would be to remove the panics from Address shim, but this would incur cost for all shim-to-type conversions; given this fails only for FVM2 which is seldom used, this doesn't make economical sense.

Reference issue to close (if applicable)

Closes #6430

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a panic occurring when syncing from genesis on the calibration network.
    • Added validation to reject unsupported delegated addresses with appropriate error messaging in FVMv2 environments.

✏️ Tip: You can customize this high-level summary in your review settings.

@LesnyRumcajs LesnyRumcajs requested a review from a team as a code owner January 16, 2026 10:34
@LesnyRumcajs LesnyRumcajs requested review from akaladarshi and hanabi1224 and removed request for a team January 16, 2026 10:34
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Walkthrough

The changes add a validation guard in FvmV2's StateTree::get_actor to reject Delegated addresses with an error message, preventing a panic during genesis snapshot import. A changelog entry documents this fix for issue #6430.

Changes

Cohort / File(s) Summary
Documentation
CHANGELOG.md
Added changelog entry under "Fixed" for issue #6430 documenting the panic fix during genesis sync on calibration network
Core Logic
src/shim/state_tree.rs
Added preliminary validation in StateTree::get_actor for FvmV2 variant to explicitly reject Delegated addresses with error message "Delegated addresses are not supported in FVMv2 state trees"

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • hanabi1224
  • akaladarshi
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The PR partially addresses issue #6430: it fixes the immediate panic for FVM2 delegated addresses, but does not include regression tests or CI checks as specified in the issue requirements. Verify whether regression tests and CI checks covering snapshot import and address conversion paths, including FVM4 addresses, have been added or are expected in a follow-up PR.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly summarizes the main change: fixing an invalid conversion issue when syncing from genesis on calibnet, which matches the core objective of resolving the address conversion panic.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the issue: CHANGELOG.md documents the fix, and src/shim/state_tree.rs implements the delegated address guard for FVM2 as a targeted workaround.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f665ed0 and 43acc80.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • src/shim/state_tree.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use anyhow::Result<T> for most operations and add context with .context() when operations fail
Use tokio::task::spawn_blocking for CPU-intensive work such as VM execution and cryptography operations
Use .get() instead of indexing with [index] for safe element access
Do not use unwrap() in production code; use ? or expect() with descriptive messages instead
Do not use dbg! macro in non-test Rust code
Do not use todo! macro in non-test Rust code
Use strum crate for enum string conversions
Use derive_more crate for common trait implementations
Document all public functions and structs with doc comments
Use #[cfg(test)] or #[cfg(feature = "doctest-private")] for test-only code
Use channel-based communication (flume, tokio channels) between async tasks
Use consistent formatting following cargo fmt standards
Ensure code passes cargo clippy --all-targets --no-deps -- --deny=warnings linter checks

Files:

  • src/shim/state_tree.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 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: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.

Applied to files:

  • src/shim/state_tree.rs
📚 Learning: 2026-01-05T12:54:40.850Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.

Applied to files:

  • src/shim/state_tree.rs
📚 Learning: 2026-01-05T12:56:13.802Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.

Applied to files:

  • src/shim/state_tree.rs
🧬 Code graph analysis (1)
src/shim/state_tree.rs (1)
src/shim/address.rs (1)
  • protocol (171-173)
⏰ 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). (8)
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: All lint checks
  • GitHub Check: rubocop
  • GitHub Check: tests-release
  • GitHub Check: Coverage
🔇 Additional comments (2)
CHANGELOG.md (1)

63-64: Changelog entry looks good.
Concise, user-facing, and correctly references the issue.

src/shim/state_tree.rs (1)

209-217: Verify other FvmV2 address conversions can’t still panic.
The new guard blocks Delegated in get_actor, but lookup_id and set_actor for FvmV2 still do addr.into(), which likely panics on Delegated. Please confirm delegated addresses never reach those paths during import/sync, or add a shared guard there as well.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.51%. Comparing base (ebd6f37) to head (43acc80).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/shim/state_tree.rs 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/shim/state_tree.rs 56.38% <75.00%> (+0.23%) ⬆️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd6f37...43acc80. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Jan 16, 2026
Merged via the queue into main with commit a51d976 Jan 16, 2026
51 checks passed
@LesnyRumcajs LesnyRumcajs deleted the fix-panic-from-genesis-calibnet branch January 16, 2026 11:51
@coderabbitai coderabbitai bot mentioned this pull request Feb 5, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression while running forest calibnet node from genesis

2 participants