feat(evmrpc): bound debug_trace struct-logger memory via max_trace_struct_log_bytes (PLT-205)#3677
feat(evmrpc): bound debug_trace struct-logger memory via max_trace_struct_log_bytes (PLT-205)#3677amir-deris wants to merge 2 commits into
Conversation
PR SummaryLow Risk Overview
Tests cover clamp branches and config parsing for the new flag. Reviewed by Cursor Bugbot for commit fae41aa. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3677 +/- ##
==========================================
- Coverage 59.27% 58.28% -0.99%
==========================================
Files 2272 2185 -87
Lines 188193 178197 -9996
==========================================
- Hits 111547 103861 -7686
+ Misses 66593 65104 -1489
+ Partials 10053 9232 -821
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A clean, well-tested, well-documented change that adds an operator cap on default struct-logger trace output. However, Codex flags a likely-correctness issue that defeats the feature's purpose: geth's logger.Config.Limit may count retained struct-log entries, not bytes, which would make the byte-named MaxTraceStructLogBytes config and its 256 MiB default ineffective — this must be verified against the pinned geth version before merge.
Findings: 2 blocking | 4 non-blocking | 1 posted inline
Blockers
- Verify the units of geth's
eth/tracers/logger.Config.Limitagainst the pinned dependency (sei-protocol/go-ethereum v1.15.7-sei-17). Codex reports (and historically geth implemented)Limitas a count of retained StructLog entries (len(l.logs) >= cfg.Limit), not bytes. If that holds at this version, the config nameMaxTraceStructLogBytes, the doc comments ("in bytes"), and the 256 MiB default are misleading and provide essentially no memory protection (256M entries × hundreds of bytes each = tens of GB), so the PR would not accomplish its stated goal. If verified that this geth version bounds by output bytes (resultSize/logs.Size()), the PR is correct and this resolves to a no-op. I could not verify this in-sandbox (module cache,gh api, and web access were all blocked), so this needs author confirmation. If it is entry-count, fix by either renaming the knob to reflect entries and choosing a sane entry-count default, or by enforcing a true byte cap. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Behavior change: the default debug_trace* path was previously unlimited and now truncates at the cap. When the struct logger hits its Limit it silently stops appending entries — the trace result is returned incomplete with no error/flag. Tools consuming full opcode traces may now get partial results without indication. Worth a CHANGELOG/release note and possibly surfacing the truncation in the response.
- Consider an integration-level test that actually exercises a trace exceeding the cap and asserts the output is bounded/truncated end-to-end; current tests only unit-test the
clampDefaultStructLogLimitbranch logic (which is good but doesn't prove the limit takes effect in a real trace). - Cursor's review file (
cursor-review.md) was empty — that pass produced no output. Codex contributed the single (High) finding noted above. - Nit:
int(debugCfg.MaxTraceStructLogBytes)narrows a uint64 to int. On 64-bit nodes (the only supported target) this is fine and the//nolint:gosecis reasonable, but an operator-set value above 2^31 on a 32-bit build would overflow to a negative int and disable the cap. A clamp tomath.MaxInt(or documenting 64-bit-only) would be more defensive.
| MaxConcurrentSimulationCalls: runtime.NumCPU(), | ||
| MaxTraceLookbackBlocks: 10000, | ||
| TraceTimeout: 30 * time.Second, | ||
| MaxTraceStructLogBytes: 256 * 1024 * 1024, // 256 MiB |
There was a problem hiding this comment.
[blocker] The 256 MiB default and the MaxTraceStructLogBytes name assume geth's logger.Config.Limit is measured in bytes. Codex flags (and historical geth implements) Limit as a count of StructLog entries (len(l.logs) >= cfg.Limit), not bytes. Please verify against the pinned sei-protocol/go-ethereum v1.15.7-sei-17 source: if OnOpcode/CaptureState compares against len(logs) rather than an output byte size (resultSize/logs.Size()), then this default permits ~256M entries (tens of GB) and the cap does not bound memory as intended — the config would need to either be renamed to reflect entry-count with an appropriate default, or be changed to enforce an actual byte limit. If this geth version does bound by output bytes, disregard — the PR is correct as written. (I could not verify the dependency source in this review environment.)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small operational guardrail on debug trace memory with conservative defaults and clear test coverage.
Extended reasoning...
Overview
Adds MaxTraceStructLogBytes (default 256 MiB, 0=unlimited) to evmrpc/config and wires it into DebugAPI via a new clampDefaultStructLogLimit helper invoked by TraceTransaction, TraceBlockByNumber, TraceBlockByHash, and TraceCall. The clamp only affects the default geth struct logger; named/custom tracers are skipped, and the cap only ever tightens — never loosens — a caller-supplied Limit.
Security risks
None of note. This is a memory bound that tightens an existing behavior; it does not touch auth, crypto, consensus, or any external surface. Worst-case it returns a smaller trace than a caller requested, which is the documented intent. Defaults preserve upstream geth behavior when set to 0.
Level of scrutiny
Low. This is operator-facing tunable behind a config flag, with the default selected to be conservative (256 MiB). Verified Config.Limit semantics in the vendored geth (eth/tracers/logger/logger.go:49: "maximum size of output, but zero means unlimited") match the PR — so the "honor smaller caller limit, override 0/larger" logic is correct.
Other factors
Test coverage in tracers_structlog_limit_test.go hits the edges that matter (nil config, nil nested Config, larger caller limit, smaller caller limit, custom-tracer no-op, disabled-cap no-op, nil-safe). Config plumbing follows the established pattern in the file (flag constant, default value, ReadConfig branch, app.toml template snippet, bad-type test). No outstanding reviewer comments to address.
There was a problem hiding this comment.
The config plumbing, clamp logic, and tests are clean and well-structured, but the feature does not achieve its stated goal: go-ethereum's logger.Config.Limit bounds the number of retained struct-log entries (opcodes), not bytes, so treating it as a byte count means the advertised 256 MiB memory cap is not actually enforced and the OOM this PR targets can still occur.
Findings: 2 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
int(debugCfg.MaxTraceStructLogBytes)at tracers.go:208 (with//nolint:gosec): a very large operator-configureduint64(> math.MaxInt, or > MaxInt32 on 32-bit builds) wraps to a negativeint, whichclampDefaultStructLogLimitthen treats as<= 0and silently disables the cap — the opposite of the operator's intent. Consider clamping tomath.MaxIntinstead of a raw conversion.- Cursor second-opinion review file (
cursor-review.md) was empty — no findings were contributed from that pass. - Codex's single finding (Limit is an entry count, not bytes) is corroborated here and captured as the primary inline blocker.
| config.Config = &traceLogger.Config{} | ||
| } | ||
| if config.Config.Limit <= 0 || config.Config.Limit > api.maxStructLogBytes { | ||
| config.Config.Limit = api.maxStructLogBytes |
There was a problem hiding this comment.
[blocker] This assigns a byte budget to logger.Config.Limit, but in go-ethereum that field caps the number of retained struct-log entries (one per executed opcode), not bytes — the struct logger compares len(l.logs) against Limit. With the 256 MiB default this becomes Limit = 268,435,456 entries. Since each struct-log entry (PC, op, gas, and optionally stack/memory/storage snapshots) is far larger than one byte, a node would OOM long before accumulating 268M entries, so the intended memory bound is effectively never enforced and the OOM this PR targets can still happen. The cap needs to be expressed in entries with a realistic default (order 10^4–10^6, tuned to the desired memory ceiling), or memory must be bounded some other way — the byte framing does not map onto Limit.
| // Timeout for each trace call | ||
| TraceTimeout time.Duration `mapstructure:"trace_timeout"` | ||
|
|
||
| // MaxTraceStructLogBytes bounds the retained struct-logger output (in bytes) for a |
There was a problem hiding this comment.
[blocker] The Bytes/"in bytes" naming and documentation are misleading given that this value flows into logger.Config.Limit, which is an entry (opcode) count rather than a byte count (see tracers.go:224). Both the config name (max_trace_struct_log_bytes / MaxTraceStructLogBytes) and the 256 MiB default should be reworked to reflect entry-count semantics, and the default chosen to actually bound memory to the intended ceiling. As written the default provides essentially no protection.
2 participants
Summary
debug_trace*calls that use the default struct logger retain one structured log entry per executed opcode. Traces that touch many distinct storage slots grow memory quadratically, and an unbounded trace can OOM the RPC node. Upstream geth exposes a per-callLimiton the struct logger but leaves it unset (unlimited) by default.This PR adds an operator-configurable, node-side cap on the retained default struct-logger output so a single trace cannot grow without bound.
Changes
evm.max_trace_struct_log_bytes(MaxTraceStructLogBytes) bounding the bytes of retained struct-logger output for a single defaultdebug_trace*call. Defaults to 256 MiB;0disables the cap (matches upstream geth's unlimited behavior).DebugAPI.clampDefaultStructLogLimitapplies the cap acrossTraceTransaction,TraceBlockByNumber,TraceBlockByHash, andTraceCall. It:0),Limitdown to the node cap,Limit.ReadConfig), the TOML template, andDefaultConfig.Tests
TestClampDefaultStructLogLimitcovers all clamp/no-op branches (no limit set, nil nested config, larger limit clamped, smaller limit honored, custom tracer, disabled cap, nil config).config_test.goasserts the default value and rejects a bad type for the new flag.PLT-205