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 non-optional allow_replaced flag; exposes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 6 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.
| 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).
| /// - `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 ❌ |
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
✏️ Tip: You can customize this high-level summary in your review settings.