fix(erpc:PLA-751): ignore oversized eth_getLogs payloads#49
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecd6f41ae4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cleanup != nil { | ||
| defer cleanup() |
There was a problem hiding this comment.
This branch defers cleanup() immediately after readResponseBodyMax, but JsonRpcResponse.ParseFromBytes can retain slices into bodyBytes for large result payloads instead of copying them (see common/json_rpc.go behavior for large results). When the function returns, the deferred cleanup returns that pooled buffer, so subsequent reads of the parsed response can observe corrupted data under normal concurrent traffic. This affects successful eth_getLogs responses with large payloads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eaa16e8.
Change:
- detach large parsed
eth_getLogsresult bytes from the pooled read buffer before running cleanup inclients/http_json_rpc_client.go - added regression coverage in
clients/http_json_rpc_client_getlogs_limit_test.go
This keeps the bounded read path while avoiding buffer aliasing after ParseFromBytes.
| if math.Trunc(v) != v { | ||
| return 0, fmt.Errorf("%s must be an integer", getLogsMaxSizeField) | ||
| } | ||
| return int64(v), nil |
There was a problem hiding this comment.
maxSize from JSON is decoded as float64, and this path only checks that it is non-negative and integral before casting to int64. For values larger than math.MaxInt64 (for example 1e20), the conversion result is implementation-dependent, so invalid client input can silently become a truncated/negative limit instead of being rejected, which can disable or misapply log payload filtering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eaa16e8.
Added an explicit float64 > math.MaxInt64 guard in extractGetLogsMaxDataBytes, plus a regression test that rejects oversized numeric maxSize values early.
|
@claude review |
|
Claude finished @0x666c6f's task in 1m 31s —— View job PR Review: fix(erpc:PLA-751): ignore oversized eth_getLogs payloads
Overall this is a well-structured PR. The transport sanitization, cache-key separation, and filtering across all three paths (direct, chunked, fallback) are cleanly implemented. The test coverage is good. A few issues worth addressing: 1. Float64 overflow in
|
There was a problem hiding this comment.
Pull request overview
Adds an opt-in per-log data payload size cap for eth_getLogs, allowing oversized logs to be dropped before merge/cache while ensuring upstreams only receive standard JSON-RPC parameters.
Changes:
- Introduces
getLogsMaxDataBytesnetwork config + request override viaeth_getLogs.params[0].maxSize. - Sanitizes forwarded upstream payloads by stripping
maxSizewhile preserving cache-key separation. - Applies payload filtering across direct responses, split sub-requests, and block-receipts fallback; adds regression tests and docs.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/config/src/generated.ts | Exposes getLogsMaxDataBytes in generated TS config types. |
| typescript/config/lib/generated.d.ts | Exposes getLogsMaxDataBytes in published TS typings. |
| docs/pages/config/projects/networks.mdx | Documents per-log payload cap and request override behavior. |
| common/config.go | Adds GetLogsMaxDataBytes to Go network config struct. |
| common/validation.go | Validates GetLogsMaxDataBytes is non-negative. |
| common/defaults.go | Copies GetLogsMaxDataBytes from network defaults when unset. |
| common/defaults_test.go | Tests default-copy behavior for GetLogsMaxDataBytes. |
| common/request.go | Strips maxSize from forwarded upstream request payloads. |
| common/request_test.go | Tests cache-hash separation + upstream payload sanitization. |
| architecture/evm/eth_getLogs_payload_filter.go | Implements maxSize parsing and response filtering by log data size. |
| architecture/evm/eth_getLogs_blockreceipts_fallback.go | Extends log filter to include per-log data size limit; plumbs into receipts fallback. |
| architecture/evm/eth_getLogs.go | Applies payload filtering in upstream post-forward, split sub-requests, and receipts fallback path. |
| architecture/evm/eth_getLogs_test.go | Adds tests covering direct, chunked, fallback, and transport behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if body := r.Body(); len(body) > 0 { | ||
| return body, nil | ||
| method, _ := r.Method() | ||
| if !strings.EqualFold(method, "eth_getLogs") { | ||
| return body, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
ForwardBody now bypasses the raw-body fast path for all eth_getLogs requests, even when params[0] does not include maxSize. That forces a full JSON-RPC unmarshal + re-marshal on every unmodified eth_getLogs, which is likely a noticeable perf regression on high-throughput endpoints. Consider preserving the fast path when maxSize is absent (e.g., detect params[0].maxSize cheaply and only re-marshal when it exists).
There was a problem hiding this comment.
Fixed in b02ade6.
Change:
- preserve the raw-body fast path for unmodified
eth_getLogsrequests unless the raw payload actually contains"maxSize" - added a regression test covering the no-
maxSizefast-path case incommon/request_test.go
| if v > float64(math.MaxInt64) { | ||
| return 0, fmt.Errorf("%s must be less than or equal to %d", getLogsMaxSizeField, int64(math.MaxInt64)) | ||
| } | ||
| return int64(v), nil |
There was a problem hiding this comment.
extractGetLogsMaxDataBytes treats an empty string maxSize: "" as 0 (disabled). Because extractGetLogsPayloadLimitFromRequest treats presence of the key as an override, this can silently disable a non-zero configured GetLogsMaxDataBytes. It would be safer to reject empty strings as invalid input, or treat them as “not set” so the configured default still applies.
| return int64(v), nil | |
| return 0, fmt.Errorf("%s must not be empty", getLogsMaxSizeField) |
There was a problem hiding this comment.
Fixed in b02ade6.
Empty-string maxSize is now rejected as invalid input instead of silently overriding the configured default to 0. Added regression coverage in architecture/evm/eth_getLogs_test.go.
| if reqFilter != nil { | ||
| filter := newGetLogsFilter(reqFilter["address"], reqFilter["topics"], payloadLimit) | ||
| dropped, err := filterGetLogsResponseByDataLimit(jrr, filter) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if dropped > 0 { | ||
| n.Logger().Debug(). | ||
| Int("droppedLogs", dropped). | ||
| Int64("maxSize", payloadLimit). | ||
| Msg("filtered oversized eth_getLogs payloads") | ||
| } | ||
| } |
There was a problem hiding this comment.
When payloadLimit > 0, filtering is currently skipped unless the request params can be asserted to map[string]interface{} (reqFilter != nil). This means the configured GetLogsMaxDataBytes cap won’t apply for requests with missing/invalid filter objects, even though you already computed a non-zero payloadLimit. Consider applying the size-only filtering with newGetLogsFilter(nil, nil, payloadLimit) when reqFilter is nil so the cap is consistently enforced.
| if reqFilter != nil { | |
| filter := newGetLogsFilter(reqFilter["address"], reqFilter["topics"], payloadLimit) | |
| dropped, err := filterGetLogsResponseByDataLimit(jrr, filter) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if dropped > 0 { | |
| n.Logger().Debug(). | |
| Int("droppedLogs", dropped). | |
| Int64("maxSize", payloadLimit). | |
| Msg("filtered oversized eth_getLogs payloads") | |
| } | |
| } | |
| // Always enforce payloadLimit: fall back to size-only filtering when reqFilter is nil. | |
| var addr interface{} | |
| var topics interface{} | |
| if reqFilter != nil { | |
| addr = reqFilter["address"] | |
| topics = reqFilter["topics"] | |
| } | |
| filter := newGetLogsFilter(addr, topics, payloadLimit) | |
| dropped, err := filterGetLogsResponseByDataLimit(jrr, filter) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if dropped > 0 { | |
| n.Logger().Debug(). | |
| Int("droppedLogs", dropped). | |
| Int64("maxSize", payloadLimit). | |
| Msg("filtered oversized eth_getLogs payloads") | |
| } |
There was a problem hiding this comment.
Fixed in b02ade6.
The payload cap is now always enforced when non-zero: if the request filter object is missing/unparseable we fall back to size-only filtering via newGetLogsFilter(nil, nil, payloadLimit). Added regression coverage for that path in architecture/evm/eth_getLogs_test.go.
Summary
eth_getLogs.params[0].maxSizehandling for per-log data payload filteringmaxSizefrom upstream wire requests while preserving cache-key separationChanges
eth_getLogsso upstreams only receive standard JSON-RPC paramsdatapayloadsLinear