feat(evmrpc): bound eth_getLogs peak memory with matched-log count and byte budgets#3759
feat(evmrpc): bound eth_getLogs peak memory with matched-log count and byte budgets#3759amir-deris wants to merge 19 commits into
Conversation
Thread a limit param through the ReceiptStore.FilterLogs interface so log queries abort with ErrTooManyLogs once matches exceed the cap instead of materializing an unbounded result set. Enforce it in both the litt range path (errgroup-cancelled fan-out) and the block-by-block fallback (cooperative early-abort), bounding peak memory to O(maxLog). Rename applyOpenEndedLogLimit to applyOpenEndedBlockWindow to reflect that it windows the block range; the log cap is now enforced separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
PR SummaryMedium Risk Overview Config: new Enforcement: per-log Polling/subscriptions: Breaking: bounded queries that previously returned >10k matches now error; open-ended over-cap results error instead of being truncated. Reviewed by Cursor Bugbot for commit 1275398. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3759 +/- ##
==========================================
- Coverage 60.16% 59.28% -0.89%
==========================================
Files 2327 2236 -91
Lines 194617 184256 -10361
==========================================
- Hits 117100 109243 -7857
+ Misses 66937 65274 -1663
+ Partials 10580 9739 -841
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.
Solid, well-tested change that bounds eth_getLogs peak memory by pushing a matched-log cap into ReceiptStore.FilterLogs; the implementation and call-site updates are correct. Main concern is an undocumented behavior change: the cap is sourced from MaxLogNoBlock (documented as open-ended-only) but now also errors on previously-uncapped bounded queries, and the config doc is now stale.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Config semantics drift:
MaxLogNoBlock(mapstructuremax_log_no_block) is documented in evmrpc/config/config.go:103 and the config template (~line 699) as "max number of logs returned if block range is open-ended." This PR now uses it as a hard error threshold for ALL queries (bounded and open-ended). Update those doc comments to reflect that it now (a) applies to bounded queries and (b) causes an ErrTooManyLogs error rather than truncating returned logs. Consider whether a distinct, clearly-named general cap setting is warranted rather than overloading the open-ended one. - Breaking-change coverage: the PR description's "breaking" section calls out the open-ended truncation→error switch but not that bounded eth_getLogs requests matching more than MaxLogNoBlock (default 10000) logs — previously uncapped and successful — now return ErrTooManyLogs. This should be surfaced in the changelog/release notes so client integrations can adjust.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // both bounded and open-ended queries. Exceeding it is an error (not | ||
| // silent truncation), so peak memory stays bounded to the cap. A value of 0 | ||
| // disables the cap. | ||
| limit := f.filterConfig.maxLog |
There was a problem hiding this comment.
[suggestion] limit is sourced from MaxLogNoBlock, whose config doc reads "max number of logs returned if block range is open-ended." On main, bounded queries were uncapped on both the range path (tryFilterLogsRange passed no limit) and the block-by-block path (limit only applied when applyOpenEndedLogLimit). Applying limit unconditionally here silently repurposes an open-ended-only setting into a hard error threshold for bounded queries: a bounded eth_getLogs matching more than MaxLogNoBlock (default 10000) logs now returns ErrTooManyLogs where it previously succeeded. If this broader cap is intended, update the max_log_no_block doc comment (config.go:103 and the config template) and the changelog; otherwise consider preserving the open-ended-only semantics or introducing a distinct general-cap setting.
| break | ||
| } | ||
| before := len(localLogs) | ||
| f.GetLogsForBlockPooled(block, crit, &localLogs) |
There was a problem hiding this comment.
[nit] Nit: the cap is checked only between blocks, so GetLogsForBlockPooled materializes a full block's matching logs before collected is incremented. Peak matched-log memory is therefore limit + (workers × single-block logs), not strictly limit. This is inherent (at least one block must be materialized) and matches the PR's documented "cap plus at most one in-flight block per worker" — memory stays bounded — so no change needed, just flagging that the bound is cap + overshoot rather than the cap alone.
There was a problem hiding this comment.
Pushes a matched-log cap into the receipt store to bound eth_getLogs peak memory; the concurrency/cancellation logic is sound, but the config documentation claims 0 disables the cap (it's coerced to the default) and the range path counts synthetic logs that later get normalized away, so eth_getLogs can spuriously return ErrTooManyLogs.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output.
- The PR description flags a breaking behavior change (open-ended queries over maxLog now return ErrTooManyLogs instead of silently truncating), and the author notes it 'must be called out in the changelog/docs', but the diff contains no changelog/doc update. Add one before merge.
- Nit: the drain comment in GetLogsByFilters (filter.go:922-923) says producer goroutines could be 'left blocked on a full buffer', but fetchBlocksByCrit fully buffers
res(size = range) and closes it before returning, so producers never block and the channel is already closed when drained — the loop is harmless cleanup but the stated rationale is inaccurate. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // max number of logs returned if block range is open-ended | ||
| // max number of logs a single eth_getLogs query may match before it errors, | ||
| // for both bounded and open-ended block ranges (0 disables the cap) |
There was a problem hiding this comment.
[suggestion] This doc says (0 disables the cap), but NewFilterAPI coerces any maxLog <= 0 to DefaultMaxLogLimit (10000) at filter.go:287-288, so an operator setting max_log_no_block: 0 gets the default cap, not an uncapped query. Either document that 0 falls back to the default, or skip the coercion when the value is explicitly 0 to actually allow disabling. (Flagged by Codex P2.)
| // maxLog caps the number of matching logs a single query may return, for | ||
| // both bounded and open-ended queries. Exceeding it is an error (not | ||
| // silent truncation), so peak memory stays bounded to the cap. A value of 0 | ||
| // disables the cap. |
There was a problem hiding this comment.
[nit] Same inaccuracy as the config doc: "A value of 0 disables the cap" isn't reachable here — limit comes from f.filterConfig.maxLog, which NewFilterAPI has already forced to a positive DefaultMaxLogLimit when the configured value was <= 0. At this call site limit is always > 0.
| sdkCtx := f.ctxProvider(int64(toBlock)) | ||
|
|
||
| logs, err := store.FilterLogs(sdkCtx, fromBlock, toBlock, crit) | ||
| logs, err := store.FilterLogs(sdkCtx, fromBlock, toBlock, crit, limit) |
There was a problem hiding this comment.
[suggestion] The range path applies the cap against the store's pre-normalization count. filterLogsByTags counts every indexed matching log (including synthetic/Cosmos receipts), but normalizeRangeQueryLogs (called just below) rebuilds only from EVM-visible txs — for the eth_* namespace includeSyntheticReceipts=false, so synthetic logs are dropped afterward. Result: eth_getLogs can return ErrTooManyLogs even when the actual RPC result is below maxLog. The block-by-block fallback counts correctly (its collectLogs already excludes synthetic), so the two paths are inconsistent. Consider capping against the normalized count, or documenting that the range-path cap is a conservative over-count. (Codex P1.)
There was a problem hiding this comment.
A well-structured change that pushes a matched-log cap into both eth_getLogs query paths to bound peak memory, with consistent error-on-overflow semantics and good test coverage for both paths. No correctness blockers; the main gaps are documentation of a config field that cannot actually disable the cap and a broader client-visible breaking change that lacks a changelog entry.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Breaking change is under-documented and broader than the PR description states. The description frames the break as only "open-ended queries now error instead of silently truncating." But because
limit := f.filterConfig.maxLogis now passed unconditionally to both the litt range path and the block-by-block fallback, bounded queries (both fromBlock and toBlock set) are now capped too — previously they had no matched-log cap and would return a large result set successfully. A bounded query matching more thanmaxLog(default 10000) logs within the block window will now returnErrTooManyLogswhere it previously succeeded. Add a CHANGELOG.md entry and call out the bounded-query behavior change explicitly (Codex raised the missing-changelog point). - cursor-review.md was empty — the Cursor second-opinion pass produced no output, so only Codex's findings (both incorporated here) were available to merge.
- Minor: in
GetLogsByFiltersthe post-wg.Wait()drain loopfor range blocks {}is effectively unnecessary and its comment ("producer goroutines are not left blocked on a full buffer") is inaccurate —fetchBlocksByCritsizes the channel buffer to the full block range (make(chan ..., end-begin+1)), so producers can never block on a full buffer. Harmless, but the rationale should be corrected or the loop dropped. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // max number of logs returned if block range is open-ended | ||
| // max number of logs a single eth_getLogs query may match before it errors, | ||
| // for both bounded and open-ended block ranges (0 disables the cap) |
There was a problem hiding this comment.
[suggestion] This comment says 0 disables the cap, but that is not achievable via config: NewFilterAPI replaces any maxLog <= 0 with DefaultMaxLogLimit (10000) at evmrpc/filter.go:287-289, and the default for MaxLogNoBlock is already 10000 (config.go:237). So operators cannot disable the cap by setting max_log_no_block = 0 as documented — it silently becomes 10000. Either drop the "(0 disables the cap)" claim here (and mirror it in the limit comment at filter.go:33-34) or actually honor 0 by not defaulting it in NewFilterAPI. (Raised by Codex.)
There was a problem hiding this comment.
This PR pushes a matched-log cap into the receipt store to bound eth_getLogs peak memory, returning ErrTooManyLogs instead of silently truncating. The implementation is sound, well-documented, and well-tested; no blockers. The main non-blocking concern (also raised by Codex) is that the range path enforces the cap on the raw store count before normalization, which can produce a false ErrTooManyLogs for queries whose EVM-visible result is within the limit.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavior change worth emphasizing: on main, bounded block-range queries had no matched-log cap and would return arbitrarily large result sets successfully; they now error once the match count exceeds max_log_no_block (default 10000). The PR description only calls out the open-ended truncation→error change as breaking, but the new cap on bounded queries is an additional behavior change. It is captured in the CHANGELOG, but any client-facing docs should also state that wide bounded queries returning >max_log_no_block logs will now fail.
- Test coverage: the end-to-end RPC test (TestFilterGetLogsMatchedLogCap) uses a blockHash query, which skips the range path and exercises only the block-by-block fallback. The litt range path's cap is covered at the store level (TestLittIdxFilterLogsLimit) but there is no end-to-end test exercising tryFilterLogsRange + normalizeRangeQueryLogs + the cap together through the JSON-RPC handler.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| sdkCtx := f.ctxProvider(int64(toBlock)) | ||
|
|
||
| logs, err := store.FilterLogs(sdkCtx, fromBlock, toBlock, crit) | ||
| logs, err := store.FilterLogs(sdkCtx, fromBlock, toBlock, crit, limit) |
There was a problem hiding this comment.
[suggestion] The cap is enforced by the store on the raw tag-index match count, which includes non-EVM-visible / synthetic logs, before normalizeRangeQueryLogs filters the result down to the EVM-visible set. Since normalized <= raw, a query whose EVM-visible result is within limit can still trip ErrTooManyLogs when the raw match count exceeds it — a false positive that the block-by-block path (which counts already-normalized logs via GetLogsForBlockPooled) does not have. Consider whether the range path should cap on the EVM-visible count for consistency, or document that the cap is intentionally a conservative pre-normalization bound. (Matches Codex's finding.)
|
|
||
| // Re-check the cap against the normalized (EVM-visible) count, which the | ||
| // store's tag-index count does not match. | ||
| if limit > 0 && int64(len(normalized)) > limit { |
There was a problem hiding this comment.
[nit] This post-normalization re-check appears to be dead code: it only fires when len(normalized) > limit, but the store already aborts with ErrTooManyLogs whenever the raw count exceeds limit, and normalized <= raw. So whenever the store returns successfully (raw <= limit), normalized <= limit too and this branch can never trigger. It's harmless as defense-in-depth, but it cannot recover the false-positive case described above — it can only tighten the cap further, never loosen it. A brief comment clarifying it's defensive would help, or it could be removed.
| var limit int64 | ||
| if applyOpenEndedLogLimit { | ||
| limit = f.filterConfig.maxLog | ||
| // Drain the blocks channel so its producer goroutines are not left blocked |
There was a problem hiding this comment.
[nit] The rationale in this comment is slightly inaccurate. fetchBlocksByCrit sizes the channel buffer to the full range (end-begin+1) and blocks on errChan until every producer goroutine has finished and res is closed, so by the time blocks is returned here the producers have already completed — they are never "left blocked on a full buffer." The drain loop is still fine (it just releases buffered ResultBlocks for GC and terminates on the closed channel), but the comment's justification doesn't hold. Consider rewording to reflect that it's freeing buffered blocks rather than unblocking producers.
amir-deris
changed the title
There was a problem hiding this comment.
Adds count + estimated-heap-byte budgets to bound eth_getLogs peak memory, threaded cleanly through the receipt store and both fetch paths with thorough tests and correct nil/concurrency handling. No blocking issues; a few non-blocking notes around peak memory and the documented breaking behavior change.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavior change awareness: bounded eth_getLogs queries (both fromBlock and toBlock set) previously had no matched-log cap and now error with ErrTooManyLogs above max_log_no_block (default 10000), and open-ended queries that were silently truncated now error. This is intentional and documented in the PR/CHANGELOG, but it can break existing indexers/clients — worth calling out to operators. Consider adding a metric/counter for rejected (over-cap) queries for observability.
- Range path peak memory (Codex): tryFilterLogsRange holds the store's byte-only-budgeted candidate slice while normalizeRangeQueryLogs builds a second byte-budgeted rebuilt slice, so peak retained log memory can approach 2x max_log_bytes rather than the single per-query bound the config comment implies. Acceptable as an OOM guard, but the config doc for max_log_bytes could note the ~2x worst case, or the candidate slice could be released earlier.
- Store-side byte-only budget can produce a false-positive ErrTooManyLogBytes: the raw tag-index candidate byte total (which may include synthetic / non-EVM-visible logs that normalization later drops) is what's charged against max_log_bytes at the store, so a query whose EVM-visible result is small could still be rejected if its raw candidates are byte-heavy. This mirrors the count false-positive the PR deliberately avoids; consider documenting that the byte cap is enforced on raw candidates, not the normalized result.
- Cursor produced no review output (cursor-review.md was empty).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| return f.normalizeRangeQueryLogs(ctx, logs, crit) | ||
| budget := f.newLogBudget(limit) | ||
| normalized, err := f.normalizeRangeQueryLogs(ctx, logs, crit, budget) |
There was a problem hiding this comment.
[suggestion] Peak memory on this path can approach ~2x max_log_bytes: logs (the store's byte-only-budgeted candidate slice) stays referenced as candidateLogs for the duration of normalizeRangeQueryLogs, while rebuilt grows under its own byte budget. normalizeRangeQueryLogs only needs candidateLogs to derive the unique block numbers and a capacity hint up front. Consider extracting blockNumbers first and dropping the candidate slice before the rebuild loop, or documenting the ~2x worst case on max_log_bytes. Non-blocking — the caps still prevent unbounded growth.
| return []*ethtypes.Log{}, nil | ||
| } | ||
|
|
||
| return f.normalizeRangeQueryLogs(ctx, logs, crit) | ||
| budget := f.newLogBudget(limit) | ||
| normalized, err := f.normalizeRangeQueryLogs(ctx, logs, crit, budget) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return normalized, nil | ||
| } |
There was a problem hiding this comment.
🟡 The litt-backed range path in tryFilterLogsRange keeps the store's byte-budgeted candidate slice (max_log_bytes) alive as normalizeRangeQueryLogs builds a second, independently-budgeted rebuilt slice, so both can be simultaneously live and peak matched-log heap on this path can approach ~2x max_log_bytes instead of a hard cap. Fixing it just means extracting the block numbers from candidateLogs up front and letting the candidate slice go before the rebuild loop runs.
Extended reasoning...
What the bug is
tryFilterLogsRange fetches candidate logs from the litt store under a byte-only budget capped at max_log_bytes (f.storeCandidateBudget()), storing them in logs (evmrpc/filter.go:1041). That slice is then passed into normalizeRangeQueryLogs as the candidateLogs parameter. Inside normalizeRangeQueryLogs, candidateLogs is only read once, at the very top, to build the blockSet/blockNumbers slice (evmrpc/filter.go:1080-1086) — after that point nothing in the function touches it again.
However, the parameter (and the caller-side logs variable that still references the same backing array) remains in scope for the entire duration of normalizeRangeQueryLogs, which then builds a brand new rebuilt slice via a pooledCollector under a second, completely independent receipt.LogBudget (f.newLogBudget(limit), also capped at max_log_bytes). Because Go does not null out a still-referenced parameter and the caller keeps logs live in its own frame, the garbage collector cannot reclaim candidateLogs while rebuilt is growing.
Why this defeats the byte cap
The two slices hold genuinely distinct backing data, not aliases of the same bytes: candidateLogs’ per-log Data comes from the litt store’s getLogsForTx/convertLog path, while rebuilt’s per-log Data is freshly copied from cached receipts fetched via getOrSetCachedReceipt inside the rebuild loop. So in the worst case, both slices independently approach the full max_log_bytes ceiling at the same time — meaning peak heap for matched-log data on this path can approach roughly 2x the configured budget (e.g. ~128 MiB with the 64 MiB default), even though each individual budget object correctly enforces its own cap in isolation. This directly undercuts the PR’s stated goal of bounding eth_getLogs peak memory to max_log_bytes.
Why nothing today prevents it
There is no code path that drops or truncates candidateLogs before or during the rebuild loop — it is simply held by the candidateLogs parameter binding until the function returns. The existing budgets (storeCandidateBudget() for the store fetch and newLogBudget(limit) for the rebuild) are each correctly enforced independently, but neither is aware of the other’s live allocation, so their sum is what actually bounds peak heap, not either one alone.
Step-by-step proof
- A wide-range
eth_getLogsquery hits the litt-backed range path intryFilterLogsRange. store.FilterLogs(...)returnslogs, a slice of*ethtypes.Logwhose cumulative estimated heap footprint is close tomax_log_bytes(the byte-only budget just barely let it through).tryFilterLogsRangecallsf.normalizeRangeQueryLogs(ctx, logs, crit, budget), passinglogsascandidateLogs.- Inside,
blockNumbersis derived fromcandidateLogsin the first ~15 lines — at this pointcandidateLogs’ actual payload (theData/Topicsbytes) is no longer needed for anything. - Despite that,
candidateLogsremains referenced by the still-live parameter for the rest of the function while the loop overblockNumbersfetches receipts and appends newly-built logs intorebuiltviacollector.Append, which itself charges a secondmax_log_bytes-capped budget. - If
rebuiltalso grows to nearmax_log_bytesbefore the budget trips (a realistic case, since the normalized/EVM-visible result can be close in size to the raw candidate set), bothcandidateLogs(max_log_bytes) andrebuilt(max_log_bytes) are simultaneously reachable, so total live matched-log heap approaches ~2xmax_log_bytes— not the single-cap bound the config name and PR description promise.
Fix
Extract blockNumbers (and hasFilters/filterIndexes, which don’t depend on candidateLogs at all) before doing anything else, and avoid retaining candidateLogs as a live binding for the rest of the call — e.g. take only the small derived slice as input to the rest of the function, or explicitly stop referencing the parameter once blockNumbers is built. This tightens the true worst-case bound back down to a single max_log_bytes, matching the cap the config value is meant to enforce. This does not need to block merge: the growth is still bounded to a constant (2x) multiple of a configurable value, not unbounded, and does not cause a crash, correctness issue, or data loss on its own.
There was a problem hiding this comment.
The windowing change in ddd2038 ("Added window log scan for litt store...") rewrote this exact code path but does not fix the issue this comment raised — it just shrinks the window the doubling can occur in.
candidates is now scoped to a single rangeQueryWindowBlocks-sized window (4 blocks) and passed into normalizeRangeQueryLogs as candidateLogs, same as before. Inside normalizeRangeQueryLogs, candidateLogs is still only read once (to build blockNumbers) and then remains live through the parameter binding for the rest of the function while rebuilt grows — nothing was changed there.
Critically, storeCandidateBudget() is still a fresh, independent max_log_bytes byte-only budget created per window call, while the rebuild's budget is the cumulative, shared budget passed into tryFilterLogsRange across all windows. So in the worst case, a single 4-block window can itself accumulate close to max_log_bytes of candidate data (e.g. a few logs with very large Data payloads), and that candidates slice stays reachable for the duration of that window'''s normalizeRangeQueryLogs call while the already-accumulated result from prior windows plus the in-progress rebuilt slice (bounded together by the shared budget) also approach max_log_bytes. Peak matched-log heap on this path can still reach ~2x max_log_bytes in that scenario — the same bound described in the original comment, just re-scoped to a window instead of the whole query range.
The originally suggested fix still applies: extract blockNumbers from candidateLogs up front in normalizeRangeQueryLogs and drop the reference (e.g. set candidateLogs = nil, or take only []uint64 as the parameter) before the rebuild loop runs, so the per-window candidate slice can't stay live alongside the rebuild's growing allocation.
There was a problem hiding this comment.
Still not fixed as of the latest commits (dddb9aa, 6202f73), which only touch context-cancellation handling in litt_tag_index.go / filter.go and don't change normalizeRangeQueryLogs's candidateLogs handling. candidateLogs is still passed by reference and stays live for the whole call while normalizeRangeQueryLogs builds a second, independently-budgeted rebuilt/result slice, so the transient ~2x max_log_bytes peak described in the original comment (and reiterated by seidroid[bot] on 2026-07-24T12:13:48Z) is unchanged. The suggested fix is still open: extract blockNumbers from candidateLogs up front and drop the reference (e.g. candidateLogs = nil after building blockNumbers, or take only []uint64 as the parameter) before the rebuild loop runs.
There was a problem hiding this comment.
Still not fixed as of the latest commit (15f3e2f, "Fixed polling filter stuck issue + test update for context cancelled"), which only adds getLogsByFiltersWithBackoff to address the separate polling-wedge blocker and doesn't touch normalizeRangeQueryLogs. candidateLogs is still passed by reference and stays live for the whole call while normalizeRangeQueryLogs builds a second, independently-budgeted rebuilt slice, so the transient ~2x max_log_bytes peak described in the original comment (and reiterated multiple times in this thread) is unchanged. The suggested fix is still open: extract blockNumbers from candidateLogs up front and drop the reference (e.g. candidateLogs = nil after building blockNumbers, or take only []uint64 as the parameter) before the rebuild loop runs.
There was a problem hiding this comment.
A well-structured, thoroughly-tested change that bounds eth_getLogs peak memory via a per-log count+byte LogBudget. The enforcement logic is correct across the litt range path, the range-normalization path, and the block-by-block fallback (mutex-guarded reservations, errgroup cancellation on trip, nil-budget preserving prior behavior). No blocking issues; only minor observations and an intentional, documented breaking behavior change.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex reported no material issues.
- Intentional breaking behavior change (well-documented in the PR body): bounded queries — and blockHash queries — that previously returned large/uncapped result sets will now fail with ErrTooManyLogs once they exceed max_log_no_block (default 10000). Confirm downstream consumers / integrations are aware, since this can turn previously-succeeding calls into errors after upgrade.
- On the litt range path, peak heap can reach ~2x max_log_bytes transiently: the store materializes candidate logs bounded by the byte-only budget, and normalizeRangeQueryLogs rebuilds a second bounded slice while the candidates are still referenced. This is acceptable and arguably out of scope, but the effective ceiling is roughly double the configured 64 MiB for that path — worth a note in the config doc if precision matters.
- Minor layering: evmrpc/config/config.go now imports sei-db/ledger_db/receipt solely for the DefaultMaxLogBytes constant, coupling the config package to the storage package. Not a problem, but consider whether the default belongs in a lower-level shared location.
- newLogBudget/storeCandidateBudget re-coerce maxLogBytes<=0 to the default even though NewFilterAPI already coerces filterConfig.maxLogBytes; the redundant guard is harmless but could be dropped.
There was a problem hiding this comment.
A well-structured and thoroughly tested change that bounds eth_getLogs peak memory via a new receipt.LogBudget (matched-log count + estimated heap byte caps), threaded cleanly through the FilterLogs interface with nil meaning uncapped. No blockers; two accuracy caveats on the memory bound are worth noting but non-blocking.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Range-query path can hold roughly 2×
max_log_bytesat once (Codex): the litt store returns byte-budgetedcandidateLogs(up tomax_log_bytes), and that slice stays referenced bylogsintryFilterLogsRangewhilenormalizeRangeQueryLogsindependently buildsrebuiltunder a secondmax_log_bytesbudget. Peak is still bounded but ~2× the advertised ceiling. Additionally, the store materializes full logDatafor candidates, yetnormalizeRangeQueryLogsonly consumes theirBlockNumber— the byte budget is spent on payloads that are immediately discarded. Consider dropping thecandidateLogsreference (or returning only block numbers from the store) before rebuilding. - Cursor second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - Minor:
logs := make([]*ethtypes.Log, 0, budget.UsedCount())infilterLogsByTagspreallocates 0 capacity when the budget is nil (uncapped) — same as the priorvar logsbehavior, so no regression, but the prealloc optimization is lost for uncapped internal callers. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // slot pointer, struct fields, and topics slice header. This bounds peak | ||
| // heap for OOM prevention, not JSON-RPC wire size. | ||
| logHeapPointerOverhead = int64(8) | ||
| logHeapStructOverhead = int64(64) |
There was a problem hiding this comment.
[suggestion] logHeapStructOverhead = 64 undercounts the real ethtypes.Log struct. On 64-bit, the struct is ~160 bytes (Address 20 + Topics header 24 + Data header 24 + BlockNumber 8 + TxHash 32 + TxIndex 8 + BlockHash 32 + Index 8 + Removed 1, with alignment). Since EstimateLogHeapBytes also adds the address (20) and topics-header (24) separately, the combined fixed estimate (~116B) is still well below the true per-log heap (~200B incl. the pointer and topics backing array). For a count-heavy query of small logs this lets actual memory run ~30-40% over the configured max_log_bytes before the budget trips. Since this is an OOM-prevention bound, consider raising the constant to reflect the actual struct size (~160B, or drop the separately-counted address/topics-header and use ~200B flat).
There was a problem hiding this comment.
This PR adds count and byte budgets (LogBudget) to bound peak memory for eth_getLogs across the litt range path and the block-by-block fallback. The implementation is careful and well-tested; I found no blocking correctness issues, only a minor peak-memory observation on the range path and a note that the Cursor pass produced no output.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. Codex's single P2 finding (range-path peak memory ~2x max_log_bytes) is captured as an inline suggestion below.
- Intentional breaking change: bounded queries (both fromBlock and toBlock set) are now capped at max_log_no_block (default 10000) and will error with ErrTooManyLogs where they previously succeeded; open-ended queries now error instead of silently truncating. This is documented in the PR body and CHANGELOG.md — flagging only so reviewers/operators are aware clients may need to narrow ranges.
- Nice defensive touches worth acknowledging: nil-budget is handled uniformly (Reserve/Tripped/Err/UsedCount all nil-safe, pooledCollector guards c.budget != nil), the litt fan-out uses a byte-only budget so the authoritative count cap is applied deterministically in normalizeRangeQueryLogs rather than nondeterministically across parallel workers, and the drain loop is safe because fetchBlocksByCrit fully buffers and closes its channel before returning.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| return f.normalizeRangeQueryLogs(ctx, logs, crit) | ||
| budget := f.newLogBudget(limit) | ||
| normalized, err := f.normalizeRangeQueryLogs(ctx, logs, crit, budget) |
There was a problem hiding this comment.
[suggestion] Minor (matches Codex P2): on the range path, store.FilterLogs above materializes the candidate slice bounded by max_log_bytes (via storeCandidateBudget()), and normalizeRangeQueryLogs then rebuilds a second slice also bounded by max_log_bytes while logs is still referenced by this frame. Transient peak retained log memory can therefore approach ~2x max_log_bytes rather than the single stated bound. normalizeRangeQueryLogs only needs logs to collect the unique block numbers up front, so setting logs = nil after that extraction (or documenting the 2x transient) would tighten the guarantee. Not blocking.
| // slot pointer, struct fields, and topics slice header. This bounds peak | ||
| // heap for OOM prevention, not JSON-RPC wire size. | ||
| logHeapPointerOverhead = int64(8) | ||
| logHeapStructOverhead = int64(64) | ||
| logTopicsSliceHeader = int64(24) | ||
| ) | ||
|
|
||
| // LogBudget tracks matched-log count and estimated heap bytes for eth_getLogs | ||
| // queries. Call Reserve before appending each log; Reserve returns an error | ||
| // without charging the budget when either ceiling would be exceeded (> limit, | ||
| // not >=). Concurrent Reserve calls may overshoot by ~O(workers) before Tripped | ||
| // is observed. | ||
| type LogBudget struct { | ||
| mu sync.Mutex | ||
| usedBytes int64 | ||
| usedCount int64 | ||
| maxBytes int64 | ||
| maxLog int64 | ||
| tripped atomic.Bool | ||
| tripErr error | ||
| } | ||
|
|
||
| func NewLogBudget(maxLog, maxBytes int64) *LogBudget { | ||
| return &LogBudget{maxLog: maxLog, maxBytes: maxBytes} | ||
| } | ||
|
|
||
| // NewLogBudgetBytesOnly caps estimated heap bytes without a matched-log count | ||
| // ceiling. Used by the litt store on the range-query path for early OOM abort | ||
| // before eth normalization rebuilds canonical logs. |
There was a problem hiding this comment.
🟡 EstimateLogHeapBytes (sei-db/ledger_db/receipt/log_budget.go:17-45) undercounts the true retained heap of an ethtypes.Log by roughly 30-40% for small/zero-data logs, because it omits a slice header for Data and its struct-overhead constant (64B) is well below the real ~160B struct size. This lets a count-heavy small-log result overshoot the configured max_log_bytes OOM-prevention budget by a similar margin before Reserve trips. Note this duplicates the seidroid[bot] suggestion already posted on this PR (log_budget.go line 20) making the same observation.
Extended reasoning...
EstimateLogHeapBytes computes the per-log estimate as AddressLength(20) + len(Data) + logTopicsSliceHeader(24) + HashLength*len(Topics) + logHeapPointerOverhead(8) + logHeapStructOverhead(64). For a log with no Data and no Topics, that fixed portion is only 20+24+8+64 = 116 bytes.
The real ethtypes.Log struct is roughly 160 bytes on a 64-bit build (Address 20B, Topics slice header 24B, Data slice header 24B, BlockNumber 8B, TxHash 32B, TxIndex 8B, BlockHash 32B, Index 8B, Removed 1B padded, plus alignment), and the result slice itself retains an 8-byte *ethtypes.Log pointer per element. That puts the true fixed per-log retained heap at roughly 160-176 bytes — noticeably more than the 116 bytes the estimator charges. Two things drive the gap: the logHeapStructOverhead constant (64) is far below the real struct size (~160), and the estimator never adds a slice-header charge for Data (only Topics gets one via logTopicsSliceHeader), even though Data is itself a slice with its own 24-byte header cost independent of its length.
This estimate feeds directly into LogBudget.Reserve, which is the only thing standing between an eth_getLogs query and unbounded memory growth — it is charged per matched log on both the litt range path (storeCandidateBudget, byte-only) and the block-by-block fallback (pooledCollector.Append). Because the estimate is a per-log constant-time computation with no visibility into actual Go allocator behavior, no other code path double-checks or corrects it; whatever Reserve decides is charged is the only signal the budget acts on.
For a query that matches many small or zero-Data logs — exactly the workload where the count cap (maxLog) is least protective, since it only bounds count, not size — the under-charge compounds across every matched log. At the default max_log_bytes (64 MiB), actual retained heap for such a result set could run on the order of 30-40% past the configured ceiling before Reserve returns ErrTooManyLogBytes, i.e. an effective ceiling closer to ~85-90 MiB rather than 64 MiB. Since the whole point of this budget is to bound peak memory and prevent OOM, undercounting is specifically the unsafe direction here: a safety bound should over-estimate cost, not under-estimate it.
Concrete walkthrough: take a query matching N small logs, each with Address set, no Topics, and no Data. The estimator charges each one 116 bytes, so Reserve will admit up to floor(max_log_bytes / 116) logs before tripping — e.g. with the 64 MiB default, floor(67108864/116) ≈ 578525 logs. But the true retained heap per log (struct ~160B + 8B slice pointer ≈ 168B) means that many logs actually occupy 578525 * 168 ≈ 92.2 MiB, about 1.44x the intended 64 MiB cap. The budget still bounds memory to some multiple of the configured value (it does not regress to unbounded growth), but that multiple is materially larger than 1x for exactly the small-log-heavy workload the count cap does the least to protect against.
Fix: raise logHeapStructOverhead to reflect the real ~160-byte struct size (or, equivalently, add a Data slice-header charge and bump the struct constant closer to the true value) so the estimate is comfortably at or above the true retained footprint rather than ~30% under it.
This is the same observation independently raised by the seidroid[bot] reviewer earlier on this PR (inline suggestion on log_budget.go line 20, with matching numbers: "~116B" fixed estimate vs. "~200B" true footprint, "~30-40%" overage), so the author already has this feedback surfaced once; treating this as a duplicate is reasonable, but the underlying observation itself is technically sound. It does not change existing behavior or introduce a regression — the function is explicitly documented as an approximation, and memory usage remains bounded to a small, predictable multiple of the configured cap rather than growing unbounded — so it does not rise to a merge-blocking defect. A one-line constant adjustment closes most of the gap.
There was a problem hiding this comment.
A well-structured, thoroughly tested change that pushes count + byte budgets into the receipt store so eth_getLogs aborts instead of accumulating unbounded results. No correctness blockers found; a couple of non-blocking notes on peak-memory accounting and the documented behavior change.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Peak-memory bound is ~2x
max_log_bytes, not the single 64 MiB the config/PR text implies (Codex P2, confirmed): on the range path the accumulated normalizedresultis bounded by the query-wide authoritative budget (~max_log_bytes) while each window also mints a fresh byte-onlystoreCandidateBudget()that can materialize up to max_log_bytes of transient candidates. Still bounded (windows are only 4 blocks, so candidate sets are usually small), but consider clarifying themax_log_bytesdoc comment to state it bounds the result, with transient per-window candidate memory on top. - Intentional breaking behavior change: bounded
eth_getLogsqueries matching more thanmax_log_no_block(default 10000) now error withErrTooManyLogswhere they previously succeeded, and open-ended queries now error instead of silently truncating. This is documented in the CHANGELOG and PR body and the PR is labelednon-app-hash-breaking; flagging only for operator/client awareness. - Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output. Codex's review contributed the peak-memory note above. - Minor:
filterLogsByTagspreallocates the merged slice withmake([]*ethtypes.Log, 0, budget.UsedCount()); with a byte-only store-candidate budget (maxLog=0)UsedCount()can be large but is bounded by the byte cap, so this is fine — just noting the capacity hint is driven by the byte budget rather than a count cap on that path. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // #nosec G115 -- windowTo is a block height which fits in int64 | ||
| sdkCtx := f.ctxProvider(int64(windowTo)).WithContext(ctx) | ||
|
|
||
| candidates, err := store.FilterLogs(sdkCtx, windowFrom, windowTo, crit, f.storeCandidateBudget()) |
There was a problem hiding this comment.
[nit] Each window calls f.storeCandidateBudget(), minting a fresh byte-only budget (up to max_log_bytes) per window. Combined with the query-wide budget that keeps prior windows' normalized results resident (also up to max_log_bytes), peak matched-log memory is bounded to roughly 2x max_log_bytes rather than the single 64 MiB the config comment suggests. Not a correctness issue (peak stays bounded, and windows are only 4 blocks so candidate sets are typically small), but worth reflecting in the max_log_bytes doc comment.
| if filterConfig.maxLog <= 0 { | ||
| filterConfig.maxLog = DefaultMaxLogLimit | ||
| } | ||
| if filterConfig.maxLogBytes <= 0 { | ||
| filterConfig.maxLogBytes = receipt.DefaultMaxLogBytes | ||
| } | ||
|
|
||
| shutdownCtx, shutdownCancel := context.WithCancel(context.Background()) | ||
| logFetcher := &LogFetcher{ |
There was a problem hiding this comment.
🟡 The new maxLog<=0 → DefaultMaxLogLimit coercion this PR adds and documents (in NewFilterAPI, evmrpc/filter.go:306-308) is never applied on the WS eth_subscribe(logs) path: NewSubscriptionAPI (evmrpc/subscribe.go) assigns the raw, uncoerced FilterConfig straight onto the LogFetcher. So an operator who sets evm.max_log_no_block=0 — following the doc comment this PR adds — gets the coerced 10000-log cap on HTTP eth_getLogs/eth_newFilter, but an uncapped matched-log count on WS log subscriptions (only the byte budget remains as a backstop there). Fix by applying the same <=0 coercion in NewSubscriptionAPI, or coercing MaxLogNoBlock/MaxLogBytes once in ReadConfig so every consumer sees an already-normalized value.
Extended reasoning...
This PR introduces a documented contract for Config.MaxLogNoBlock: "a non-positive value falls back to DefaultMaxLogLimit." That contract is implemented in exactly one place — NewFilterAPI (evmrpc/filter.go:306-308), which runs if filterConfig.maxLog <= 0 { filterConfig.maxLog = DefaultMaxLogLimit } (and the analogous coercion for maxLogBytes) before constructing its LogFetcher. NewFilterAPI backs the HTTP eth_getLogs / eth_newFilter+eth_getFilterChanges endpoints wired in server.go.
The WS eth_subscribe(logs) path is wired differently: NewEVMWebSocketServer builds a LogFetcher inline (with no filterConfig set at all) and passes a separate &FilterConfig{maxLog: config.MaxLogNoBlock, maxLogBytes: config.MaxLogBytes, maxBlock: config.MaxBlocksForLog} straight into NewSubscriptionAPI. NewSubscriptionAPI (evmrpc/subscribe.go:49-50) does logFetcher.filterConfig = filterConfig with no coercion whatsoever — it never calls or shares logic with NewFilterAPI. So on this path, maxLog/maxLogBytes are whatever the operator configured, unmodified, including a non-positive value.
The Logs subscription loop (subscribe.go, calling a.logFetcher.GetLogsByFilters) reads limit := f.filterConfig.maxLog (evmrpc/filter.go:855) and feeds it to f.newLogBudget(limit) → receipt.NewLogBudget(limit, maxLogBytes). LogBudget.Reserve's count check is b.maxLog > 0 && newCount > b.maxLog (log_budget.go) — so a maxLog of 0 or less disables the count dimension entirely, silently, rather than falling back to DefaultMaxLogLimit as the new doc comment promises. The byte budget is separately coerced per-use inside newLogBudget/storeCandidateBudget (if maxLogBytes <= 0 { maxLogBytes = receipt.DefaultMaxLogBytes }), so it does still apply on this path — maxLog is the one dimension that only ever gets coerced inside NewFilterAPI, and nowhere else.
Step-by-step proof: (1) Operator sets evm.max_log_no_block = 0 in app.toml, following exactly the semantics this PR's own new doc comment describes ("a non-positive value falls back to DefaultMaxLogLimit"). (2) A client calls eth_getLogs over HTTP: NewFilterAPI runs at startup, sees filterConfig.maxLog <= 0, sets it to DefaultMaxLogLimit (10000) once, and every query through that FilterAPI/LogFetcher is capped at 10000 matched logs as documented. (3) A client instead calls eth_subscribe("logs", ...) over WS: the LogFetcher backing NewSubscriptionAPI was constructed with maxLog: config.MaxLogNoBlock (== 0) directly, no coercion ever ran on it. (4) The subscription's poll loop calls GetLogsByFilters → tryFilterLogsRange/block-by-block fallback with limit = 0, so LogBudget.maxLog = 0, and Reserve's b.maxLog > 0 guard is false for every call — the matched-log count is never capped on this path, contradicting the very doc comment this PR adds. Only the byte budget (default 64 MiB, coerced per-use) remains as a backstop.
Nothing in the existing code prevents this: NewSubscriptionAPI genuinely never touches NewFilterAPI or shares its coercion logic, so the PR's stated goal — enforcing max_log_no_block "for both bounded and open-ended block ranges" with a documented fallback for non-positive values — is not actually reached on the WS subscription entry point.
This is a narrow issue in practice: it requires an operator to explicitly set max_log_no_block <= 0 (the shipped default, 10000, makes the coercion a no-op everywhere), the byte budget still bounds peak memory on this path so there is no OOM risk, and the runtime behavior for this specific edge case on the WS path is unchanged from before this PR (the prior applyOpenEndedLogLimit gate was also maxLog > 0-gated). What the PR does change is introducing the documented <=0 → DefaultMaxLogLimit contract while only enforcing it on one of the two entry points, plus extending the count-cap goal to a path that still doesn't receive it. A clean fix is to apply the same <=0 coercion inside NewSubscriptionAPI (mirroring NewFilterAPI), or, more robustly, to coerce MaxLogNoBlock/MaxLogBytes once inside config.ReadConfig so every consumer of Config already sees normalized values and no call site can forget the check.
There was a problem hiding this comment.
The WS eth_subscribe(logs) path is untouched by this PR: evmrpc/subscribe.go is not among the changed files, and NewSubscriptionAPI (evmrpc/subscribe.go:49-50) still does logFetcher.filterConfig = filterConfig with no <=0 coercion. The FilterConfig built for it in NewEVMWebSocketServer (evmrpc/server.go) only gained the new maxLogBytes field; maxLog is still passed through raw from config.MaxLogNoBlock with no fallback to DefaultMaxLogLimit. So an operator setting evm.max_log_no_block=0 still gets an uncapped matched-log count on WS log subscriptions, contradicting the doc comment this PR adds in evmrpc/config/config.go. The fix (coerce in NewSubscriptionAPI, or centralize the coercion in config.ReadConfig) has not been applied.
There was a problem hiding this comment.
Still unaddressed as of the latest commits: evmrpc/subscribe.go's NewSubscriptionAPI (line 50) still does 'logFetcher.filterConfig = filterConfig' with no <=0 coercion, and evmrpc/server.go still builds that FilterConfig directly from config.MaxLogNoBlock/config.MaxLogBytes with no fallback to DefaultMaxLogLimit/DefaultMaxLogBytes. This is the third confirmation of the same gap (see the 2026-07-24T12:08:29Z follow-up). Leaving open.
There was a problem hiding this comment.
Still unaddressed: evmrpc/subscribe.go NewSubscriptionAPI still does logFetcher.filterConfig = filterConfig with no <=0 coercion, and evmrpc/server.go NewEVMWebSocketServer still constructs &FilterConfig{maxLog: config.MaxLogNoBlock, maxLogBytes: config.MaxLogBytes, maxBlock: config.MaxBlocksForLog} directly with no fallback to DefaultMaxLogLimit/DefaultMaxLogBytes. This is the fourth confirmation of the same gap across the review thread. Leaving open.
| // global request rate limit, only applies to queries > RPSLimitThreshold | ||
| GlobalRPSLimit = 30 | ||
| RPSLimitThreshold = 100 // block range queries below this threshold bypass rate limiting | ||
|
|
||
| // rangeQueryWindowBlocks bounds how many blocks tryFilterLogsRange asks the | ||
| // litt store to materialize per call, so raw candidates are discarded | ||
| // between windows instead of accumulating over the whole query range. | ||
| rangeQueryWindowBlocks = 4 | ||
| ) | ||
|
|
||
| // BlockCacheEntry for sotring block, bloom, and receipts cache |
There was a problem hiding this comment.
🟡 tryFilterLogsRange now scans the litt receipt store in strictly sequential rangeQueryWindowBlocks=4-sized windows instead of one call over the whole range, so each window's litt fan-out is capped at 4 concurrent block scans even though the configured LogFilterParallelism defaults to 16. For a full 2000-block range this is roughly 500 sequential store round trips instead of ~125, a real latency regression (~4x for wide ranges) on the primary littidx eth_getLogs path that isn't mentioned in the PR description. It's an intentional memory/latency tradeoff (the byte budget is what actually bounds memory), so consider widening the window to match logFilterParallelism to recover most of the lost concurrency.
Extended reasoning...
What changed
Before this PR, tryFilterLogsRange issued a single store.FilterLogs(sdkCtx, fromBlock, toBlock, crit) call over the entire requested range. Inside the litt store, filterLogsByTags fans that range out across an errgroup capped at s.logFilterParallelism (default DefaultReceiptLogFilterParallelism = 16, see sei-db/config/receipt_config.go:28), so a wide query could scan up to 16 blocks concurrently.
After this PR, tryFilterLogsRange walks [fromBlock, toBlock] in a plain sequential for loop, stepping by rangeQueryWindowBlocks = 4 (evmrpc/filter.go:55). Each iteration calls store.FilterLogs for just that 4-block window and fully awaits it — plus normalizeRangeQueryLogs — before advancing to the next window. Since each window contains at most 4 blocks, filterLogsByTags's nBlocks is <= 4, so the same errgroup.SetLimit(logFilterParallelism) can now only ever schedule 4 concurrent block scans, no matter how high logFilterParallelism is configured. Windows never overlap, so there is no pipelining to make up the difference.
Impact
For the default maxBlock = 2000 range, this turns what used to be ~125 rounds of 16-way-parallel block scans into ~500 sequential rounds of 4-way-parallel scans — roughly a 4x wall-clock regression on the litt-backed eth_getLogs range-query path, which is the primary/fast path this PR itself is built around. The regression applies even to sparse or no-match queries, since each window still requires its own store round trip to check the tag index. None of this is called out in the PR description or CHANGELOG, which otherwise document the new memory-bounding behavior in detail.
Why the window size doesn't need to be this small
The windowing itself is a deliberate and reasonable choice — it discards raw litt candidates between windows so the per-window byte budget (storeCandidateBudget(), driven by max_log_bytes) bounds memory regardless of how wide the overall query range is. But the size chosen for that window (4) is far below the configured fan-out (16 by default), so it throttles concurrency without any corresponding memory benefit: the byte budget is enforced per-window and is exactly as effective whether the window is 4 blocks or 16 blocks wide. A window sized to (or a small multiple of) logFilterParallelism would preserve full concurrency inside each window while still bounding per-window memory the same way.
Step-by-step illustration
- Client issues
eth_getLogswithfromBlock=1000, toBlock=3000(2000 blocks, themaxBlockdefault) against a littidx-backed node with default config (logFilterParallelism = 16). - Pre-PR: one
store.FilterLogs(1000, 3000, crit)call fans out across the errgroup, scanning up to 16 blocks concurrently — roughly 125 "rounds" of parallel work to cover 2000 blocks. - Post-PR:
tryFilterLogsRangeloopswindowFrom := 1000; windowFrom <= 3000; windowFrom += 4, issuing 500 separatestore.FilterLogscalls of width 4, each blocking on its ownnormalizeRangeQueryLogscall before the next window starts. Each call's internal errgroup can schedule at most 4 goroutines (nBlocks <= 4), regardless oflogFilterParallelism=16. - Net effect: ~500 serial round trips instead of ~125 parallel rounds — an approximately 4x latency increase for this query, with no corresponding gain (the byte budget bounds memory the same way at window size 4 or 16).
Why this is a nit rather than a blocker
This is a real, quantifiable performance regression on a primary RPC path, but it is not a correctness issue — results are unchanged, nothing crashes, and it reflects an intentional (if perhaps under-tuned) memory-vs-latency tradeoff that the PR author can reasonably choose to accept or tune in a follow-up. The default 120s filter timeout also makes outright query failures from this alone unlikely. Recommend raising rangeQueryWindowBlocks to match (or be a multiple of) logFilterParallelism, or pipelining window fetches, to recover most of the lost concurrency without giving up the memory-bounding benefit.
There was a problem hiding this comment.
Adds count + estimated-heap-byte caps for eth_getLogs (LogBudget) pushed into the receipt store, windowed litt range scanning, and context threading; the design is sound, correctness looks solid, and it is extensively tested. No blockers — a few non-blocking notes, chiefly a ~2× peak-memory weakening from per-window fresh byte budgets and an inaccurate PR-description "rename".
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Breaking behavior change (documented in CHANGELOG/description, flagging for operator awareness): bounded eth_getLogs queries that previously returned arbitrarily large result sets now error with ErrTooManyLogs once matches exceed max_log_no_block (default 10000); open-ended queries that were silently truncated now also error. Ensure downstream clients/indexers handle the new error rather than treating it as an empty result.
- PR-description inaccuracy: the description states
applyOpenEndedLogLimitwas renamed toapplyOpenEndedBlockWindow, but no such function exists — the open-ended block-windowing branch infetchBlocksByCritwas removed entirely. This is safe (that branch was effectively dead: bothFilterAPI.GetLogsandGetLogsByFiltersrejectblockRange > maxBlockbeforefetchBlocksByCritruns, so itsblockRange > maxBlockclamp was unreachable), but the description should be corrected to say 'removed' not 'renamed'. - Latency tradeoff (acknowledged in the description): rangeQueryWindowBlocks=4 caps effective litt fan-out at 4 concurrent block scans regardless of receipt-store.log-filter-parallelism (default 16), ~4× more scan waves on wide store-bound queries. Consider widening the window toward log-filter-parallelism, which recovers concurrency without weakening the byte cap.
- Cursor's second-opinion pass produced no output (empty cursor-review.md); only the Codex pass contributed a finding (the 2× memory note below).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // #nosec G115 -- windowTo is a block height which fits in int64 | ||
| sdkCtx := f.ctxProvider(int64(windowTo)).WithContext(ctx) | ||
|
|
||
| candidates, err := store.FilterLogs(sdkCtx, windowFrom, windowTo, crit, f.storeCandidateBudget()) |
There was a problem hiding this comment.
[suggestion] Peak memory can reach ~2× max_log_bytes rather than the ~1× the PR contract implies. Each window is given a fresh full-size byte budget via storeCandidateBudget() (maxBytes = full max_log_bytes), while the query-wide budget separately accumulates the bytes of already-normalized results held in result. Late in a wide query, result can be near max_log_bytes at the same time a single window materializes candidates near max_log_bytes, so transient peak ≈ 2× the configured cap. Still bounded (not the pre-PR unbounded case), so not a blocker, but consider charging window candidates against the remaining query-wide byte budget (e.g. derive the store budget from max_log_bytes - budget.usedBytes) so the stated hard bound actually holds. (Raised by Codex.)
There was a problem hiding this comment.
Well-structured, thoroughly tested change that bounds eth_getLogs peak memory via a matched-log count cap and a new estimated-heap-byte cap threaded through the receipt store and both query paths. No correctness blockers; the main substantive note is that the per-log heap estimate undercounts the real footprint, so the effective byte ceiling runs ~40% above the configured value.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
- Intentional latency tradeoff: the 4-block
rangeQueryWindowBlockscaps effective litt fan-out at 4 concurrent block scans regardless of the configuredreceipt-store.log-filter-parallelism(default 16), roughly 4x more scan waves on wide store-bound queries. Documented by the author; consider widening the window towardlog-filter-parallelismto recover concurrency without weakening the byte cap. - Breaking behavior change (documented in PR/CHANGELOG): bounded
eth_getLogsqueries matching more thanmax_log_no_blocknow error with ErrTooManyLogs where they previously succeeded, and open-ended queries that were silently truncated now error. Ensure operators/clients are aware. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // slot pointer, struct fields, and topics slice header. This bounds peak | ||
| // heap for OOM prevention, not JSON-RPC wire size. | ||
| logHeapPointerOverhead = int64(8) | ||
| logHeapStructOverhead = int64(64) |
There was a problem hiding this comment.
[suggestion] The fixed per-log overhead here undercounts the real heap footprint. EstimateLogHeapBytes charges 116 fixed bytes (Address 20 + topics header 24 + pointer 8 + struct 64), but an ethtypes.Log value is ~168 bytes on 64-bit Go plus the 8-byte result-slice slot pointer (~176 B fixed) — the 64-byte logHeapStructOverhead models the struct as ~40% smaller than it is. For collections of small logs the estimate is ~1.4x low, so actual peak heap can exceed the configured max_log_bytes by tens of MiB at the 64 MiB default, partially undermining the intended ceiling. Memory stays proportionally bounded so this isn't blocking, but consider raising logHeapStructOverhead (or deriving it from unsafe.Sizeof(ethtypes.Log{})) so the byte cap reflects true retained memory. (Matches Codex's finding.)
There was a problem hiding this comment.
The count/byte budget plumbing is well-structured and thoroughly tested, but the final commit's parent-context check in filterLogsByTags contradicts two tests added earlier in the same PR (they assert a canceled context returns no error), and the new hard ErrTooManyLogs on open-ended queries permanently wedges polling filters because eth_getFilterChanges returns the error without advancing lastToHeight. The 4-block range window is also a large, self-acknowledged latency regression on wide eth_getLogs queries.
Findings: 3 blocking | 8 non-blocking | 6 posted inline
Blockers
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only the Codex findings plus my own.- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Breaking change worth double-checking with consumers: bounded
eth_getLogs(bothfromBlockandtoBlockset) previously had no matched-log cap and now hard-fails atmax_log_no_block(default 10000). Indexers/dapps routinely pull >10k logs from a bounded range; consider a larger default for the bounded case, or shipping the bounded-range cap disabled-by-default first. It is documented in the CHANGELOG/PR body, so flagging rather than blocking. - No test covers the
eth_getFilterChanges/ WS-subscription overflow path (filter.go:528,subscribe.go:277) — only the one-shoteth_getLogshandler is exercised. Given the cursor-advance interaction, that path deserves a regression test. TestFilterGetLogsMatchedLogCapdepends on running in the suite's serial phase and on a cache warm-up side effect (as its own comment says). This is a fragile ordering dependency in a shared-fixture suite; a fixture-localLogFetcher(likefilter_range_cap_test.gouses) would be more robust.- Disagreement with Codex's High finding (
filter.go:844, removal of the open-ended block windowing infetchBlocksByCrit): that windowing was already unreachable. Both callers —FilterAPI.GetLogs(filter.go:620) andGetLogsByFilters(filter.go:845) — unconditionally rejectblockRange > maxBlockbeforefetchBlocksByCritruns, and both derive bounds from the same watermarks, so deleting the branch is dead-code removal, not a behavior change. The real filter/subscription regression comes from the log-count cap, not this deletion (see inline comment onfilter.go:853). - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := eg.Wait(); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
[blocker] This parent-context check (added in the last commit, "Added parent context check after error group context check") contradicts two tests added earlier in this same PR:
litt_ctx_internal_test.go:181-183(TestFilterLogsByTagsPreCanceledContextReturnsEmptyFast) assertsrequire.NoError+require.Emptyfor a pre-canceled context.litt_ctx_internal_test.go:211-213(TestFilterLogsThreadsSDKContext) asserts the same forFilterLogswith a canceled wrappedcontext.Context.
With blocks 1..5 (resp. block 6) written, SetReceipts advances latestVersion, so neither test hits the fromBlock > toBlock early return. The for i loop breaks immediately on egCtx.Err() != nil, no worker returns an error, so eg.Wait() returns nil — and then this new check returns context.Canceled. Both assertions should fail.
Pick one behavior and make code + tests + doc comments agree: either keep this check and update those tests/comments to expect context.Canceled, or drop it and rely on blockLogs' per-block check. Returning the cancellation error is the better semantic (silently returning an empty, non-error result for a canceled scan is indistinguishable from "no matches"), so I'd fix the tests.
| // silent truncation), so peak memory stays bounded to the cap. NewFilterAPI | ||
| // coerces a non-positive value to DefaultMaxLogLimit, so limit is always > 0 | ||
| // here; downstream helpers still treat limit <= 0 as uncapped. | ||
| limit := f.filterConfig.maxLog |
There was a problem hiding this comment.
[blocker] Applying maxLog as a hard error here (instead of the previous silent truncation for open-ended queries) permanently wedges polling log filters. getFilterChanges (filter.go:528-531) returns the error and never reaches the updatedFilter.lastToHeight = lastToHeight + 1 update at line 539, so the next poll re-queries the same window — which has only grown — and fails again, forever. Before this PR an open-ended filter over a busy contract truncated to maxLog and the cursor advanced, so it caught up. Recreating the filter doesn't help: the same criteria hit the same wall.
Same concern for the WS backfill at subscribe.go:277.
Suggested fix: keep the hard error for the one-shot eth_getLogs entrypoint, but for the filter/subscription paths either advance lastToHeight to the last fully-processed height before returning the overflow error, or keep truncating there so the cursor can move forward.
| // rangeQueryWindowBlocks bounds how many blocks tryFilterLogsRange asks the | ||
| // litt store to materialize per call, so raw candidates are discarded | ||
| // between windows instead of accumulating over the whole query range. | ||
| rangeQueryWindowBlocks = 4 |
There was a problem hiding this comment.
[suggestion] A 4-block window caps effective store concurrency at 4 even though receipt-store.log-filter-parallelism defaults to 16: a 2000-block range becomes ~500 sequential FilterLogs calls / scan waves instead of ~125, i.e. roughly 4× more waves on store-bound wide queries (the PR description says as much). Since the byte budget is what actually bounds memory, the window buys very little here. Recommend defaulting the window to logFilterParallelism (or making it configurable) rather than 4.
| // #nosec G115 -- windowTo is a block height which fits in int64 | ||
| sdkCtx := f.ctxProvider(int64(windowTo)).WithContext(ctx) | ||
|
|
||
| candidates, err := store.FilterLogs(sdkCtx, windowFrom, windowTo, crit, f.storeCandidateBudget()) |
There was a problem hiding this comment.
[suggestion] f.storeCandidateBudget() is constructed fresh on every window, so the candidate byte cap is per-window, not per-query, and it is independent of the query-wide budget charged in normalizeRangeQueryLogs. Peak heap is therefore up to ~2× max_log_bytes (all retained normalized logs plus one full window of raw candidates), while the config comment in evmrpc/config/config.go describes max_log_bytes as bounding the matched-log bytes a query may materialize. (Codex raised this too.) Either charge candidates and normalized logs against one shared budget, or document the 2× factor and halve the effective store-side allowance.
Related asymmetry worth a comment: the count cap was deliberately deferred to normalization to avoid synthetic-log false positives, but the byte cap is still applied to raw candidates, so a query whose EVM-visible result is tiny can still fail with ErrTooManyLogBytes because of non-visible candidate payloads.
| // LogBudget tracks matched-log count and estimated heap bytes for eth_getLogs | ||
| // queries. Call Reserve before appending each log; Reserve returns an error | ||
| // without charging the budget when either ceiling would be exceeded (> limit, | ||
| // not >=). Concurrent Reserve calls may overshoot by ~O(workers) before Tripped |
There was a problem hiding this comment.
[nit] "Concurrent Reserve calls may overshoot by ~O(workers) before Tripped is observed" is misleading for usedCount/usedBytes: Reserve holds b.mu for the whole check-and-charge and never charges when a ceiling would be exceeded, so the charged totals can't overshoot at all (as TestLogBudgetConcurrentSlack asserts). What can lag is other workers noticing Tripped() and stopping their own scans. Reword to say that.
| } | ||
|
|
||
| var logs []*ethtypes.Log | ||
| logs := make([]*ethtypes.Log, 0, budget.UsedCount()) |
There was a problem hiding this comment.
[nit] budget.UsedCount() as the prealloc hint silently assumes the budget is exclusive to this one FilterLogs call. That holds today (the eth range path builds a fresh candidate budget per window), but it breaks quietly if a caller ever shares one budget across calls (over-allocation), and with the common nil budget the prealloc degrades to 0. Summing len(results[i]) is exact and independent of budget lifetime.
There was a problem hiding this comment.
The LogBudget design, litt-store plumbing, and test coverage are solid, but the new getLogsByFiltersWithBackoff narrowing collapses to mid == lastToHeight when the newest block overflows the cap, which wedges polling filters and WS log subscriptions and re-delivers the previous block's logs on every poll. The intentionally-tiny 4-block litt window also ships a self-documented ~4× latency regression on the hot eth_getLogs range path.
Findings: 2 blocking | 13 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Codex flagged (High) that removing the open-ended
maxBlockwindowing fromfetchBlocksByCritmakes open-ended polling queries fail with "block range too large" once the cursor falls behind. I checked the base commit (9872e54) and this is not a regression:GetLogsByFiltersalready returned that error at base (filter.go:808) using the sameComputeBlockBounds(latest, earliest, lastToHeight, crit)inputs, so the windowing branch infetchBlocksByCrit(base lines 1219-1223) was already unreachable from the only caller. Removing it is a correct dead-code cleanup — no action needed. - The PR description claims a "Rename —
applyOpenEndedLogLimit→applyOpenEndedBlockWindow". No such symbol exists anywhere in the tree; the flag was removed entirely (see above). Please fix the description so reviewers/backporters aren't looking for a function that isn't there. - The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so nothing from it is merged here. evmrpc/config/config_test.goadds themaxLogBytesopt and itsGetwiring but never asserts thatReadConfigactually populatescfg.MaxLogBytes, so the newflagMaxLogBytesbranch inReadConfigis unexercised. Worth onerequire.Equalalongside the existingMaxLogNoBlockassertion.evmrpc/confignow importssei-db/ledger_db/receiptsolely to readDefaultMaxLogBytes. That's a new RPC-config → ledger-DB dependency for one constant; consider defining the default inevmrpc/config(or a shared constants package) and lettingreceiptstay leaf-level.TestFilterGetLogsMatchedLogCapdepends on a shared-fixture cache warm-up query and on running during the serial (non-parallel) phase offilter_test.go. That makes it order-dependent and easy to break when other tests in the file change; a self-contained fixture (like the good ones infilter_range_cap_test.go) would be more durable.docs/migration/seiv2_config_migration.mddocuments sampleapp.tomlblocks containingmax_log_no_blockbut was not updated with the newmax_log_byteskey. Low priority, but it's the doc operators copy from.- Operational note for the release: this makes a bounded
eth_getLogsmatching >10000 logs a hard error rather than a full result, which is a client-visible break for indexers that currently rely on wide bounded queries. It is in the CHANGELOG, but consider calling it out in release notes and double-checking that 10000 is the right public-RPC default given it was previously only applied to open-ended queries. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // Even a single block overflows the cap; nothing left to narrow. | ||
| return nil, 0, err | ||
| } | ||
| mid := begin + (curEnd-begin)/2 |
There was a problem hiding this comment.
[blocker] The narrowing degenerates and wedges polling filters instead of advancing them.
ComputeBlockBounds uses if lastToHeight > begin { begin = lastToHeight } — i.e. begin == lastToHeight, not lastToHeight + 1. So when only the newest block overflows the cap, curEnd == begin + 1 and mid = begin + 1/2 == begin == lastToHeight.
Trace with filter.lastToHeight = L, latest = L+1, block L+1 matching >maxLog logs, crit = {FromBlock: 1, ToBlock: nil}:
GetLogsByFilters→begin=L, end=L+1→ErrTooManyLogs.- Backoff:
begin=L,curEnd=L+1,curEnd > beginso the guard on line 1005 does not fire;mid = L;ToBlock = L. - Retry:
ComputeBlockBounds→begin=L, end=L→ succeeds, returning block L's logs again withend = L. GetFilterChangessetsfilter.lastToHeight = L— unchanged. Next poll repeats step 1 forever.
So the client gets block L's logs duplicated on every eth_getFilterChanges / WS logs notification, never advances past L+1, and never sees the ErrTooManyLogs error — the opposite of the "cursor still advances instead of wedging forever" intent in the doc comment. At base this case silently truncated via mergeSortedLogs and the cursor advanced normally, so this is a new regression.
Suggest bailing out (or advancing past the offending block with the error surfaced) when narrowing can't make progress, e.g.:
mid := begin + (curEnd-begin)/2
if mid <= lastToHeight {
// Narrowing cannot exclude any block the cursor hasn't already delivered.
return nil, 0, err
}TestGetLogsByFiltersWithBackoffPropagatesSingleBlockOverflow doesn't catch this because it pins crit.FromBlock = rangeCapTestHeight, which pushes begin up to curEnd and trips the line-1005 guard. Please add a case with FromBlock absent (or below lastToHeight) and exactly one over-cap block at latest.
| // rangeQueryWindowBlocks bounds how many blocks tryFilterLogsRange asks the | ||
| // litt store to materialize per call, so raw candidates are discarded | ||
| // between windows instead of accumulating over the whole query range. | ||
| rangeQueryWindowBlocks = 4 |
There was a problem hiding this comment.
[blocker] A 4-block window is very small relative to receipt-store.log-filter-parallelism (default 16), and the PR description already quantifies the cost: a 2000-block range becomes ~500 sequential store calls / scan waves instead of ~125, with per-query concurrency effectively clamped to 4. That's a ~4× latency regression on store-bound wide eth_getLogs — the exact hot path this file optimizes elsewhere.
The description also states the fix: "Widening the window toward log-filter-parallelism would recover most of the lost concurrency without weakening the byte cap." Since the byte budget (not the window) is what actually bounds memory, please apply that here — ideally deriving the window from the store's configured parallelism rather than hardcoding a constant — instead of landing the regression and following up.
| // #nosec G115 -- windowTo is a block height which fits in int64 | ||
| sdkCtx := f.ctxProvider(int64(windowTo)).WithContext(ctx) | ||
|
|
||
| candidates, err := store.FilterLogs(sdkCtx, windowFrom, windowTo, crit, f.storeCandidateBudget()) |
There was a problem hiding this comment.
[suggestion] Each window gets a fresh full-size storeCandidateBudget() while result from prior windows is still retained, so actual peak heap is up to ~2× max_log_bytes (accumulated normalized logs + the current window's raw candidates). Also flagged by Codex.
Given the whole point of max_log_bytes is a hard memory ceiling, a single query-scoped byte budget shared between the store candidates and normalization would make the configured value mean what it says. If the two-budget split is deliberate, please at least document the 2× factor on the MaxLogBytes config comment so operators size it correctly.
| if budget.Tripped() { | ||
| break | ||
| } | ||
| if err := f.GetLogsForBlockPooled(block, crit, &localLogs, budget); err != nil { |
There was a problem hiding this comment.
[suggestion] The error from GetLogsForBlockPooled is discarded — correctness here depends entirely on the invariant that budget.Reserve is the only error source in collectLogs, since the overflow is later recovered from budget.Tripped() at line 963. If collectLogs ever grows another failure mode, this silently truncates the result and returns HTTP 200 with partial logs. Capture the error into a shared variable (under resultsMutex) and surface it after wg.Wait() rather than relying on the budget as the sole channel.
| return nil | ||
| } | ||
|
|
||
| b.mu.Lock() |
There was a problem hiding this comment.
[suggestion] Reserve takes a sync.Mutex once per matched log, and the same *LogBudget is shared across all logFilterParallelism (default 16) filterLogsByTags workers. On a high-match query that turns the parallel fan-out into a single serialization point in its innermost loop. Since the struct only needs monotonic counters plus a first-error latch, atomic.AddInt64 for usedCount/usedBytes with a sync.Once-style trip would keep the fast path lock-free; the doc comment already allows ~O(workers) overshoot, so exactness isn't required.
| // slot pointer, struct fields, and topics slice header. This bounds peak | ||
| // heap for OOM prevention, not JSON-RPC wire size. | ||
| logHeapPointerOverhead = int64(8) | ||
| logHeapStructOverhead = int64(64) |
There was a problem hiding this comment.
[nit] logHeapStructOverhead = 64 materially undercounts ethtypes.Log, which is closer to ~168 bytes on amd64 (Address 20 + Topics slice header 24 + Data slice header 24 + BlockNumber 8 + TxHash 32 + BlockHash 32 + TxIndex 8 + Index 8 + Removed 1, plus padding) — and AddressLength is added separately on line 55, so it's partly double-counted while the rest is missing. For zero-Data logs the estimate is ~2× low, meaning max_log_bytes under-bounds real heap in exactly the many-small-logs case. Harmless against a 64 MiB default where the count cap dominates, but worth either correcting to unsafe.Sizeof(ethtypes.Log{}) or noting the direction of the error in the comment.
| if matchLog(lg, crit) { | ||
| logs = append(logs, lg) | ||
| for _, rawLog := range receipt.Logs { | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
[nit] ctx.Err() on a cancelCtx acquires a mutex, and this runs once per raw log (not per tx) inside the innermost decode loop. Combined with the per-log budget.Tripped() and budget.Reserve, that's three synchronization ops per log on the hottest loop in the range path. The tx-level check on line 349 already bounds cancellation latency to one receipt; consider dropping this one, or checking every N logs, unless a single receipt can hold enough logs to matter.
| localLogs := f.globalLogSlicePool.Get() | ||
|
|
||
| for _, block := range batch { | ||
| f.GetLogsForBlockPooled(block, crit, &localLogs) | ||
| if budget.Tripped() { | ||
| break | ||
| } | ||
| if err := f.GetLogsForBlockPooled(block, crit, &localLogs, budget); err != nil { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Sort the local batch |
There was a problem hiding this comment.
🟡 The block-by-block eth_getLogs fallback (GetLogsByFilters → processBatch → GetLogsForBlockPooled → collectLogs) only checks budget.Tripped() while scanning blocks/receipts and never observes ctx.Err(), unlike the sibling litt range-query path that this same PR instrumented with cancellation checks at window/block/tx/log granularity. A canceled or disconnected client request on this path (blockHash queries, or a pebble-backed node without range-query support) keeps scanning up to maxBlock (default 2000) blocks of receipts instead of aborting early — wasted CPU/IO, not incorrect results.
Extended reasoning...
The gap. GetLogsByFilters in evmrpc/filter.go threads the request ctx through the pre-scan phase (latestHeight/earliestHeight/tryFilterLogsRange/fetchBlocksByCrit), but once fetchBlocksByCrit returns and the block-by-block fallback begins, ctx is never referenced again. The for block := range blocks loop and processBatch's inner loop over each block only check budget.Tripped(). GetLogsForBlockPooled and collectLogs — both modified in this PR to add the *receipt.LogBudget parameter and error returns — take no context.Context argument at all; collectLogs derives its own sdk.Context via f.ctxProvider(block.Block.Height), entirely independent of the request context. So once a client disconnects or cancels mid-scan, nothing in this path observes it.\n\nContrast with the sibling path. This same PR instrumented the litt range-query path (tryFilterLogsRange → normalizeRangeQueryLogs, and litt_tag_index.go's filterLogsByTags/blockLogs/candidateBlockLogs) with ctx.Err() checks at window, per-block, per-tx, and per-log granularity, specifically so a canceled request aborts promptly (there's even a dedicated test file, litt_ctx_internal_test.go, exercising this). The block-by-block fallback got the analogous budget-based early-abort treatment (GetLogsForBlockPooled/collectLogs gained the budget param, and processBatch/the outer batching loop check budget.Tripped()), but the ctx-based treatment was omitted, leaving this as the one path in the PR still unresponsive to cancellation.\n\nConcrete trigger. This path is used whenever tryFilterLogsRange returns ErrRangeQueryNotSupported (pebble-backed receipt stores, which index receipts by tx hash rather than block number) or whenever the query sets crit.BlockHash. On such a backend, a wide eth_getLogs request (or an eth_newFilter/eth_subscribe("logs") poll) over up to maxBlock (2000) blocks with a filter that stays under max_log_no_block/max_log_bytes will run to full completion — fetching and scanning every block/receipt from disk under the dbReadSemaphore and worker pool — even after the HTTP client has disconnected or the request's context has been canceled, because nothing in this path ever calls ctx.Err().\n\nStep-by-step: (1) client issues eth_getLogs with a 2000-block range on a pebble-backed node; (2) tryFilterLogsRange returns ErrRangeQueryNotSupported, falling through to fetchBlocksByCrit + the batch/worker-pool scan; (3) client disconnects, canceling ctx; (4) processBatch's loop (for _, block := range batch { if budget.Tripped() { break }; ... }) and GetLogsForBlockPooled/collectLogs keep running because neither checks ctx.Err() — only budget.Tripped(), which won't trip unless the matched-log/byte cap is actually exceeded; (5) the full 2000-block scan completes and is discarded by the (now-gone) caller, having consumed dbReadSemaphore slots and worker-pool time for nothing.\n\nWhy this is low severity, not blocking. All three independent verifiers converged on the same three points, which I agree with: (1) this is not a correctness bug — the query still returns the right answer, the only cost is wasted CPU/I/O on an already-abandoned request; (2) it's substantially pre-existing — GetLogsForBlockPooled/collectLogs never took a ctx before this PR either, and this PR only added the budget-based abort as an improvement, it didn't regress any existing cancellation handling; (3) exposure is narrow in the common production configuration, since litt-backed nodes use the ctx-aware range-query path for ordinary block-range queries — this fallback mainly matters for blockHash queries (single block, trivial cost) or pebble-backed/non-litt backends.\n\nSuggested fix. Thread ctx into GetLogsForBlockPooled/collectLogs (mirroring the litt path's approach) and add a ctx.Err() check alongside the existing budget.Tripped() checks in processBatch's inner loop and the outer for block := range blocks loop in GetLogsByFilters, so a canceled request aborts the block-by-block scan the same way it now aborts the litt range-query scan.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1275398. Configure here.
| if mid <= lastToHeight { | ||
| // Halving cannot exclude any block the cursor has not already passed. | ||
| return nil, 0, err | ||
| } |
There was a problem hiding this comment.
Backoff wedges on two-block overflow
Medium Severity
getLogsByFiltersWithBackoff treats mid <= lastToHeight as unnarrowable and returns the cap error. For GetFilterChanges, lastToHeight is the inclusive scan start, so a two-block window [cursor, cursor+1] yields mid == cursor and aborts before trying the single-block prefix. When only the tip overflows, logs from the preceding under-cap block are never delivered and the filter cursor never advances.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1275398. Configure here.
There was a problem hiding this comment.
Solid, well-tested memory-bounding change: the LogBudget plumbing, the count/byte split on the range path, the per-window candidate discard, and the polling-path backoff wrapper all hold up under scrutiny, and test coverage is unusually thorough. The one blocking issue is rangeQueryWindowBlocks = 4, which cuts litt fan-out to 4 concurrent block scans (~4× more store round-trips on wide eth_getLogs) without buying any additional memory bound over a window sized to log-filter-parallelism.
Findings: 1 blocking | 12 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Both second-opinion passes produced no output:
codex-review.mdandcursor-review.mdare empty files, so this review reflects only my own analysis. - No metric/counter is emitted when either cap trips. Since bounded
eth_getLogsqueries that previously succeeded will now fail (default 10000 matches), operators have no way to observe rollout impact or tunemax_log_no_block/max_log_bytesfrom data. A counter onErrTooManyLogs/ErrTooManyLogBytes(labelled by dimension) would make this safer to ship. - PR description is stale relative to the final diff: it describes renaming
applyOpenEndedLogLimit->applyOpenEndedBlockWindow, but the final diff deletes the helper and the open-ended clamping infetchBlocksByCritentirely. That removal is behaviour-neutral (the pre-existing unconditionalblockRange > maxBlockcheck inGetLogsByFilters, its only caller, already rejected those ranges first, making the clamp unreachable), but it is unrelated cleanup folded into this PR and the description should be corrected. getLogsForTx(sei-db/ledger_db/receipt/receipt_store.go:397) is now referenced only fromexport_test.goand the read bench aftercandidateBlockLogswas inlined. Worth a comment noting it is test-only, or dropping it in favour of the test helper.- The byte cap on the block-by-block fallback path is only covered by unit tests on
LogBudgetitself (filter_budget_internal_test.go);TestFilterGetLogsMatchedLogCapcovers the count cap end-to-end but not bytes. An end-to-end fallback byte-cap case would close the gap. evmrpc/confignow importssei-db/ledger_db/receiptsolely forDefaultMaxLogBytes, coupling the RPC config package to the storage layer. Consider defining the default inevmrpc/config(or a small shared constants package) instead.mergeSortedLogs(sortedBatches, limit)truncation is now dead: the budget guarantees the merged count never exceedslimit. Harmless as defence-in-depth, but the function's doc comment about "stops once limit logs have been collected" no longer describes a reachable path.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // rangeQueryWindowBlocks bounds how many blocks tryFilterLogsRange asks the | ||
| // litt store to materialize per call, so raw candidates are discarded | ||
| // between windows instead of accumulating over the whole query range. | ||
| rangeQueryWindowBlocks = 4 |
There was a problem hiding this comment.
[blocker] A 4-block window caps litt's per-query fan-out at 4 concurrent block scans, well under the configured receipt-store.log-filter-parallelism (default 16). For a 2000-block range that is ~500 sequential store calls / scan waves instead of ~125, i.e. roughly 4x the round-trips on the hottest public RPC method.
The concern is that the small window buys nothing here: storeCandidateBudget() is a fresh byte-only budget per window, so transient candidate memory is bounded at max_log_bytes (64 MiB) regardless of window width. A window of 16 gives the identical memory bound with 4x the concurrency. The PR description already identifies this ("Widening the window toward log-filter-parallelism would recover most of the lost concurrency without weakening the byte cap") — please do that rather than shipping the regression.
Concretely: derive the window from the store's logFilterParallelism (or expose it as config) instead of hardcoding 4. RangeQueryWindowBlocksForTest already indirects the constant, so filter_range_cap_test.go's window-boundary assertions should follow without change.
| if matchLog(lg, crit) { | ||
| logs = append(logs, lg) | ||
| for _, rawLog := range receipt.Logs { | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
[suggestion] These two checks run once per raw log, inside the innermost loop of the tag-index scan. ctx here is the errgroup's derived cancelCtx, and cancelCtx.Err() takes c.mu — so all logFilterParallelism workers now contend on a single mutex once per log decoded, on the hottest path in the store.
The per-transaction checks at lines 349-354 already bound cancellation latency to one receipt's worth of work (receipt.Logs is small), and budget.Reserve below already returns the trip error for this worker. Dropping the per-log ctx.Err()/Tripped() pair should be free in responsiveness terms and removes the contention.
| ErrTooManyLogBytes = errors.New("query matches too many log bytes") | ||
| ) | ||
|
|
||
| func NewTooManyLogsError(limit int64) error { |
There was a problem hiding this comment.
[suggestion] Since bounded queries now fail where they previously returned all matches, the error text matters for client recovery. ethers.js, viem, and subgraph/indexer tooling auto-split log ranges by pattern-matching geth's canonical message query returned more than %d results. "query matches too many logs: result exceeds the maximum of %d logs" won't be recognised, so those clients will surface a hard failure instead of retrying with a narrower range.
Suggest wording the wrapped message to include geth's phrasing (the sentinel ErrTooManyLogs and the "narrow the block range" hint can stay), so existing clients back off automatically. Same for NewTooManyLogBytesError.
| // window and retries so the cursor still advances instead of wedging forever. | ||
| func (f *LogFetcher) getLogsByFiltersWithBackoff(ctx context.Context, crit filters.FilterCriteria, lastToHeight int64) ([]*ethtypes.Log, int64, error) { | ||
| narrowed := crit | ||
| for { |
There was a problem hiding this comment.
[suggestion] Each retry re-runs the full windowed scan from scratch, so a wedged polling filter costs up to ~log2(range) ≈ 11 complete scans per poll — and that amplification kicks in exactly when the node is already under log-volume pressure. Worth bounding the number of halvings (e.g. 3-4) and/or logging at debug when backoff engages, so a pathological filter can't turn one poll into an order-of-magnitude load multiplier.
Termination itself is sound: curEnd > begin is required and mid = begin + (curEnd-begin)/2 < curEnd, so the range strictly shrinks each iteration.
| // LogBudget tracks matched-log count and estimated heap bytes for eth_getLogs | ||
| // queries. Call Reserve before appending each log; Reserve returns an error | ||
| // without charging the budget when either ceiling would be exceeded (> limit, | ||
| // not >=). Concurrent Reserve calls may overshoot by ~O(workers) before Tripped |
There was a problem hiding this comment.
[nit] "Concurrent Reserve calls may overshoot by ~O(workers) before Tripped is observed" reads as if usedCount can exceed maxLog. It can't — Reserve holds mu for the whole check-and-charge and returns before charging on overflow, which is exactly what TestLogBudgetConcurrentSlack asserts. The real slack is in already-materialized logs held in per-worker slices, not in the counters. Suggest rewording to say the counters are exact and only in-flight per-worker buffers can overshoot.
| // Drain any blocks still buffered in the channel after an early abort so | ||
| // they can be GC'd promptly. fetchBlocksByCrit fully buffers the channel and | ||
| // closes it before returning, so producers are never blocked here. | ||
| for range blocks { |
There was a problem hiding this comment.
[nit] This drain is a no-op: fetchBlocksByCrit allocates res with capacity end-begin+1 and closes it before returning, so nothing is ever blocked, and the channel plus its buffered blocks become unreachable (hence collectable) as soon as this function returns whether or not it is drained. The comment's stated rationale ("so they can be GC'd promptly") doesn't hold. Safe to delete, or keep it and adjust the comment to say it's purely defensive against a future unbuffered/streaming producer.
2 participants
eth_getLogscould materialize an unbounded result set: a wide block range orloose filter that matched millions of logs, or logs with large
Datapayloads, would allocate them all before returning, spiking peak memory. The
block-range and open-ended windowing caps limited how many blocks were
scanned, but nothing capped how many matching logs — or how many bytes of
log data — were held in memory at once.
This PR pushes two independent caps into the receipt store so a query aborts
instead of accumulating past the limit:
max_log_no_blockconfig(
evm.max_log_no_block, default 10000), now enforced for both boundedand open-ended block ranges, not just open-ended ones.
max_log_bytesconfig(
evm.max_log_bytes, default 64 MiB), bounding the total estimated heapfootprint (
Address+Data+Topics+ per-log overhead) of thein-memory result.
Both are tracked by a new
receipt.LogBudget:Reserve(log)is charged permatched log at append time and returns an error the moment either ceiling is
exceeded, so overflow is a clear error (
ErrTooManyLogs/ErrTooManyLogBytes)rather than silent truncation.
Where the caps are enforced
ReceiptStore.FilterLogsnow takes a*LogBudgetinstead of a barelimit int64;nildisables all caps (preserves prior behavior forinternal callers, benches, and the simulator).
filterLogsByTags→blockLogs→candidateBlockLogs) — the parallel fan-out chargesbudget.Reservepermatched log; the worker that trips the budget returns its error, cancelling
the errgroup so already-scheduled blocks bail before reading.
count can include synthetic/non-EVM-visible logs that
eth'snormalizeRangeQueryLogslater drops. Enforcing the count cap on that rawcount would produce false-positive
ErrTooManyLogserrors for querieswhose true (EVM-visible) result is under the limit. To avoid this,
tryFilterLogsRangepasses the litt store a byte-only budget(
storeCandidateBudget(),maxLog=0) — bounding materialization memorywithout prematurely capping on count — and the authoritative count +
byte cap is enforced afterward in
normalizeRangeQueryLogs, against thenormalized, EVM-visible log set.
tryFilterLogsRangewalks the requestedrange in successive
rangeQueryWindowBlocks-sized windows (currently 4)instead of one
FilterLogscall over the whole range. Each window's rawcandidates are normalized against the query-wide budget and discarded
before the next window is requested, so transient candidate memory does
not accumulate across the full range. The hard memory bound is still the
byte budget; windowing is an intentional memory/latency tradeoff on top
of that.
GetLogsByFilters) —pooledCollector.Appendcharges the same
LogBudgetper log; workers stop materializing blocks andthe fan-out stops queueing batches once the budget trips, then the overflow
is reported after
wg.Wait.applyOpenEndedLogLimit→applyOpenEndedBlockWindowtoreflect that it windows the block range; the matched-log cap is enforced
separately by the
LogBudget.No new config surface beyond
max_log_bytes;max_log_no_blockis reusedfor the count cap, now with corrected doc comments (see below).
Latency tradeoff (windowed litt path)
Because each window is only 4 blocks wide, litt's per-query fan-out
(
receipt-store.log-filter-parallelism, default 16) is effectively cappedat 4 concurrent block scans for the duration of that window — even though
the store is configured for higher parallelism. For a full 2000-block range
this is roughly 500 sequential store calls / scan waves instead of
~
ceil(2000/16) = 125waves under a single full-range call (~4× more waveson store-bound wide queries). That is an intentional memory/latency
tradeoff: the byte budget is what actually bounds memory; the small window
lets candidates be dropped between chunks. Widening the window toward
log-filter-parallelismwould recover most of the lost concurrency withoutweakening the byte cap.
Config doc corrections
max_log_no_blockdoc comment now states it applies to both boundedand open-ended ranges and that a non-positive value falls back to
DefaultMaxLogLimit, rather than the stale "0 disables the cap" claim(unreachable —
NewFilterAPIalways coerces<= 0to the default).CHANGELOG.mdentry for this PR.fromBlockortoBlock) that matched morethan
max_log_no_blocklogs were previously silently truncated viamergeSortedLogs(..., limit). They now returnErrTooManyLogsinstead.fromBlockandtoBlockset) previously hadno matched-log cap and could return arbitrarily large result sets
successfully. They are now capped too: a bounded query matching more than
max_log_no_block(default 10000) logs will now fail withErrTooManyLogswhere it previously succeeded.or filter criteria.
Testing performed to validate your change
log_budget_test.go: count/byte ceilings, concurrentReserveovershootbounds, byte-only budget behavior.
littidx_test.go(TestLittIdxFilterLogsLimitand related): FilterLogsreturns all logs at or below the limit, returns the budget's error when
exceeded, and treats a
nil/disabled budget as uncapped.filter_range_cap_test.go: end-to-end coverage of the range-query path'sbyte-only store budget + authoritative post-normalization count/byte cap,
including the synthetic-log false-positive scenario, plus window-boundary
and between-window cancellation coverage for
tryFilterLogsRange.filter_test.go/filter_budget_internal_test.go: block-by-blockfallback budget enforcement and early-abort behavior.
FilterLogscall sites (tests, benches, simulator,the pebble backend stub, and
fakeReceiptStoreinwatermark_manager_test) to the new*LogBudgetsignature.parallel-order) passes unchanged with the new parameter.