Skip to content

fix: convert look back limit to actual epoch before searching for msg #6354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
akaladarshi wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: convert look back limit to actual epoch before searching for msg #6354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/rpc/methods/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,6 @@ impl RpcMethod<4> for StateSearchMsg {
}
}

/// Looks back up to limit epochs in the chain for a message, and returns its receipt and the tipset where it was executed.
/// See <https://github.com/filecoin-project/lotus/blob/master/documentation/en/api-methods-v0-deprecated.md#StateSearchMsgLimited>
pub enum StateSearchMsgLimited {}

Expand All @@ -1275,7 +1274,9 @@ impl RpcMethod<2> for StateSearchMsgLimited {
const PARAM_NAMES: [&'static str; 2] = ["message_cid", "look_back_limit"];
const API_PATHS: BitFlags<ApiPaths> = make_bitflags!(ApiPaths::V0); // Not supported in V1
const PERMISSION: Permission = Permission::Read;

const DESCRIPTION: Option<&'static str> = Some(
"Looks back up to limit epochs in the chain for a message, and returns its receipt and the tipset where it was executed.",
);
type Params = (Cid, i64);
type Ok = MessageLookup;

Expand Down
2 changes: 1 addition & 1 deletion src/rpc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub use methods::*;
/// Protocol or transport-specific error
pub use jsonrpsee::core::ClientError;

const LOOKBACK_NO_LIMIT: ChainEpoch = -1;
pub const LOOKBACK_NO_LIMIT: ChainEpoch = -1;
Copy link
Member

@LesnyRumcajs LesnyRumcajs Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document this constant while we're at it; the name is not self-explanatory.


/// The macro `callback` will be passed in each type that implements
/// [`RpcMethod`].
Expand Down
35 changes: 30 additions & 5 deletions src/state_manager/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,17 +1047,17 @@ where
&self,
mut current: Tipset,
message: &ChainMessage,
look_back_limit: Option<i64>,
allow_replaced: Option<bool>,
min_epoch: ChainEpoch,
Copy link
Member

@LesnyRumcajs LesnyRumcajs Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

allow_replaced: bool,
) -> Result<Option<(Tipset, Receipt)>, Error> {
let allow_replaced = allow_replaced.unwrap_or(true);
let message_from_address = message.from();
let message_sequence = message.sequence();
let mut current_actor_state = self
.get_required_actor(&message_from_address, *current.parent_state())
.map_err(Error::state)?;
let message_from_id = self.lookup_required_id(&message_from_address, &current)?;
while current.epoch() > look_back_limit.unwrap_or_default() {

while current.epoch() >= min_epoch {
let parent_tipset = self
.chain_index()
.load_required_tipset(current.parents())
Expand Down Expand Up @@ -1092,14 +1092,39 @@ where
Ok(None)
}

/// Searches backwards through the chain for a message receipt.
///
/// # Arguments
/// * `current` - The tipset to start searching from
/// * `message` - The message to search for
/// * `look_back_limit` - Maximum number of epochs to search backwards:
/// - `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)
Comment on lines +1101 to +1104
Copy link
Member

@LesnyRumcajs LesnyRumcajs Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - `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.

/// * `allow_replaced` - Whether to accept replacement messages (same nonce, different CID)
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)
}
Comment on lines 1106 to 1128
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the file and check if it exists
find . -name "mod.rs" -path "*/state_manager/*" 2>/dev/null | head -20

Repository: 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.rs

Repository: 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 -20

Repository: 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.rs

Repository: 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 -20

Repository: 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.rs

Repository: 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 -40

Repository: 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 -80

Repository: 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 -20

Repository: 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.rs

Repository: 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.rs

Repository: 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 -40

Repository: 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.rs

Repository: 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_search uses while current.epoch() >= min_epoch loop, 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_epoch properly 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)

Copy link
Member

@LesnyRumcajs LesnyRumcajs Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akaladarshi It'd be actually nice to implement some tests here.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


/// Returns a message receipt from a given tipset and message CID.
Expand Down
Loading