Impl state fork handler in state manager call_raw#7061
Conversation
WalkthroughForest now detects and rejects RPC calls that would trigger expensive protocol upgrade migrations. Network configuration marks which protocol heights are expensive via ChangesExpensive Fork Handling in State Simulation
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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
sudo-shashank
changed the title
prepare_parent_state in state manager call_raw
![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
🤖 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/message_simulation.rs`:
- Around line 42-44: The call to exec.prepare_parent_state(genesis_timestamp,
VMTrace::NotTraced, &mut no_cb)? lacks any error context; change it to use
anyhow::Context so failures include call-site info (e.g., use
exec.prepare_parent_state(...).context(format!("failed to prepare parent state
for genesis_timestamp={}", genesis_timestamp))?), ensuring the returned
(state_cid, _, _) bind remains the same and still handles the Result from
prepare_parent_state with context attached.
🪄 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: 418976d3-f7ec-4592-a04e-233e6206d71f
📒 Files selected for processing (1)
src/state_manager/message_simulation.rs
sudo-shashank
changed the title
prepare_parent_state in state manager call_raw
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/networks/mod.rs`:
- Around line 542-550: The has_expensive_fork_between function incorrectly
includes the parent epoch in its search window causing a migration at parent to
make the next tipset appear to require migration; update the lower bound check
so it excludes parent by using info.epoch > parent (instead of info.epoch >=
parent) when scanning self.height_infos.values() for info.expensive, i.e. change
the predicate in has_expensive_fork_between to info.expensive && info.epoch >
parent && info.epoch < height so parent-side migrations are not double-counted.
In `@src/state_manager/errors.rs`:
- Around line 15-17: The error message for the ExpensiveFork variant is not
self-contained because it mentions "at epoch" but the variant carries no epoch;
fix by either (A) adding an epoch field to the variant (e.g., change
ExpensiveFork to ExpensiveFork { epoch: u64 } or similar type and update the
#[error(...)] to include the epoch placeholder like #[error("refusing explicit
call due to state fork at epoch {epoch}")] and then update all places that
construct ExpensiveFork to pass the epoch, or (B) remove the dangling suffix by
changing the #[error(...)] text to a self-contained message such as
#[error("refusing explicit call due to state fork")]; touch the ExpensiveFork
variant and its usages accordingly to keep types/constructors consistent.
In `@src/state_manager/message_simulation.rs`:
- Around line 30-56: The code currently performs the expensive-fork check
(chain_config.has_expensive_fork_between) and runs migration prep even when the
caller supplied a state (state_cid is Some), which can reject or re-mutate a
caller-provided post-fork state; in call_on_state, guard the expensive-fork
rejection and run_state_migrations() so they only execute when
state_cid.is_none() (i.e., when we derive state from a tipset), by moving the
chain_config.has_expensive_fork_between checks and the run_state_migrations()
calls inside the state_cid.is_none() branch; apply the same change for the other
occurrence around the code referenced at 58-79 and keep references to
chain_index().load_required_tipset, has_expensive_fork_between,
run_state_migrations, and call_on_state to locate the spots to change.
🪄 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: 8082d114-8347-4a81-b4bb-2fa3b521a995
📒 Files selected for processing (3)
src/networks/mod.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rs
sudo-shashank
force-pushed
the
shashank/fix-call-raw
branch
from
c2ff120 to
23e2128
Compare
sudo-shashank
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/networks/mod.rs (1)
731-737: ⚡ Quick winPin the inclusive lower-bound case in this regression test.
This currently locks in the exclusive upper bound only. Adding an assertion like
assert!(cfg.has_expensive_fork_between(shark, shark + 1));would protect the intended[parent, height)behavior from regressing again.Based on learnings:
ChainConfig::has_expensive_fork_between(parent, height)intentionally uses an inclusive lower bound matching Lotus’s[parent, height)semantics.🤖 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/networks/mod.rs` around lines 731 - 737, Update the test has_expensive_fork_between_matches_upgrade_epochs to pin the inclusive lower-bound case by adding an assertion that the fork exists at the lower bound: insert assert!(cfg.has_expensive_fork_between(shark, shark + 1)); inside the test (the test uses ChainConfig::mainnet(), variable shark = cfg.epoch(Height::Shark)) so update that function to include this new assertion that calls ChainConfig::has_expensive_fork_between with (shark, shark + 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/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 2450-2456: The code computing expensive_fork_epoch uses
.max().expect(...) which can panic; update this in the helper that returns
anyhow::Result to return an error instead of aborting: replace the
.max().expect(...) pattern on
chain_config.height_infos.values().filter(...).map(...).max() with
.ok_or_else(|| anyhow::anyhow!("calibnet must define at least one expensive
fork"))? so the function returns Err(...) when no epoch is found, propagating
the anyhow::Result; keep references to expensive_fork_epoch, chain_config,
heaviest_tipset, and height_infos so the change is localized.
---
Nitpick comments:
In `@src/networks/mod.rs`:
- Around line 731-737: Update the test
has_expensive_fork_between_matches_upgrade_epochs to pin the inclusive
lower-bound case by adding an assertion that the fork exists at the lower bound:
insert assert!(cfg.has_expensive_fork_between(shark, shark + 1)); inside the
test (the test uses ChainConfig::mainnet(), variable shark =
cfg.epoch(Height::Shark)) so update that function to include this new assertion
that calls ChainConfig::has_expensive_fork_between with (shark, shark + 1).
🪄 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: 4821a8ea-e361-4550-aaaf-255a36880eac
📒 Files selected for processing (6)
src/networks/mod.rssrc/rpc/methods/eth.rssrc/rpc/methods/state.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2456-2456: ⚡ Quick winEnhance error message with diagnostic context.
The change from
.expect()to.ok_or_else()correctly addresses the past review feedback. However, the error message could be more diagnostic by including the heaviest tipset epoch, as suggested in the original review comment.📋 Suggested improvement for diagnostics
- .ok_or_else(|| anyhow::anyhow!("calibnet must define at least one expensive fork"))?; + .ok_or_else(|| { + anyhow::anyhow!( + "no expensive fork epoch <= heaviest tipset epoch {} for calibnet snapshot", + heaviest_tipset.epoch() + ) + })?;🤖 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/api_compare_tests.rs` at line 2456, The error created by .ok_or_else currently uses a generic message "calibnet must define at least one expensive fork"; update the closure to include diagnostic context by appending the heaviest tipset epoch (e.g., heaviest_tipset_epoch or heaviest_tipset.epoch()) so the error shows the epoch value when no expensive fork is found; locate the call site where .ok_or_else(|| anyhow::anyhow!(...)) is used and interpolate the existing heaviest tipset variable into the anyhow message to produce a clearer, actionable error.
🤖 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/api_compare_tests.rs`:
- Line 2456: The error created by .ok_or_else currently uses a generic message
"calibnet must define at least one expensive fork"; update the closure to
include diagnostic context by appending the heaviest tipset epoch (e.g.,
heaviest_tipset_epoch or heaviest_tipset.epoch()) so the error shows the epoch
value when no expensive fork is found; locate the call site where .ok_or_else(||
anyhow::anyhow!(...)) is used and interpolate the existing heaviest tipset
variable into the anyhow message to produce a clearer, actionable error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3494d18e-f244-4632-b2a5-c3084f080270
📒 Files selected for processing (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs
| pub const fn is_expensive_migration(height: Height) -> bool { | ||
| matches!( | ||
| height, | ||
| Height::Assembly |
There was a problem hiding this comment.
How about *Fix migrations on calibnet?
There was a problem hiding this comment.
*Fix are not marked as expensive migrations in lotus here
There was a problem hiding this comment.
I think we could implement expensive check by checking height key existence in get_migrations result.
- I think fix heights are not marked as expensive in Lotus because they don't work on mainnet, it's logically correct only on mainnet but wrong on calibnet unless they define separate mappings for calibnet and mainnet.
- We could add unimplemented migrations to the
get_migrationsmappings withunimplemented_migrationplaceholder
@coderabbitai could you check in https://github.com/Filecoin-project/lotus if my understanding is correct
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Lotus upgrades for *Fix migrations and their Expensive flags
# Also check if there's a separate calibnet upgrade schedule
echo "=== Mainnet upgrade schedule (Fix entries) ==="
rg -n "Fix|WatermelonFix|DragonFix|TockFix" chain/consensus/filcns/upgrades.go -A2 -B2 | head -120
echo ""
echo "=== Is there a calibnet-specific upgrade schedule? ==="
fd -t f "upgrades" chain/ --exec echo {}
echo ""
echo "=== expensiveUpgrades population ==="
rg -n "expensiveUpgrades|Expensive" chain/stmgr/stmgr.go -C3 | head -60
Repository: filecoin-project/lotus
Length of output: 5136
🏁 Script executed:
#!/bin/bash
# Check Forest's get_migrations function
echo "=== get_migrations function ==="
ast-grep --pattern 'fn get_migrations($$$) {
$$$
}' | head -100
echo ""
echo "=== rg get_migrations ==="
rg -n "get_migrations" --type rust -C3 | head -100
echo ""
echo "=== unimplemented_migration ==="
rg -n "unimplemented_migration\|noop_migration\|no_migration" --type rust -C3 | head -60
Repository: ChainSafe/forest
Length of output: 1620
🏁 Script executed:
#!/bin/bash
echo "=== Full get_migrations function ==="
rg -n "" src/state_migration/mod.rs | head -80
echo ""
echo "=== Lotus buildconstants for Fix heights on mainnet ==="
Repository: ChainSafe/forest
Length of output: 3427
🏁 Script executed:
#!/bin/bash
echo "=== Fix heights values in buildconstants ==="
rg -n "WatermelonFix|DragonFix|TockFix|CalibrationDragon" build/buildconstants/ -C2 | head -80
echo ""
echo "=== mainnet buildconstants ==="
rg -n "WatermelonFix|DragonFix|TockFix" build/buildconstants/consensus.go -C1 | head -40
echo ""
echo "=== Separate mainnet vs calibnet upgrade schedule? ==="
rg -n "func.*Upgrade.*Schedule\|UpgradeSchedule\|defaultUpgrade" chain/consensus/filcns/upgrades.go | head -30
echo ""
echo "=== How the upgrade schedule is selected per-network ==="
rg -n "UpgradeSchedule\|GetUpgradeSchedule\|upgradeSchedule" chain/stmgr/stmgr.go -C3 | head -60
Repository: filecoin-project/lotus
Length of output: 5829
@hanabi1224 Your understanding is confirmed. Here's what the Lotus codebase shows:
*Why Fix heights are not marked expensive in Lotus:
In params_mainnet.go, all fix heights are set to negative values with explicit comments:
UpgradeWatermelonFixHeight = -1— "This fix upgrade only ran on calibrationnet"UpgradeWatermelonFix2Height = -2— same commentUpgradeCalibrationDragonFixHeight = -3— same commentUpgradeTockFixHeight = -1— "Only applied to calibnet..."
Lotus uses a single upgrade schedule (upgrades.go), but per-network build constants make fix heights negative on mainnet so they're never triggered. Since they don't apply on mainnet, they were never marked Expensive: true. There is no separate mapping per-network — the fix is in the constants.
Forest's get_migrations is already per-network: It already correctly excludes all fix heights from mainnet and includes them in calibnet:
- Mainnet: no
WatermelonFix,WatermelonFix2,DragonFix,TockFix - Calibnet: includes all four
Regarding @hanabi1224's suggestion: Deriving the expensive check from get_migrations keyset makes a lot of sense and would be more correct than the current hard-coded is_expensive_migration predicate. The current approach misses marking fix migrations as expensive on calibnet, where they do have real migrations. Using get_migrations as the source of truth would automatically handle this correctly without any unimplemented_migration placeholder needed.
In practice this would mean: instead of the static is_expensive_migration(Height) function, has_expensive_fork_between (or the HeightInfo::expensive flag population) would check whether a given height has an entry in get_migrations(&network) for the relevant network. Any height with a registered migration is inherently expensive.
🧠 Learnings used
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.
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.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6666
File: src/tool/subcommands/archive_cmd.rs:628-631
Timestamp: 2026-03-02T09:43:34.946Z
Learning: In Rust sources related to Forest snapshot handling, implement the rule: enable message_receipts and events (message_receipts: true, events: true) only for GC snapshots as defined in src/db/gc/snapshot.rs, since these are internal snapshots created during garbage collection. For user-facing export commands such as src/tool/subcommands/archive_cmd.rs, disable receipts and events by default (message_receipts: false, events: false) to keep user-facing snapshots smaller, unless explicitly requested. This guidance targets Rust files; apply consistently across similar snapshot-related modules, using the narrowest applicable scope when extending beyond the two specified files.
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6903
File: src/shim/executor.rs:243-251
Timestamp: 2026-04-14T17:24:13.631Z
Learning: In the ChainSafe/forest (LesnyRumcajs) codebase, do not require rustdoc comments on public methods when the method’s purpose is clearly self-explanatory from its name (e.g., `entries(&self)`, `into_entries(self)`). When reviewing Rust (`.rs`) files, avoid flagging missing documentation for these obviously named public APIs, reserving doc requirements for less clear or non-obvious public methods.
Learnt from: sudo-shashank
Repo: ChainSafe/forest PR: 7061
File: src/networks/mod.rs:542-550
Timestamp: 2026-05-19T05:07:54.755Z
Learning: When reviewing ChainSafe/forest code that implements the Lotus-style “has expensive fork between” logic (e.g., `ChainConfig::has_expensive_fork_between(parent, height)`), accept comparisons that use an inclusive lower bound like `info.epoch >= parent`. This is intentional and matches Lotus’s `HasExpensiveForkBetween`, which iterates `for h := parent; h < height`, defining the interval `[parent, height)`—do not flag or “fix” `>= parent` to `> parent` or otherwise change the lower-bound semantics.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 7077
File: src/utils/cache/size_tracking.rs:174-182
Timestamp: 2026-05-19T10:55:04.684Z
Learning: In ChainSafe/forest code reviews, avoid raising review comments for issues that are likely compile-time failures (e.g., missing imports, unresolved types, or other compiler errors that CI will catch). Instead, focus feedback on logic, correctness, and design/maintainability concerns, since the CI pipeline reliably verifies compilation.
There was a problem hiding this comment.
@coderabbitai I meant we do need to add mappings like Height::Trust -> 'unimplemented_migration' to the mappings in Forest to properly mark Height::Trust as expensive to match Lotus. Note that some RPC methods returns error on expensive fork without running the migration, missing the mapping would lead to incorrect result
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== state_migration mod.rs full content ==="
cat src/state_migration/mod.rs
echo ""
echo "=== get_migrations function ==="
rg -n "get_migrations\|fn.*migration" src/state_migration/ -C2 | head -100
echo ""
echo "=== unimplemented_migration or noop or placeholder ==="
rg -n "unimplemented\|noop\|placeholder\|no.op" src/state_migration/ -i -C2 | head -60
echo ""
echo "=== Which heights are registered in get_migrations for mainnet vs calibnet? ==="
rg -n "Height::" src/state_migration/ -C1 | head -200
Repository: ChainSafe/forest
Length of output: 15570
@hanabi1224 Thank you for the clarification — understood now. The analysis confirms there are two concrete gaps:
Gap 1 — Old pre-NV17 heights (Assembly, Trust, Turbo, Hyperdrive, Chocolate, OhSnap, Skyr): These are in is_expensive_migration but have no entry in get_migrations for any network, because Forest only implements migrations from NV17 (Shark) onward. Adding unimplemented_migration placeholder stubs for these in get_migrations would let the keyset-based approach cover them correctly.
Gap 2 — Calibnet Fix heights (WatermelonFix, WatermelonFix2, DragonFix, TockFix): These already have real migration implementations registered in calibnet's get_migrations, but are not in is_expensive_migration. This is an actual correctness bug in the current PR: has_expensive_fork_between will return false for these epochs on calibnet, so the expensive-fork guard won't fire and the node could end up running a heavy migration during an RPC call instead of returning ErrExpensiveFork.
The proposed fix — deriving is_expensive_migration (or the HeightInfo::expensive flag) from the get_migrations keyset — would resolve both gaps:
- Replace the static predicate with a lookup into
get_migrations(&network) - Add
unimplemented_migrationstubs for old pre-NV17 expensive heights (Assembly, Trust, Turbo, Hyperdrive, Chocolate, OhSnap, Skyr) to each applicable network's migration list
This would make the migration map the single source of truth, with no possibility of the two lists drifting out of sync.
@sudo-shashank — does this refactor make sense to incorporate into this PR, or would you prefer to address it as a follow-up?
✏️ Learnings added
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 7061
File: src/networks/mod.rs:230-230
Timestamp: 2026-05-22T06:39:02.436Z
Learning: In ChainSafe/forest, the `is_expensive_migration` static predicate in `src/networks/mod.rs` has a correctness gap: calibnet Fix heights (WatermelonFix, WatermelonFix2, DragonFix, TockFix) are registered in `src/state_migration/mod.rs` `get_migrations` for calibnet but are NOT marked expensive in `is_expensive_migration`, causing `has_expensive_fork_between` to return false for those epochs on calibnet. The preferred fix is to derive the expensive flag from the `get_migrations` keyset per-network, and add `unimplemented_migration` stubs for pre-NV17 expensive heights (Assembly, Trust, Turbo, Hyperdrive, Chocolate, OhSnap, Skyr) that lack Forest migration implementations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6666
File: src/tool/subcommands/archive_cmd.rs:628-631
Timestamp: 2026-03-02T09:43:34.946Z
Learning: In Rust sources related to Forest snapshot handling, implement the rule: enable message_receipts and events (message_receipts: true, events: true) only for GC snapshots as defined in src/db/gc/snapshot.rs, since these are internal snapshots created during garbage collection. For user-facing export commands such as src/tool/subcommands/archive_cmd.rs, disable receipts and events by default (message_receipts: false, events: false) to keep user-facing snapshots smaller, unless explicitly requested. This guidance targets Rust files; apply consistently across similar snapshot-related modules, using the narrowest applicable scope when extending beyond the two specified files.
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6903
File: src/shim/executor.rs:243-251
Timestamp: 2026-04-14T17:24:13.631Z
Learning: In the ChainSafe/forest (LesnyRumcajs) codebase, do not require rustdoc comments on public methods when the method’s purpose is clearly self-explanatory from its name (e.g., `entries(&self)`, `into_entries(self)`). When reviewing Rust (`.rs`) files, avoid flagging missing documentation for these obviously named public APIs, reserving doc requirements for less clear or non-obvious public methods.
Learnt from: sudo-shashank
Repo: ChainSafe/forest PR: 7061
File: src/networks/mod.rs:542-550
Timestamp: 2026-05-19T05:07:54.755Z
Learning: When reviewing ChainSafe/forest code that implements the Lotus-style “has expensive fork between” logic (e.g., `ChainConfig::has_expensive_fork_between(parent, height)`), accept comparisons that use an inclusive lower bound like `info.epoch >= parent`. This is intentional and matches Lotus’s `HasExpensiveForkBetween`, which iterates `for h := parent; h < height`, defining the interval `[parent, height)`—do not flag or “fix” `>= parent` to `> parent` or otherwise change the lower-bound semantics.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 7077
File: src/utils/cache/size_tracking.rs:174-182
Timestamp: 2026-05-19T10:55:04.684Z
Learning: In ChainSafe/forest code reviews, avoid raising review comments for issues that are likely compile-time failures (e.g., missing imports, unresolved types, or other compiler errors that CI will catch). Instead, focus feedback on logic, correctness, and design/maintainability concerns, since the CI pipeline reliably verifies compilation.
2 participants
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6446
Closes #5965
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Tests