fix(eth): scope receipt logs by message, cap event filter, max rpc size#7025
Conversation
WalkthroughThis PR configures the JSON-RPC response body size limit via environment variable and refactors event filtering to support per-message CID scoping. Transaction receipts now scope logs to specific messages, and deferred execution for future chain events is explicitly rejected with a new error variant. ChangesRPC Response Size Configuration
Event Filtering with Message CID Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
69878eb to
63d715e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Line 32: Update the changelog entries that currently reference PR `#7024` to the
correct PR number `#7025`: replace the markdown link targets and displayed numbers
(e.g., the link text "[`#7024`](...)" and any other occurrences) with
"[`#7025`](...)" for both instances mentioned (the entry describing
FOREST_RPC_MAX_RESPONSE_BODY_SIZE around the earlier block and the duplicate at
the later block), ensuring the links point to the PR rather than an issue and
that the displayed PR number matches the URL.
In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 93-109: The cap exemption currently uses tipsets_contributing <= 1
which allows wide-range queries to bypass max_filter_results if all matches
happen to fall in one tipset; instead, change the logic to gate the exemption on
the *requested filter shape* (e.g., whether the RPC requested a single-height
range or a block-hash/Key filter) not on post-hoc match distribution.
Concretely, modify ensure_filter_cap (and the analogous logic at the other
occurrence) to accept a boolean like is_single_block_query /
requested_single_height_or_hash and replace the tipsets_contributing <= 1 check
with that flag (keep max_filter_results == 0 to disable cap entirely); update
call sites to pass true only when the caller explicitly asked for a
single-block/hash/single-height range.
🪄 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: 61c62217-58a9-4bc6-90e6-896beeebb5ca
📒 Files selected for processing (9)
CHANGELOG.mdsrc/cli_shared/cli/config.rssrc/rpc/client.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/errors.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
63d715e to
802ba2c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth/errors.rs (1)
115-125: ⚡ Quick winAssert the fallback RPC code, not only the message.
Line 120 states fallback-to-default behavior, but the test currently validates only the message. Add a code assertion so this contract doesn’t regress silently.
Proposed test hardening
#[test] fn test_events_not_yet_available_converts_to_server_error() { let err = EthErrors::EventsNotYetAvailable; let server_err: ServerError = err.into(); + let default_server_err: ServerError = anyhow::anyhow!("baseline").into(); // No specific RPC error code is assigned; falls back to default. + assert_eq!( + server_err.inner().code(), + default_server_err.inner().code() + ); assert_eq!( server_err.message(), "events for the requested block are not yet available" ); }🤖 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/errors.rs` around lines 115 - 125, The test test_events_not_yet_available_converts_to_server_error currently only checks the error message; add an assertion that the RPC error code falls back to the default as promised. After creating server_err from EthErrors::EventsNotYetAvailable, assert that server_err.code() (or the appropriate accessor on ServerError) equals the ServerError default RPC code (e.g. ServerError::default_rpc_code() or the module's DEFAULT RPC code constant) so the fallback-to-default behavior is enforced.
🤖 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/rpc/methods/eth/errors.rs`:
- Around line 115-125: The test
test_events_not_yet_available_converts_to_server_error currently only checks the
error message; add an assertion that the RPC error code falls back to the
default as promised. After creating server_err from
EthErrors::EventsNotYetAvailable, assert that server_err.code() (or the
appropriate accessor on ServerError) equals the ServerError default RPC code
(e.g. ServerError::default_rpc_code() or the module's DEFAULT RPC code constant)
so the fallback-to-default behavior is enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df620788-9cb3-4998-8e26-6e50bfccebc8
📒 Files selected for processing (9)
CHANGELOG.mdsrc/cli_shared/cli/config.rssrc/rpc/client.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/errors.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/mod.rs
✅ Files skipped from review due to trivial changes (3)
- src/rpc/methods/eth/filter/event.rs
- src/cli_shared/cli/config.rs
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rpc/mod.rs
- src/rpc/client.rs
- src/rpc/methods/eth/filter/mod.rs
- src/rpc/methods/eth.rs
802ba2c to
d0a9a49
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
3124-3148: ⚡ Quick winAdd context to the new fallible log-conversion paths.
These
?sites now sit on the critical path for every returned log. If one fails, the RPC error will be hard to tie back to the offending event/message without extra context.♻️ Suggested improvement
- match eth_tx_hash_from_message_cid(ctx.store(), &event.msg_cid, chain_id)? { + match eth_tx_hash_from_message_cid(ctx.store(), &event.msg_cid, chain_id) + .with_context(|| { + format!("failed to derive transaction hash for event message {}", event.msg_cid) + })? { Some(h) => { tx_hash_by_msg.insert(event.msg_cid, h); h @@ - let h: EthHash = event.tipset_key.cid()?.into(); + let h: EthHash = event + .tipset_key + .cid() + .context("failed to derive block hash from event tipset key")? + .into(); @@ - let a = EthAddress::from_filecoin_address(&event.emitter_addr)?; + let a = EthAddress::from_filecoin_address(&event.emitter_addr) + .with_context(|| { + format!("failed to convert event emitter {} to an Ethereum address", event.emitter_addr) + })?;As per coding guidelines "Use anyhow::Result 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/rpc/methods/eth.rs` around lines 3124 - 3148, The fallible conversions on the critical path (eth_tx_hash_from_message_cid(ctx.store(), &event.msg_cid, chain_id)?, event.tipset_key.cid()?, and EthAddress::from_filecoin_address(&event.emitter_addr)?) must attach context so RPC errors can be traced to the offending event; wrap each `?`-returning call with anyhow context (e.g. `.context(...)`) that includes identifying info such as the event.msg_cid, event.tipset_key, and event.emitter_addr, and update the error messages in the branches using those symbols (eth_tx_hash_from_message_cid, tipset_key.cid, EthAddress::from_filecoin_address) to provide clear, concise context for debugging.
🤖 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/rpc/methods/eth.rs`:
- Around line 3124-3148: The fallible conversions on the critical path
(eth_tx_hash_from_message_cid(ctx.store(), &event.msg_cid, chain_id)?,
event.tipset_key.cid()?, and
EthAddress::from_filecoin_address(&event.emitter_addr)?) must attach context so
RPC errors can be traced to the offending event; wrap each `?`-returning call
with anyhow context (e.g. `.context(...)`) that includes identifying info such
as the event.msg_cid, event.tipset_key, and event.emitter_addr, and update the
error messages in the branches using those symbols
(eth_tx_hash_from_message_cid, tipset_key.cid,
EthAddress::from_filecoin_address) to provide clear, concise context for
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a66722fe-c3a8-4ab9-81e2-5cf46d5929bf
📒 Files selected for processing (9)
CHANGELOG.mdsrc/cli_shared/cli/config.rssrc/rpc/client.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/errors.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/mod.rs
✅ Files skipped from review due to trivial changes (3)
- src/rpc/methods/eth/filter/event.rs
- CHANGELOG.md
- src/cli_shared/cli/config.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/rpc/methods/chain.rs
- src/rpc/client.rs
- src/rpc/methods/eth/errors.rs
- src/rpc/mod.rs
- src/rpc/methods/eth/filter/mod.rs
Summary of changes
Changes introduced in this pull request:
FOREST_RPC_MAX_RESPONSE_BODY_SIZEto deal with large responses. The default stays the same, but we let RPC providers allow for more.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes
Documentation