Implement expensive fork check #6423
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds detection of "expensive" state-migration epochs and blocks RPC execution that would cross such a fork by returning an ExpensiveFork error instead of performing the migration. Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC apply_message
participant TS as TipsetStore
participant SM as StateManager
participant Chain as ChainConfig
RPC->>TS: load parent tipset for provided tipset
RPC->>SM: has_expensive_fork_between(parent.epoch, tipset.epoch+1)
alt expensive fork detected
SM-->>RPC: Err::ExpensiveFork
RPC-->>Client: return error
else no expensive fork
RPC->>SM: apply_on_state_with_gas(...)
SM->>Chain: (may run migrations / state transitions)
SM-->>RPC: result
RPC-->>Client: return result
end
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. Comment |
sudo-shashank
requested review from
LesnyRumcajs,
akaladarshi and
hanabi1224
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/state_manager/errors.rs`:
- Around line 18-19: The ExpensiveFork enum variant's error message should match
Lotus and include a doc comment: update the #[error(...)] on the ExpensiveFork
variant in src/state_manager/errors.rs to use the text "refusing explicit call
due to state fork at epoch" and add a short /// doc comment above the
ExpensiveFork variant describing when this error is returned (e.g., returned
when an explicit call is refused due to a state fork at a particular epoch);
ensure the variant name ExpensiveFork remains unchanged and that the new doc
comment follows Rust doc comment conventions.
🧹 Nitpick comments (4)
src/state_manager/tests.rs (1)
11-11: Strengthen test to cover the “exclude target height” boundary.Right now the test only covers the positive case. Add a negative assertion to validate that an expensive migration at the target height is excluded (per the method docs/intent).
Proposed diff
-use crate::state_manager::get_expensive_migration_heights; +use crate::state_migration::get_expensive_migration_heights; #[test] fn test_has_expensive_fork_between() { let TestChainSetup { state_manager, .. } = setup_chain_with_tipsets(); let chain_config = state_manager.chain_config(); let expensive_epoch = get_expensive_migration_heights(&chain_config.network) .iter() .find_map(|height| { let epoch = chain_config.epoch(*height); (epoch > 0).then_some(epoch) }) .expect("expected at least one expensive migration epoch > 0"); + // Excludes migrations at the target height. + assert!(!state_manager.has_expensive_fork_between(expensive_epoch - 1, expensive_epoch)); assert!(state_manager.has_expensive_fork_between(expensive_epoch, expensive_epoch + 1)); }Also applies to: 438-452
src/rpc/methods/eth.rs (1)
57-57: Avoid cloning the tipset; use.context()for the parent load.Proposed diff
- if let Some(tipset) = tipset.clone() - && tipset.epoch() > 0 - { - let parent_ts = Tipset::load_required(ctx.store(), tipset.parents()) - .map_err(|e| anyhow::anyhow!("failed to load parent tipset: {e}"))?; + if let Some(tipset) = tipset.as_ref() + && tipset.epoch() > 0 + { + let parent_ts = Tipset::load_required(ctx.store(), tipset.parents()) + .context("failed to load parent tipset")?; ensure!( !ctx.state_manager .has_expensive_fork_between(parent_ts.epoch(), tipset.epoch() + 1), StateManagerError::ExpensiveFork ); }Also applies to: 2021-2032
src/state_manager/mod.rs (1)
462-472: Make this a doc comment (///) and consider an O(#expensive_epochs) implementation.This is a public method; repo guidelines call for doc comments. Also, iterating the set is enough (the set is tiny) and avoids scanning large epoch ranges.
Proposed diff
- // HasExpensiveForkBetween returns true where executing tipsets between the specified heights would - // trigger an expensive migration. - // Note: migrations occurring at the target height are not included, as they're executed after the target height. + /// Returns `true` if executing tipsets in `[parent, height)` would run an expensive migration. + /// + /// Note: migrations at `height` are excluded (they run after `height`). pub fn has_expensive_fork_between(&self, parent: ChainEpoch, height: ChainEpoch) -> bool { - for epoch in parent..height { - if self.expensive_migration_epochs.contains(&epoch) { - return true; - } - } - false + let range = parent..height; + self.expensive_migration_epochs + .iter() + .any(|&epoch| range.contains(&epoch)) }src/state_migration/mod.rs (1)
92-133: De-duplicate the common list using a constant.The arrays for Mainnet, Calibnet, and Devnet(_) are identical and should be extracted to a
const COMMON_EXPENSIVE_MIGRATIONSto reduce duplication and maintenance burden. The proposed refactoring is correct and does not change behavior.Note: Verify whether Calibnet's Fix migrations (WatermelonFix, WatermelonFix2, DragonFix, TockFix) should be included in the expensive set by cross-referencing Lotus's network upgrade definitions, as they are currently excluded while being present in
get_migrations().Proposed diff
+const COMMON_EXPENSIVE_MIGRATIONS: &[Height] = &[ + Height::Shark, + Height::Hygge, + Height::Lightning, + Height::Watermelon, + Height::Dragon, + Height::Waffle, + Height::TukTuk, + Height::Teep, + Height::GoldenWeek, +]; + /// Returns the heights at which expensive migrations occur. pub fn get_expensive_migration_heights(chain: &NetworkChain) -> &'static [Height] { match chain { - NetworkChain::Mainnet => &[ - Height::Shark, - Height::Hygge, - Height::Lightning, - Height::Watermelon, - Height::Dragon, - Height::Waffle, - Height::TukTuk, - Height::Teep, - Height::GoldenWeek, - ], - - NetworkChain::Calibnet => &[ - Height::Shark, - Height::Hygge, - Height::Lightning, - Height::Watermelon, - Height::Dragon, - Height::Waffle, - Height::TukTuk, - Height::Teep, - Height::GoldenWeek, - ], + NetworkChain::Mainnet | NetworkChain::Calibnet | NetworkChain::Devnet(_) => { + COMMON_EXPENSIVE_MIGRATIONS + } NetworkChain::Butterflynet => &[Height::Teep, Height::GoldenWeek], - - NetworkChain::Devnet(_) => &[ - Height::Shark, - Height::Hygge, - Height::Lightning, - Height::Watermelon, - Height::Dragon, - Height::Waffle, - Height::TukTuk, - Height::Teep, - Height::GoldenWeek, - ], } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/state_manager/errors.rssrc/state_manager/mod.rssrc/state_manager/tests.rssrc/state_migration/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when operations fail
Usetokio::task::spawn_blockingfor CPU-intensive work such as VM execution and cryptography operations
Use.get()instead of indexing with[index]for safe element access
Do not useunwrap()in production code; use?orexpect()with descriptive messages instead
Do not usedbg!macro in non-test Rust code
Do not usetodo!macro in non-test Rust code
Usestrumcrate for enum string conversions
Usederive_morecrate 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 followingcargo fmtstandards
Ensure code passescargo clippy --all-targets --no-deps -- --deny=warningslinter checks
Files:
src/state_manager/errors.rssrc/state_manager/tests.rssrc/state_migration/mod.rssrc/state_manager/mod.rssrc/rpc/methods/eth.rs
**/mod.rs
📄 CodeRabbit inference engine (AGENTS.md)
Each module should have a
mod.rsfile that exports the public API with private submodules for implementation details
Files:
src/state_migration/mod.rssrc/state_manager/mod.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
📚 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/state_manager/errors.rssrc/state_manager/tests.rssrc/state_migration/mod.rssrc/state_manager/mod.rssrc/rpc/methods/eth.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/state_manager/errors.rssrc/state_manager/tests.rssrc/state_migration/mod.rssrc/state_manager/mod.rssrc/rpc/methods/eth.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_manager/tests.rs
📚 Learning: 2025-08-18T03:09:47.932Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Applied to files:
src/state_manager/tests.rssrc/state_manager/mod.rssrc/rpc/methods/eth.rs
📚 Learning: 2025-10-17T14:24:47.046Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6167
File: src/tool/subcommands/state_compute_cmd.rs:89-91
Timestamp: 2025-10-17T14:24:47.046Z
Learning: In `src/tool/subcommands/state_compute_cmd.rs`, when using `ReadOpsTrackingStore` to generate minimal snapshots, `HEAD_KEY` should be written to `db.tracker` (not `db` itself) before calling `export_forest_car()`, because the export reads from the tracker MemoryDB which accumulates only the accessed data during computation.
Applied to files:
src/state_manager/mod.rssrc/rpc/methods/eth.rs
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/rpc/methods/eth.rs
🧬 Code graph analysis (2)
src/state_manager/tests.rs (2)
src/state_migration/mod.rs (1)
get_expensive_migration_heights(93-133)src/state_manager/mod.rs (1)
has_expensive_fork_between(465-472)
src/state_manager/mod.rs (1)
src/state_migration/mod.rs (2)
get_expensive_migration_heights(93-133)run_state_migrations(136-205)
⏰ 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). (3)
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Coverage
🔇 Additional comments (2)
src/state_manager/mod.rs (2)
59-65: Good: precompute expensive migration epochs during StateManager init.Keeps the runtime checks cheap and avoids repeated
Height -> epochmapping.Also applies to: 171-213
613-620: Pre-flight expensive fork guard incall_rawmatches the intent.Returning
Error::ExpensiveForkearly prevents RPC-triggered costly migrations in this path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| ensure!( | ||
| !ctx.state_manager | ||
| .has_expensive_fork_between(parent_ts.epoch(), tipset.epoch() + 1), |
There was a problem hiding this comment.
Why would it be 2 epochs in the range?
There was a problem hiding this comment.
i guess lotus does this to early detect an expensive fork
Summary of changes
Changes introduced in this pull request:
has_expensive_fork_betweento check if executing tipsets between specified heights would trigger an expensive migration, excluding migrations at the target height.ErrExpensiveForkerror , matching Lotus behavior, to prevent performance degradation.Reference issue to close (if applicable)
Closes #5965
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.