fix: limit possible underflows/overflows - #7459
Open
LesnyRumcajs wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThe PR adds checked arithmetic and explicit input validation across beacon, chain, RPC, storage, and state paths. It introduces fallible beacon round calculations, negative-height errors, shared hexadecimal parsing, and boundary regression tests. ChangesArithmetic validation and error handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
LesnyRumcajs
added
RPC
LesnyRumcajs
force-pushed
the
limit-possible-over-underflows
branch
from
August 6, 2026 14:22
da43e98 to
dcae0db
Compare
LesnyRumcajs
marked this pull request as ready for review
August 6, 2026 15:19
LesnyRumcajs
requested review from
hanabi1224 and
sudo-shashank
and removed request for
a team
August 6, 2026 15:19
LesnyRumcajs
force-pushed
the
limit-possible-over-underflows
branch
2 times, most recently
from
August 6, 2026 15:20
5344a31 to
b8dfa6f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/beacon/mock_beacon.rs (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd context to the epoch conversion error.
u64::try_from(fil_epoch)?returns a generic conversion error. Add context that identifies the invalid mock-beacon epoch.As per coding guidelines, add context with
.context()when errors occur.Proposed fix
- Ok(u64::try_from(fil_epoch)?) + u64::try_from(fil_epoch).context("mock beacon epoch must be non-negative")🤖 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/beacon/mock_beacon.rs` around lines 53 - 54, Update the epoch conversion in the mock beacon method returning anyhow::Result<u64> to attach context with .context() before propagating the error, identifying the invalid fil_epoch value while preserving the successful u64 conversion result.Source: Coding guidelines
src/state_manager/chain_rand.rs (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to the beacon-round error.
Line 129 returns the raw error. Add
.context()with the epoch before?. This makes failures actionable across the state-manager and RPC call paths.As per coding guidelines, use
anyhow::Result<T>for most operations and add context with.context()when errors occur.🤖 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/chain_rand.rs` at line 129, Update the error propagation in the beacon round lookup within the surrounding epoch-processing function by adding anyhow context containing the current epoch before the `?` operator. Preserve the existing `max_beacon_round_for_epoch` call and return behavior while making the failure context available to state-manager and RPC callers.Source: Coding guidelines
🤖 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/beacon/drand.rs`:
- Around line 136-140: Validate that the `entry.round()` returned by
`curr_beacon.entry(cur)` matches `cur` before decrementing it. Reject mismatched
rounds, including `cur + 1`, and retain the existing `checked_sub(1)` handling
for a matching round.
In `@src/chain_sync/validation.rs`:
- Around line 307-309: Replace the saturating arithmetic used to compute
expected in the timestamp validation with checked multiplication and addition,
rejecting validation when either operation overflows before comparing the block
timestamp. Extend the related regression test to use timestamp u64::MAX and
assert that the block is rejected.
In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 684-688: Update the min_height validation before the range-walk
branches to require min_height >= 0, removing acceptance of the -1 sentinel. Add
a regression test alongside the existing negative-height test covering
from_block_number(-1) and verify it is rejected.
In `@src/rpc/methods/state.rs`:
- Around line 2254-2259: In the beacon-entry wait flow, update the genesis
timestamp conversion before calling beacon_entry_wait to use i64::try_from(...),
adding the requested context message and propagating conversion failure.
Preserve the existing wait calculation while rejecting u64 timestamps that
cannot be represented as i64.
---
Nitpick comments:
In `@src/beacon/mock_beacon.rs`:
- Around line 53-54: Update the epoch conversion in the mock beacon method
returning anyhow::Result<u64> to attach context with .context() before
propagating the error, identifying the invalid fil_epoch value while preserving
the successful u64 conversion result.
In `@src/state_manager/chain_rand.rs`:
- Line 129: Update the error propagation in the beacon round lookup within the
surrounding epoch-processing function by adding anyhow context containing the
current epoch before the `?` operator. Preserve the existing
`max_beacon_round_for_epoch` call and return behavior while making the failure
context available to state-manager and RPC callers.
🪄 Autofix
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: ac8dd4e2-b749-46df-812c-3b6b1193a065
📒 Files selected for processing (22)
src/beacon/drand.rssrc/beacon/mock_beacon.rssrc/beacon/tests/drand.rssrc/blocks/header.rssrc/chain/mod.rssrc/chain/store/chain_store.rssrc/chain/store/errors.rssrc/chain/store/index.rssrc/chain/tests.rssrc/chain_sync/validation.rssrc/db/car/forest/index/mod.rssrc/fil_cns/weight.rssrc/lotus_json/mod.rssrc/rpc/methods/beacon.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/f3/types.rssrc/rpc/methods/state.rssrc/state_manager/chain_rand.rssrc/state_manager/message_search.rssrc/utils/encoding/hex.rssrc/wallet/subcommands/wallet_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
LesnyRumcajs
marked this pull request as draft
August 6, 2026 15:36
LesnyRumcajs
force-pushed
the
limit-possible-over-underflows
branch
5 times, most recently
from
August 7, 2026 12:32
96e41a2 to
7fc79aa
Compare
LesnyRumcajs
marked this pull request as ready for review
August 7, 2026 15:13
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/rpc/methods/eth/filter/mod.rs (1)
715-717: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to the fallible hexadecimal conversion.
parse_prefixed_intandChainEpoch::try_frompropagate bare errors. Add context so callers can distinguish malformed hexadecimal input from an epoch-range overflow.- let epoch: u64 = crate::utils::encoding::hex::parse_prefixed_int(hex_str)?; - Ok(ChainEpoch::try_from(epoch)?) + let epoch: u64 = crate::utils::encoding::hex::parse_prefixed_int(hex_str) + .context("failed to parse hexadecimal block number")?; + ChainEpoch::try_from(epoch) + .context("hexadecimal block number exceeds the ChainEpoch range")Ensure
anyhow::Contextis in scope.As per coding guidelines, add context with
.context()when errors occur.🤖 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/rpc/methods/eth/filter/mod.rs` around lines 715 - 717, Add anyhow::Context to scope and update the conversion in the epoch-parsing flow to attach distinct context with .context(): identify failures from parse_prefixed_int as malformed hexadecimal input and failures from ChainEpoch::try_from as epoch-range overflow, while preserving the existing error propagation behavior.Source: Coding guidelines
🤖 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/beacon/drand.rs`:
- Line 90: Prevent underflow in the beacon-round lookups by replacing unchecked
subtraction from round values with checked_sub(1). Update both the prior-entry
fetch near the computed round and the max_round - 1 path, returning or handling
the no-prior-round case without requesting u64::MAX.
In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 684-686: Update the filter range validation around the min_height
check to reject from_block values greater than heaviest before calculating the
lookback or applying max_range. Add a regression test covering
from_block_number(heaviest + 1), while preserving valid ranges at or below the
current head.
---
Nitpick comments:
In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 715-717: Add anyhow::Context to scope and update the conversion in
the epoch-parsing flow to attach distinct context with .context(): identify
failures from parse_prefixed_int as malformed hexadecimal input and failures
from ChainEpoch::try_from as epoch-range overflow, while preserving the existing
error propagation behavior.
🪄 Autofix
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: 436f368b-5ec2-48da-91e3-c4005bec38da
📒 Files selected for processing (17)
src/beacon/drand.rssrc/beacon/tests/drand.rssrc/blocks/header.rssrc/chain/store/chain_store.rssrc/chain/store/index.rssrc/chain/tests.rssrc/chain_sync/validation.rssrc/daemon/db_util.rssrc/fil_cns/weight.rssrc/lotus_json/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/f3/types.rssrc/rpc/methods/state.rssrc/state_manager/message_search.rssrc/tool/subcommands/index_cmd.rssrc/utils/encoding/hex.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/rpc/methods/f3/types.rs
- src/chain/tests.rs
- src/lotus_json/mod.rs
- src/rpc/methods/state.rs
- src/fil_cns/weight.rs
- src/chain/store/chain_store.rs
- src/beacon/tests/drand.rs
- src/utils/encoding/hex.rs
LesnyRumcajs
force-pushed
the
limit-possible-over-underflows
branch
from
August 7, 2026 15:57
7fc79aa to
3e8983d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #7448
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes