Uh oh!
There was an error while loading. Please reload this page.
fix: adjust state computation policy for Eth RPC methods - #7116
Conversation
WalkthroughThis PR introduces an environment-controlled policy to gate state recomputation when executed-tipset data is missing from storage. A new ChangesRPC State Recomputation Policy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
dda3dac to
42586feCompareUh oh!
There was an error while loading. Please reload this page.
LesnyRumcajs
left a comment
There was a problem hiding this comment.
This is a functional breaking change, hence a CHANGELOG entry is required.
There was a problem hiding this comment.
Actionable comments posted: 1
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/state_computation.rs (1)
58-79:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKey the executed-tipset cache by recompute policy or provenance.
load_executed_tipset_for_rpcandload_executed_tipsetnow have different semantics, but this cache is still keyed only byts.key(). If any non-RPC path warms the cache through theAllowedbranch, a later RPC call with recomputation disabled will reuse that recomputed entry instead of returning the intended index-miss error. That makes the new policy depend on cache history rather than the configured setting.🤖 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/state_manager/state_computation.rs` around lines 58 - 79, The cache in load_executed_tipset_with_cache is keyed only by ts.key(), causing recompute policy to leak across callers (e.g., load_executed_tipset_for_rpc vs load_executed_tipset); fix by incorporating the recompute provenance into the cache key or by validating stored provenance before returning: modify the cache access sites (calls to self.cache.get(...) and self.cache.get_or_insert_async(...)) to use a composite key that includes ts.key() plus the Policy/ provenance (or keep a separate cache per provenance), or have ExecutedTipset carry provenance and check it against the current policy before returning and evict if mismatched. Ensure changes touch load_executed_tipset_with_cache, load_executed_tipset_for_rpc, and load_executed_tipset to preserve semantics.
🤖 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/state_computation.rs`:
- Around line 138-144: Current code unconditionally bails when event AMT is
unreadable unless allow_state_compute is true, causing RPCs that only need
state_root/message/receipt to fail; change the loader used in StateComputation
(the branch using allow_state_compute and the anyhow::bail! with
msg_ts.epoch()/msg_ts.key()) so it does not hard-fail for missing/unreadable
event trees when recomputation is disabled—instead return a lightweight tipset
state result that includes state_root/messages/receipts but leaves events as
None or a lazy-hydration handle; add a separate function (e.g.,
load_tipset_with_events or hydrate_events_for_tipset) that performs the
expensive AMT/event decoding and is only called by callers that need events
(update callers like Block::from_filecoin_tipset_with_full_tx and
eth_fee_history to call this explicit hydrator), and ensure allow_state_compute
still gates full hydration and the original bail remains inside that
full-hydration path.
---
Outside diff comments:
In `@src/state_manager/state_computation.rs`:
- Around line 58-79: The cache in load_executed_tipset_with_cache is keyed only
by ts.key(), causing recompute policy to leak across callers (e.g.,
load_executed_tipset_for_rpc vs load_executed_tipset); fix by incorporating the
recompute provenance into the cache key or by validating stored provenance
before returning: modify the cache access sites (calls to self.cache.get(...)
and self.cache.get_or_insert_async(...)) to use a composite key that includes
ts.key() plus the Policy/ provenance (or keep a separate cache per provenance),
or have ExecutedTipset carry provenance and check it against the current policy
before returning and evict if mismatched. Ensure changes touch
load_executed_tipset_with_cache, load_executed_tipset_for_rpc, and
load_executed_tipset to preserve semantics.
🪄 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: 84b3a48c-1360-4810-aca0-15c2aac7184f
📒 Files selected for processing (5)
docs/docs/users/reference/env_variables.mdscripts/tests/api_compare/docker-compose.ymlsrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mod.rssrc/state_manager/state_computation.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
68-68: ⚡ Quick winDocument the
unsafeblock and consider the LazyLock pattern.The
unsafe { std::env::set_var(...) }call lacks a comment explaining why it's acceptable. The existingINIT_RNG_SEEDpattern (lines 221–225) demonstrates best practice: use aLazyLockwith a comment noting thatstd::env::set_varis not thread-safe on Linux. Although the learning confirms that nextest runs tests in separate processes (mitigating cross-test races), the safety justification should still be documented for reviewers and maintainers.Additionally, consider adopting the
LazyLockpattern for consistency and to avoid redundantunsafecalls if this function is invoked multiple times.📝 Suggested improvement with LazyLock pattern
Add a static initializer following the existing pattern:
+ // Ensure FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS is set once per process.+ // `std::env::set_var` is not thread-safe on Linux, so initialize once+ // per process — never concurrently from parallel tests.+ static INIT_ETH_RPC_COMPUTE_STATE: LazyLock<()> = LazyLock::new(|| {+ if std::env::var("FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS").is_err() {+ unsafe { std::env::set_var("FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS", "1") };+ }+ });+ pub async fn run_test_from_snapshot(path: &Path) -> anyhow::Result<()> { - unsafe { std::env::set_var("FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS", "1") };+ LazyLock::force(&INIT_ETH_RPC_COMPUTE_STATE); let mut run = false;🤖 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/test_snapshot.rs` at line 68, Document the unsafe std::env::set_var call and replace the ad-hoc unsafe call with a static LazyLock initializer similar to INIT_RNG_SEED: add a static (e.g., INIT_FOREST_ETH_RPC using std::sync::LazyLock) whose initializer performs the unsafe { std::env::set_var("FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS","1") } and include a comment explaining that setting env vars is not thread-safe on Linux and why a single-process/test-run justification still makes this safe; then remove the direct unsafe call and ensure callers trigger the static (consistent pattern with INIT_RNG_SEED).
🤖 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/test_snapshot.rs`:
- Line 68: Document the unsafe std::env::set_var call and replace the ad-hoc
unsafe call with a static LazyLock initializer similar to INIT_RNG_SEED: add a
static (e.g., INIT_FOREST_ETH_RPC using std::sync::LazyLock) whose initializer
performs the unsafe {
std::env::set_var("FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISS","1") } and
include a comment explaining that setting env vars is not thread-safe on Linux
and why a single-process/test-run justification still makes this safe; then
remove the direct unsafe call and ensure callers trigger the static (consistent
pattern with INIT_RNG_SEED).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb980bcd-6620-47f4-b847-7ce191a03e1e
📒 Files selected for processing (3)
CHANGELOG.mdsrc/state_manager/state_computation.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/state_manager/state_computation.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Actionable comments posted: 0 |
Uh oh!
There was an error while loading. Please reload this page.
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/state_computation.rs`:
- Line 50: Update the tracking issue link in the inline comment that currently
points to "https://github.com/ChainSafe/forest/issues/7118" and change it to
"https://github.com/ChainSafe/forest/issues/7117" so the comment references the
correct follow-up issue for removing the test-only override (locate the comment
in state_computation.rs and replace the issue number).
🪄 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: 41f0f5c0-9c20-4237-928f-bc02dc9eeb5a
📒 Files selected for processing (1)
src/state_manager/state_computation.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Breaking Changes
New Features
FOREST_ETH_RPC_COMPUTE_STATE_ON_INDEX_MISSenvironment variable to enable state computation for Ethereum RPC methods when needed.Chores