Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces multiple independent improvements across go-ethereum: log query limits with CLI configuration, state hook optimization for unchanged code, conditional contract code storage in EVM, transaction protocol hardening via duplicate detection, GraphQL query depth limiting, configurable RPC WebSocket message size limits, and early validation in proof API. ChangesLog Query Limit Feature
State & VM Optimizations
Transaction Protocol Hardening
GraphQL & RPC Services
API Validation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ethapi/api.go (1)
992-1003:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
GetStorageAtstill decodes the key after state access — same ordering this PR fixes inGetProof.An invalid
hexKeyineth_getStorageAtstill triggers a fullStateAndHeaderByNumberOrHashcall before the decode error is returned. The fix is the same trivial reorder applied toGetProof.🔧 Proposed fix
func (s *PublicBlockChainAPI) GetStorageAt(ctx context.Context, address common.Address, hexKey string, blockNrOrHash rpc.BlockNumberOrHash) (hexutil.Bytes, error) { + key, err := decodeHash(hexKey) + if err != nil { + return nil, fmt.Errorf("unable to decode storage key: %s", err) + } state, _, err := s.b.StateAndHeaderByNumberOrHash(ctx, blockNrOrHash) if state == nil || err != nil { return nil, err } - key, err := decodeHash(hexKey) - if err != nil { - return nil, fmt.Errorf("unable to decode storage key: %s", err) - } res := state.GetState(address, key) return res[:], state.Error() }🤖 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 `@internal/ethapi/api.go` around lines 992 - 1003, GetStorageAt currently calls s.b.StateAndHeaderByNumberOrHash before validating the hexKey, causing an unnecessary state load on invalid input; move the decodeHash(hexKey) call (and its error handling) to occur before calling s.b.StateAndHeaderByNumberOrHash, then proceed to obtain state and header, check state/err, call state.GetState(address, key) and return res and state.Error() as before.
🧹 Nitpick comments (2)
eth/filters/filter_system_test.go (1)
511-600: ⚡ Quick winAdd a regression test for negative
LogQueryLimitconfig values.Current tests cover
>0and0, but not<0. A small case here would prevent reintroducing a misconfig path that can disableeth_getLogsbehavior unexpectedly.🤖 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 `@eth/filters/filter_system_test.go` around lines 511 - 600, Add a regression test that sets Config{LogQueryLimit: -1} and verifies negative limits behave like zero (i.e. disable width limiting): create a new test (e.g., TestLogQueryLimitNegativeDisablesWidthLimit) modeled on TestLogQueryLimitZeroDisablesWidthLimit that constructs a wide FilterCriteria, calls api.NewFilter and api.events.SubscribeLogs(ethereum.FilterQuery(...)) and asserts no error, then checks that Topics length > maxTopics still returns errExceedMaxTopics; reference Config.LogQueryLimit, NewFilter, events.SubscribeLogs, errExceedMaxTopics and maxTopics when locating the logic to test.core/state/statedb_hooked.go (1)
172-181: ⚡ Quick win
newHashandprevHashuse asymmetric empty-code handling — minor fragility.
prevHashfalls back to the constantcodehash.EmptyKeccakCodeHashwhenlen(prev) == 0, butnewHashalways callscrypto.Keccak256Hash(code)— including whencodeisnil. This is correct only ifcodehash.EmptyKeccakCodeHash == crypto.Keccak256Hash(nil), which is currently true, but the asymmetry is fragile: ifEmptyKeccakCodeHashis ever redefined (e.g., to a Poseidon-based sentinel), the comparison silently breaks and the hook fires for a no-opSetCode(addr, nil)call on an account with no code.Apply the same guard symmetrically:
♻️ Proposed symmetric fix
- newHash := crypto.Keccak256Hash(code) - if prevHash != newHash { + newHash := codehash.EmptyKeccakCodeHash + if len(code) != 0 { + newHash = crypto.Keccak256Hash(code) + } + if prevHash != newHash { s.hooks.OnCodeChange(address, prevHash, prev, newHash, code) }🤖 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 `@core/state/statedb_hooked.go` around lines 172 - 181, The OnCodeChange hook computes prevHash using codehash.EmptyKeccakCodeHash when prev is empty but always computes newHash with crypto.Keccak256Hash(code), which is asymmetric and fragile; update the block in the s.hooks.OnCodeChange path so newHash is computed the same way as prevHash (i.e., if len(code) == 0 set newHash = codehash.EmptyKeccakCodeHash else newHash = crypto.Keccak256Hash(code)) and then compare prevHash vs newHash before calling s.hooks.OnCodeChange(address, prevHash, prev, newHash, code); keep references to prev, code, prevHash, newHash and s.hooks.OnCodeChange exactly as in the diff.
🤖 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 `@cmd/utils/flags.go`:
- Around line 1715-1717: The CLI/config parsing currently allows negative values
for RPCGlobalLogQueryLimit which are assigned into cfg.LogQueryLimit via
ctx.IsSet(RPCGlobalLogQueryLimit.Name) and ctx.Int(...); add validation there to
reject negatives: when ctx.IsSet(RPCGlobalLogQueryLimit.Name) read the int into
a temp, if temp < 0 return a validation error (or propagate an existing sanitize
error type) explaining rpc.logquerylimit must be non-negative instead of
assigning it to cfg.LogQueryLimit; ensure references to
RPCGlobalLogQueryLimit.Name, ctx.Int(...) and cfg.LogQueryLimit are used so the
correct branch is fixed.
In `@rpc/client_opt.go`:
- Around line 72-75: The WithWebsocketMessageSizeLimit API accepts negative
values which are silently ignored later; update WithWebsocketMessageSizeLimit to
validate messageSizeLimit at the API boundary by checking if messageSizeLimit <
0 and failing fast (panic with a clear message) so callers are warned
immediately, otherwise continue to set cfg.wsMessageSizeLimit =
&messageSizeLimit; reference the function name WithWebsocketMessageSizeLimit and
the cfg.wsMessageSizeLimit field when making this change.
In `@rpc/server.go`:
- Around line 85-87: SetWebsocketReadLimit currently accepts negative values;
add validation in the Server.SetWebsocketReadLimit method so that if limit is
negative it is guarded against (e.g., clamp to 0 or return early) instead of
assigning to s.wsReadLimit; update the method to check limit < 0 and handle by
setting s.wsReadLimit = 0 (or leaving existing value) to avoid silently
configuring an invalid read limit.
---
Outside diff comments:
In `@internal/ethapi/api.go`:
- Around line 992-1003: GetStorageAt currently calls
s.b.StateAndHeaderByNumberOrHash before validating the hexKey, causing an
unnecessary state load on invalid input; move the decodeHash(hexKey) call (and
its error handling) to occur before calling s.b.StateAndHeaderByNumberOrHash,
then proceed to obtain state and header, check state/err, call
state.GetState(address, key) and return res and state.Error() as before.
---
Nitpick comments:
In `@core/state/statedb_hooked.go`:
- Around line 172-181: The OnCodeChange hook computes prevHash using
codehash.EmptyKeccakCodeHash when prev is empty but always computes newHash with
crypto.Keccak256Hash(code), which is asymmetric and fragile; update the block in
the s.hooks.OnCodeChange path so newHash is computed the same way as prevHash
(i.e., if len(code) == 0 set newHash = codehash.EmptyKeccakCodeHash else newHash
= crypto.Keccak256Hash(code)) and then compare prevHash vs newHash before
calling s.hooks.OnCodeChange(address, prevHash, prev, newHash, code); keep
references to prev, code, prevHash, newHash and s.hooks.OnCodeChange exactly as
in the diff.
In `@eth/filters/filter_system_test.go`:
- Around line 511-600: Add a regression test that sets Config{LogQueryLimit: -1}
and verifies negative limits behave like zero (i.e. disable width limiting):
create a new test (e.g., TestLogQueryLimitNegativeDisablesWidthLimit) modeled on
TestLogQueryLimitZeroDisablesWidthLimit that constructs a wide FilterCriteria,
calls api.NewFilter and api.events.SubscribeLogs(ethereum.FilterQuery(...)) and
asserts no error, then checks that Topics length > maxTopics still returns
errExceedMaxTopics; reference Config.LogQueryLimit, NewFilter,
events.SubscribeLogs, errExceedMaxTopics and maxTopics when locating the logic
to test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 126ab57d-a607-4b28-b972-b818dbdf59a9
📒 Files selected for processing (25)
cmd/geth/main.gocmd/geth/usage.gocmd/utils/flags.gocore/state/statedb_hooked.gocore/state/statedb_hooked_test.gocore/vm/evm.gocore/vm/evm_create_test.goeth/ethconfig/config.goeth/ethconfig/gen_config.goeth/fetcher/tx_fetcher.goeth/fetcher/tx_fetcher_test.goeth/filters/api.goeth/filters/filter_system.goeth/filters/filter_system_test.goeth/protocols/eth/handler_test.goeth/protocols/eth/handlers.gographql/graphql_test.gographql/service.gointernal/ethapi/api.gointernal/ethapi/api_morph_test.gorpc/client_opt.gorpc/server.gorpc/server_test.gorpc/websocket.gorpc/websocket_test.go
| if ctx.IsSet(RPCGlobalLogQueryLimit.Name) { | ||
| cfg.LogQueryLimit = ctx.Int(RPCGlobalLogQueryLimit.Name) | ||
| } |
There was a problem hiding this comment.
Reject negative rpc.logquerylimit values during config sanitization.
At Line 1715, negative values can flow from CLI/config into cfg.LogQueryLimit; later checks treat any negative as “everything exceeds limit”, effectively breaking all log queries.
Proposed fix
if ctx.IsSet(RPCGlobalLogQueryLimit.Name) {
cfg.LogQueryLimit = ctx.Int(RPCGlobalLogQueryLimit.Name)
}
+ if cfg.LogQueryLimit < 0 {
+ Fatalf("--%s must be >= 0", RPCGlobalLogQueryLimit.Name)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ctx.IsSet(RPCGlobalLogQueryLimit.Name) { | |
| cfg.LogQueryLimit = ctx.Int(RPCGlobalLogQueryLimit.Name) | |
| } | |
| if ctx.IsSet(RPCGlobalLogQueryLimit.Name) { | |
| cfg.LogQueryLimit = ctx.Int(RPCGlobalLogQueryLimit.Name) | |
| } | |
| if cfg.LogQueryLimit < 0 { | |
| Fatalf("--%s must be >= 0", RPCGlobalLogQueryLimit.Name) | |
| } |
🤖 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 `@cmd/utils/flags.go` around lines 1715 - 1717, The CLI/config parsing
currently allows negative values for RPCGlobalLogQueryLimit which are assigned
into cfg.LogQueryLimit via ctx.IsSet(RPCGlobalLogQueryLimit.Name) and
ctx.Int(...); add validation there to reject negatives: when
ctx.IsSet(RPCGlobalLogQueryLimit.Name) read the int into a temp, if temp < 0
return a validation error (or propagate an existing sanitize error type)
explaining rpc.logquerylimit must be non-negative instead of assigning it to
cfg.LogQueryLimit; ensure references to RPCGlobalLogQueryLimit.Name,
ctx.Int(...) and cfg.LogQueryLimit are used so the correct branch is fixed.
| func WithWebsocketMessageSizeLimit(messageSizeLimit int64) ClientOption { | ||
| return optionFunc(func(cfg *clientConfig) { | ||
| cfg.wsMessageSizeLimit = &messageSizeLimit | ||
| }) |
There was a problem hiding this comment.
Validate negative websocket size limits at the API boundary.
WithWebsocketMessageSizeLimit accepts negative values, which currently get silently ignored later. This can hide misconfiguration and make behavior non-obvious for callers.
Proposed fix
func WithWebsocketMessageSizeLimit(messageSizeLimit int64) ClientOption {
+ if messageSizeLimit < 0 {
+ panic("negative websocket message size limit")
+ }
return optionFunc(func(cfg *clientConfig) {
cfg.wsMessageSizeLimit = &messageSizeLimit
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithWebsocketMessageSizeLimit(messageSizeLimit int64) ClientOption { | |
| return optionFunc(func(cfg *clientConfig) { | |
| cfg.wsMessageSizeLimit = &messageSizeLimit | |
| }) | |
| func WithWebsocketMessageSizeLimit(messageSizeLimit int64) ClientOption { | |
| if messageSizeLimit < 0 { | |
| panic("negative websocket message size limit") | |
| } | |
| return optionFunc(func(cfg *clientConfig) { | |
| cfg.wsMessageSizeLimit = &messageSizeLimit | |
| }) | |
| } |
🤖 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 `@rpc/client_opt.go` around lines 72 - 75, The WithWebsocketMessageSizeLimit
API accepts negative values which are silently ignored later; update
WithWebsocketMessageSizeLimit to validate messageSizeLimit at the API boundary
by checking if messageSizeLimit < 0 and failing fast (panic with a clear
message) so callers are warned immediately, otherwise continue to set
cfg.wsMessageSizeLimit = &messageSizeLimit; reference the function name
WithWebsocketMessageSizeLimit and the cfg.wsMessageSizeLimit field when making
this change.
| func (s *Server) SetWebsocketReadLimit(limit int64) { | ||
| s.wsReadLimit = limit | ||
| } |
There was a problem hiding this comment.
Guard invalid negative websocket read limits.
SetWebsocketReadLimit currently accepts negative values without validation. Add a guard so invalid input cannot silently configure undefined/unintended behavior on the server path.
Proposed fix
func (s *Server) SetWebsocketReadLimit(limit int64) {
+ if limit < 0 {
+ limit = wsDefaultReadLimit
+ }
s.wsReadLimit = limit
}🤖 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 `@rpc/server.go` around lines 85 - 87, SetWebsocketReadLimit currently accepts
negative values; add validation in the Server.SetWebsocketReadLimit method so
that if limit is negative it is guarded against (e.g., clamp to 0 or return
early) instead of assigning to s.wsReadLimit; update the method to check limit <
0 and handle by setting s.wsReadLimit = 0 (or leaving existing value) to avoid
silently configuring an invalid read limit.
Added a new function to construct introspection queries for testing maximum query depth. Updated the test to validate that queries at the maximum depth succeed and those exceeding it return the expected error.
1 participant
Summary by CodeRabbit
Release Notes
New Features
eth_getLogsfilteringBug Fixes
Performance