fix: convert look back limit to actual epoch before searching for msg #6354
Conversation
akaladarshi
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
WalkthroughRefactors state-manager look-back search to use an explicit minimum-epoch boundary and a non-optional Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rpc/mod.rs (1)
44-44: PublicLOOKBACK_NO_LIMITis fine; consider documenting sentinel semantics.Exposing this constant is safe and matches its use in
StateManager. You might add a short doc comment explaining that-1means “no lookback limit” for RPCs that accept alimit, so its meaning is clear in generated docs/OpenRPC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/methods/state.rs(1 hunks)src/rpc/mod.rs(1 hunks)src/state_manager/mod.rs(4 hunks)
⏰ 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 MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Coverage
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (2)
src/rpc/methods/state.rs (1)
1277-1279: Good addition of DESCRIPTION forStateSearchMsgLimited.The description matches the Lotus docs wording and will improve OpenRPC/spec discoverability without changing behavior.
src/state_manager/mod.rs (1)
30-30: Relative lookback conversion is correctly implemented; consider a small helper for testability and verify boundary semantics against Lotus.The new mapping correctly interprets
limitas "epochs back from the current head," withLOOKBACK_NO_LIMIT == -1disabling the bound:let look_back_limit_in_epoch = look_back_limit.and_then(|limit| { if limit > LOOKBACK_NO_LIMIT && limit < current.epoch() { Some(current.epoch().saturating_sub(limit)) } else { None // no limit → search back to genesis } });The loop condition
while current.epoch() > look_back_limit_in_epoch.unwrap_or_default()uses an exclusive bound, searching epochs strictly less thanlimitdistance from the head. The RPC docstring says "looks back up to limit epochs," which aligns with this exclusive semantics; however, confirm this matches Lotus' Go implementation to ensure correctness.Edge cases are handled correctly:
None/limit <= -1/limit >= current.epoch()→look_back_limit_in_epoch == None→ search to genesis.limit == 0→ bound atcurrent.epoch()so the loop doesn't run; only the directtipset_executed_messagecheck applies.For readability and easier unit testing of boundary edge cases, extract this conversion into a small helper:
fn to_epoch_bound(head_epoch: ChainEpoch, limit: Option<i64>) -> Option<ChainEpoch> { ... }Minor layering note: importing
LOOKBACK_NO_LIMITfromcrate::rpccouples core state logic to the RPC layer. If reused broadly, consider a shared lower-level constants module.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
no green checkmark, no review ❌ |
akaladarshi
force-pushed
the
akaladarshi/fix-search-msg-bug
branch
from
e6e204c to
4316d77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/methods/state.rs(1 hunks)src/rpc/mod.rs(1 hunks)src/state_manager/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 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
⏰ 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: tests-release
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests
- GitHub Check: Coverage
🔇 Additional comments (3)
src/rpc/methods/state.rs (1)
1277-1279: Documentation improvement looks good.The DESCRIPTION constant correctly explains that the method "looks back up to limit epochs" which aligns with the fix that interprets limit as a relative look-back distance rather than an absolute epoch.
src/state_manager/mod.rs (2)
1046-1093: LGTM - Clean refactoring to use explicit epoch boundary.The refactored signature accepting
min_epoch: ChainEpochinstead oflook_back_limit: Option<i64>improves clarity by separating concerns: conversion logic is insearch_back_for_message, and this function focuses on the actual search using a concrete epoch boundary.The non-optional
allow_replacedparameter eliminates unnecessary unwrapping, making the code safer.
1095-1105: Excellent documentation!The comprehensive documentation clearly explains all cases for the
look_back_limitparameter, including the special LOOKBACK_NO_LIMIT (-1) value. This significantly improves code maintainability and makes the intended behavior explicit.
| fn search_back_for_message( | ||
| &self, | ||
| current: Tipset, | ||
| message: &ChainMessage, | ||
| look_back_limit: Option<i64>, | ||
| allow_replaced: Option<bool>, | ||
| ) -> Result<Option<(Tipset, Receipt)>, Error> { | ||
| self.check_search(current, message, look_back_limit, allow_replaced) | ||
| let current_epoch = current.epoch(); | ||
| let allow_replaced = allow_replaced.unwrap_or(true); | ||
|
|
||
| // Calculate the minimum epoch (inclusive lower bound) for the search. | ||
| let min_epoch = match look_back_limit { | ||
| // No search: limit = 0 means search 0 epochs | ||
| Some(0) => return Ok(None), | ||
| // Limited search: calculate the inclusive lower bound | ||
| // For ex: limit = 5 at epoch 1000: min_epoch = 996, searches [996, 1000] = 5 epochs | ||
| Some(limit) if limit > 0 && limit < current_epoch => current_epoch - limit + 1, | ||
| // Search all the way to genesis (epoch 0) | ||
| _ => 0, | ||
| }; | ||
|
|
||
| self.check_search(current, message, min_epoch, allow_replaced) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check if it exists
find . -name "mod.rs" -path "*/state_manager/*" 2>/dev/null | head -20Repository: ChainSafe/forest
Length of output: 86
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1106,1128p' ./src/state_manager/mod.rs
# Also check a bit before and after for context
echo "=== CONTEXT BEFORE ==="
sed -n '1090,1105p' ./src/state_manager/mod.rs
echo "=== TARGET LINES ==="
sed -n '1106,1128p' ./src/state_manager/mod.rs
echo "=== CONTEXT AFTER ==="
sed -n '1129,1140p' ./src/state_manager/mod.rsRepository: ChainSafe/forest
Length of output: 3385
🏁 Script executed:
# Search for tests related to search_back_for_message
grep -r "search_back_for_message" . --include="*.rs" -n
# Search for any tests in the state_manager module
echo "=== Looking for test modules in state_manager ==="
grep -n "mod test\|#\[test\|#\[cfg(test)" ./src/state_manager/mod.rs | head -20Repository: ChainSafe/forest
Length of output: 542
🏁 Script executed:
# Check for issue #6346 references
grep -r "6346" . --include="*.rs" --include="*.md" --include="*.txt"
# Search for any recent changes or comments about look_back_limit
grep -n "look_back_limit\|look back\|LOOKBACK_NO_LIMIT" ./src/state_manager/mod.rsRepository: ChainSafe/forest
Length of output: 5509
🏁 Script executed:
# Check for test file
find ./src/state_manager -name "tests.rs" -o -name "test.rs" 2>/dev/null
# List what's in the tests module
ls -la ./src/state_manager/Repository: ChainSafe/forest
Length of output: 642
🏁 Script executed:
# Search for tests that might test search_back_for_message or related functionality
grep -A 10 "search_back_for_message\|look_back_limit" ./src/state_manager/tests.rs 2>/dev/null | head -50
# If tests.rs exists, check its size and structure
wc -l ./src/state_manager/tests.rs 2>/dev/null
grep "#\[test\]" ./src/state_manager/tests.rs | head -20Repository: ChainSafe/forest
Length of output: 172
🏁 Script executed:
# Check the usage of search_back_for_message in the search_msg method
sed -n '1275,1295p' ./src/state_manager/mod.rsRepository: ChainSafe/forest
Length of output: 976
🏁 Script executed:
# Search for tests related to search_back_for_message or look_back_limit in tests.rs
grep -n "search_back_for_message\|search_for_message\|look_back_limit" ./src/state_manager/tests.rs
# Also check the test function names
grep -n "fn test_\|#\[test\]" ./src/state_manager/tests.rs | head -40Repository: ChainSafe/forest
Length of output: 796
🏁 Script executed:
# Check if there are any tests that cover the epoch calculation logic
grep -B 5 -A 20 "epoch\|limit" ./src/state_manager/tests.rs | head -80Repository: ChainSafe/forest
Length of output: 2370
🏁 Script executed:
# Verify the condition logic in the code by looking at the exact match pattern
sed -n '1117,1125p' ./src/state_manager/mod.rs
# Also check if there's documentation about the fix
git log --oneline --all --grep="6346\|search_back\|look_back" 2>/dev/null | head -20Repository: ChainSafe/forest
Length of output: 547
🏁 Script executed:
# Check the full condition more carefully - is there a bug with the condition?
# limit < current_epoch might miss a case
sed -n '1110,1128p' ./src/state_manager/mod.rs
# Let's trace through some examples:
# Example 1: limit=1000, current_epoch=100
# Condition: limit > 0 && limit < current_epoch = 1000 > 0 && 1000 < 100 = FALSE
# Falls through to _ => 0 (search to genesis) - CORRECT
# Example 2: limit=5, current_epoch=1000
# Condition: limit > 0 && limit < current_epoch = 5 > 0 && 5 < 1000 = TRUE
# min_epoch = 1000 - 5 + 1 = 996
# Searches [996, 997, 998, 999, 1000] = 5 epochs - CORRECT
# Example 3: limit=1000, current_epoch=1000
# Condition: limit > 0 && limit < current_epoch = 1000 > 0 && 1000 < 1000 = FALSE
# Falls through to _ => 0 (search to genesis) - CORRECT per docs
echo "Code appears correct. Verifying there are NO tests for search_back_for_message:"
grep -n "search_back_for_message\|search_for_message" ./src/state_manager/tests.rsRepository: ChainSafe/forest
Length of output: 1019
🏁 Script executed:
# Check what LOOKBACK_NO_LIMIT is defined as
grep -rn "LOOKBACK_NO_LIMIT" ./src --include="*.rs"Repository: ChainSafe/forest
Length of output: 390
🏁 Script executed:
# Check the docstring more carefully for the complete parameter documentation
sed -n '1095,1115p' ./src/state_manager/mod.rsRepository: ChainSafe/forest
Length of output: 1075
🏁 Script executed:
# Final verification: check if check_search is where the actual searching happens
grep -n "fn check_search" ./src/state_manager/mod.rs
# Get the signature and a bit of its implementation
sed -n '/fn check_search/,+30p' ./src/state_manager/mod.rs | head -40Repository: ChainSafe/forest
Length of output: 1401
🏁 Script executed:
# Verify the formula one more time with a different edge case
# If limit = 10, current_epoch = 100:
# min_epoch = 100 - 10 + 1 = 91
# Epochs searched: [91, 92, ..., 100] = 10 epochs ✓
# Confirm there are no other versions of this function
grep -n "search_back_for_message" ./src/state_manager/mod.rsRepository: ChainSafe/forest
Length of output: 356
Core fix is correct—add tests before merging.
The implementation correctly converts relative look_back_limit to absolute min_epoch. Code verification confirms:
- Math is accurate: limit=5 at epoch 1000 → min_epoch=996 →
check_searchuseswhile current.epoch() >= min_epochloop, searching [996-1000] = 5 epochs ✓ - Edge cases handled properly: Some(0) returns immediately; Some(N) where N≥current_epoch and None both default to 0 (genesis); Some(-1/LOOKBACK_NO_LIMIT) falls through to 0 ✓
- Condition logic correct:
Some(limit) if limit > 0 && limit < current_epochproperly gates the calculation ✓ - Semantic fix confirmed: Docstring specifies "number of epochs to search backwards" (relative), which code now implements ✓
Critical issue: No tests exist for search_back_for_message or search_for_message. Since this function handles message receipt lookups for RPC methods (StateSearchMsg, StateSearchMsgLimited), add unit tests covering:
- limit=0 (returns None immediately)
- limit=N where 0<N<current_epoch (searches exactly N epochs)
- limit≥current_epoch (searches to genesis)
- None and Some(-1) (both search to genesis)
There was a problem hiding this comment.
@akaladarshi It'd be actually nice to implement some tests here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| pub use jsonrpsee::core::ClientError; | ||
|
|
||
| const LOOKBACK_NO_LIMIT: ChainEpoch = -1; | ||
| pub const LOOKBACK_NO_LIMIT: ChainEpoch = -1; |
There was a problem hiding this comment.
Let's document this constant while we're at it; the name is not self-explanatory.
src/state_manager/mod.rs
Outdated
| message: &ChainMessage, | ||
| look_back_limit: Option<i64>, | ||
| allow_replaced: Option<bool>, | ||
| min_epoch: ChainEpoch, |
There was a problem hiding this comment.
Perhaps lookback_max_epoch. min is theoretically correct, but with lookback to me the meaning is a bit inverted. Not a huge deal, if you don't agree let's leave it as is (but add lookback somewhere).
src/state_manager/mod.rs
Outdated
| /// - `None` or `Some(-1)` (LOOKBACK_NO_LIMIT): Search all the way to genesis | ||
| /// - `Some(0)`: No search performed, returns `None` immediately | ||
| /// - `Some(N)` where `N > 0`: Search back at most N epochs | ||
| /// - `Some(N)` where `N >= current.epoch()`: Same as unlimited (to genesis) |
There was a problem hiding this comment.
| /// - `None` or `Some(-1)` (LOOKBACK_NO_LIMIT): Search all the way to genesis | |
| /// - `Some(0)`: No search performed, returns `None` immediately | |
| /// - `Some(N)` where `N > 0`: Search back at most N epochs | |
| /// - `Some(N)` where `N >= current.epoch()`: Same as unlimited (to genesis) |
Let's not AI generate more documentation than there's actually code.
|
Also, no green checkmark ❌ |
akaladarshi
force-pushed
the
akaladarshi/fix-search-msg-bug
branch
from
4316d77 to
0e19476
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/state_manager/mod.rs:
- Around line 1105-1114: The match on look_back_limit mis-handles limit ==
current_epoch and > current_epoch; update the branches for lookback_max_epoch so
positive limits are handled as: if Some(limit) and limit > 0 && limit <=
current_epoch compute current_epoch - limit + 1 (so limit == current_epoch
yields 1), and add a branch Some(limit) if limit > current_epoch => 0 to
explicitly clamp to genesis; keep the Some(0) => return Ok(None) and the default
=> 0 behavior unchanged.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rpc/methods/state.rssrc/rpc/mod.rssrc/state_manager/mod.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
✅ Files skipped from review due to trivial changes (1)
- src/tool/subcommands/api_cmd/test_snapshots.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/mod.rs
- src/rpc/methods/state.rs
🧰 Additional context used
🧠 Learnings (2)
📚 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/mod.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/mod.rs
⏰ 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). (6)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: Coverage
- GitHub Check: Calibnet check
- GitHub Check: Diff snapshot export checks
- GitHub Check: V2 snapshot export checks
🔇 Additional comments (3)
src/state_manager/mod.rs (3)
1049-1050: LGTM: Simplified signature with explicit boundary parameter.The signature changes clarify the contract:
check_searchnow receives an already-computedlookback_max_epochboundary and a definiteallow_replacedboolean, removing ambiguity and centralizing the epoch calculation logic in the caller.
1094-1117: Good refactoring: Centralized epoch boundary calculation.The refactoring successfully centralizes the look-back limit conversion logic in
search_back_for_message, making the boundary calculation explicit and testable before passing it tocheck_search. The early return forlimit=0is an efficient optimization.
1059-1059: Boundary logic is correct—the>=condition properly implements inclusive lower-bound search.The change from
>to>=combined with the+ 1calculation ensures exactlylimitepochs are searched. For limit=5 at epoch 1000,lookback_max_epoch = 996and the loop searches epochs [1000, 999, 998, 997, 996]—exactly 5 epochs. The>=is necessary to include the boundary epoch; using>would search only 4 epochs.
| // Calculate the max lookback epoch (inclusive lower bound) for the search. | ||
| let lookback_max_epoch = match look_back_limit { | ||
| // No search: limit = 0 means search 0 epochs | ||
| Some(0) => return Ok(None), | ||
| // Limited search: calculate the inclusive lower bound | ||
| // For ex: limit = 5 at epoch 1000: min_epoch = 996, searches [996, 1000] = 5 epochs | ||
| Some(limit) if limit > 0 && limit < current_epoch => current_epoch - limit + 1, | ||
| // Search all the way to genesis (epoch 0) | ||
| _ => 0, | ||
| }; |
There was a problem hiding this comment.
Potential off-by-one error when limit equals current_epoch.
The condition limit < current_epoch on Line 1111 excludes the case where limit == current_epoch, causing it to fall through to the unlimited search case (epoch 0). This results in searching one more epoch than requested.
Example at epoch 1000:
limit=1000: Current code searches[0, 1000]= 1001 epochs (falls through to line 1113)limit=1000: Formula would give1000 - 1000 + 1 = 1→ searches[1, 1000]= 1000 epochs ✓
🔧 Suggested fix
- // Calculate the max lookback epoch (inclusive lower bound) for the search.
+ // Calculate the lookback epoch (inclusive lower bound) for the search.
let lookback_max_epoch = match look_back_limit {
// No search: limit = 0 means search 0 epochs
Some(0) => return Ok(None),
- // Limited search: calculate the inclusive lower bound
- // For ex: limit = 5 at epoch 1000: min_epoch = 996, searches [996, 1000] = 5 epochs
- Some(limit) if limit > 0 && limit < current_epoch => current_epoch - limit + 1,
- // Search all the way to genesis (epoch 0)
+ // Limited search: calculate the inclusive lower bound, clamped to genesis
+ // Example: limit=5 at epoch=1000 → min_epoch=996, searches [996,1000] = 5 epochs
+ // Example: limit=2000 at epoch=1000 → min_epoch=0, searches [0,1000] = 1001 epochs (all available)
+ Some(limit) if limit > 0 => (current_epoch - limit + 1).max(0),
+ // Unlimited search: None or negative limit (e.g., -1) searches to genesis (epoch 0)
_ => 0,
};This change:
- Correctly handles
limit == current_epochusing the formula instead of falling through - Explicitly clamps to 0 when
limit > current_epoch, making the behavior clearer - Maintains the same behavior for unlimited searches (None or negative limits)
🤖 Prompt for AI Agents
In @src/state_manager/mod.rs around lines 1105 - 1114, The match on
look_back_limit mis-handles limit == current_epoch and > current_epoch; update
the branches for lookback_max_epoch so positive limits are handled as: if
Some(limit) and limit > 0 && limit <= current_epoch compute current_epoch -
limit + 1 (so limit == current_epoch yields 1), and add a branch Some(limit) if
limit > current_epoch => 0 to explicitly clamp to genesis; keep the Some(0) =>
return Ok(None) and the default => 0 behavior unchanged.
|
no green checkmark, no review! |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6346
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.