Add shared scan budget for KV tx_search and block_search#3747
Add shared scan budget for KV tx_search and block_search#3747amir-deris wants to merge 6 commits into
Conversation
PR SummaryMedium Risk Overview New RPC setting
Unit tests cover budget accounting, concurrency, and soft overshoot under parallel leases. Reviewed by Cursor Bugbot for commit 6c11fb6. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, defensive change adding a shared process-wide scan budget to the KV tx/block indexers, plus a welcome panic→error conversion in the tx match paths. No blockers found; the main gaps are missing tests for the new budget mechanism and some minor doc/coverage nits.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No tests cover the new ScanBudget/ScanLease (Codex P2). The mechanism is correctness- and concurrency-sensitive (atomic in-flight counter, per-lease held tracking, release-on-defer, shared cap across tx+block searches). Add unit tests for: Visit charging + ErrScanBudgetExceeded at the ceiling, Release returning the charge, nil/zero-max = unlimited, and concurrent leases sharing one budget. An integration test asserting a broad tx_search/block_search aborts with ErrScanBudgetExceeded would also protect the wiring.
- Backward-compat note: existing config.toml files that predate this change lack
max-search-scan-budget, so mapstructure leaves it at 0 (= unlimited) on upgrade. Only freshly generated configs get the 2M default, meaning already-deployed nodes won't gain the protection until they regenerate/edit their config. Worth calling out in the PR/release notes. - Grammar in budget.go: the Visit doc comment
...exceeds the budget's ceilingis missing a trailing period. - cursor-review.md is empty (Cursor pass produced no output); REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied. No prompt-injection content was detected in the PR diff or description.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // block_search requests may visit at once. This is a process-wide safety | ||
| // cap that bounds peak memory (and scan CPU) under broad or highly | ||
| // concurrent search load: it is shared across requests, not applied | ||
| // per-query 0 disables the cap (not recommended on public nodes). |
There was a problem hiding this comment.
[nit] Typo/missing sentence break: ...not applied per-query 0 disables the cap should be ...not applied per-query. 0 disables the cap.
| } | ||
|
|
||
| // Charge each driver entry the scan walks against the shared budget. | ||
| if err := lease.Visit(1); err != nil { |
There was a problem hiding this comment.
[suggestion] In the bounded fast path only driver entries are charged here; the per-candidate candidateMatches probes (store.Has(...), and hasEvent's prefix iterate in the block indexer) are not charged against the lease (Codex P1). These probes are bounded by the charged driver-entry count times the fixed number of equality probes, so this is a constant-factor gap rather than an unbounded bypass — but a query with many equality conditions still performs several uncharged KV reads per charged driver entry. Consider charging each probe (e.g. lease.Visit(1) per equality probe) so the cap reflects actual work, or document that probes are intentionally excluded.
There was a problem hiding this comment.
Adds a process-wide shared scan budget bounding the number of KV index entries in-flight tx_search/block_search requests may visit. The implementation is correct, nil-safe, and well-documented, and it also converts iterator/DB panics in the tx indexer into returned errors. The only notable gap is the absence of tests for the new concurrency-critical ScanBudget primitive.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No tests for the new
indexer.ScanBudget(budget.go), which is concurrency-critical (Codex also flags this, P2). Add coverage for: ceiling enforcement / ErrScanBudgetExceeded at the boundary, Release restoring the shared counter after both success and error, nil/zero/negative-max unlimited mode, and concurrent leases sharing exhaustion across tx and block indexers. The atomic logic is simple and looks correct, but a leaked or double-counted charge would silently degrade all searches, so it warrants direct tests. - Behavioral note worth documenting for operators: because the budget is shared and each search holds its charge until Release (at search end), a small/legitimate query can be rejected with ErrScanBudgetExceeded purely because other broad queries are concurrently in flight. This is the intended backstop semantics, but the error surfaces to end users as a failed RPC — confirm ErrScanBudgetExceeded maps to a sensible RPC error/log level rather than being treated as an internal server error.
- cursor-review.md was empty (Cursor produced no output for this PR); the codex-review.md pass produced a single finding (missing budget tests), incorporated above.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| l.held += n | ||
| if total > l.budget.max { | ||
| return ErrScanBudgetExceeded | ||
| } |
There was a problem hiding this comment.
[nit] Minor: Visit adds n to the shared counter before checking the ceiling, so the cap is soft — under concurrency it can overshoot by roughly (number of concurrent leases) entries before any of them observe the excess. That's fine for a safety backstop, but worth a one-line comment noting the cap is approximate/soft so a future reader doesn't assume a hard limit.
|
|
||
| // Charge each driver entry the scan walks against the shared budget. | ||
| if err := lease.Visit(1); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
[nit] In the bounded fast path, each driver entry visited is charged even though memory here is bounded by opts.Limit (results kept), not by entries walked. A fast-path query that skips many duplicate/non-matching driver entries can therefore consume a disproportionate share of the shared budget relative to its actual memory footprint, potentially rejecting other concurrent queries earlier than memory pressure would warrant. This is acceptable as a CPU-scan backstop and is documented, but consider noting the distinction so operators tuning max-search-scan-budget understand it bounds scan work, not just memory.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## amir/plt-786-bound-kv-tx-search #3747 +/- ##
===================================================================
- Coverage 58.39% 58.39% -0.01%
===================================================================
Files 2188 2189 +1
Lines 178997 179057 +60
===================================================================
+ Hits 104530 104556 +26
- Misses 65191 65212 +21
- Partials 9276 9289 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
A well-scoped, correct addition of a shared, process-wide scan budget bounding index entries visited by concurrent tx_search/block_search, plus a safe panic→error conversion in the tx KV fallback scans. No blocking issues; the main gap is that the new concurrency-sensitive budget code ships without unit tests.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Missing tests for the new ScanBudget/ScanLease (sei-tendermint/internal/state/indexer/budget.go). This is the core, concurrency-sensitive enforcement path and it is entirely untested — the existing test changes only update constructor call sites (all passing nil = unlimited), so the budget-exceeded, release-on-success/error, unlimited/zero-max, and shared tx+block contention behaviors are never exercised. Recommend a budget_test.go covering: Visit exceeding max returns ErrScanBudgetExceeded; Release returns held charge to InFlight (including after an error abort); nil/zero/negative max behaves as unlimited; and two leases on one budget summing to the cap. (Also raised by Codex.)
- Design/operability note: the budget is shared across requests, not per-query (as documented). Under high concurrency a small, legitimate query can be rejected with ErrScanBudgetExceeded because other in-flight scans have consumed the shared 100k budget. This is intentional, but operators tuning max-search-scan-budget should be aware failures are load-dependent rather than per-query, and it may be worth surfacing a metric (InFlight() is already exposed) so the cap can be tuned.
- Repository REVIEW_GUIDELINES.md is empty/absent and the Cursor second-opinion review (cursor-review.md) produced no output; only the Codex pass contributed a finding (the missing-tests item above), which is incorporated.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // aggregate in-flight charge exceeds the budget's ceiling. | ||
| func (l *ScanLease) Visit(n int64) error { | ||
| if l == nil || l.budget == nil || l.budget.max <= 0 || n == 0 { | ||
| return nil |
There was a problem hiding this comment.
[nit] Minor: when the budget is exceeded, n is still added to both inFlight and held before returning the error. This is intentional and correct (the deferred Release() returns the charge), but it means the shared inFlight counter can transiently overshoot max by the sum of one in-flight n per concurrent lease. Harmless with n==1 everywhere today; worth a one-line comment noting the overshoot is reclaimed on Release so a future caller passing large n doesn't misread the invariant.
There was a problem hiding this comment.
Adds a process-wide, nil-safe scan budget shared by the tx and block KV indexers that bounds concurrent index-entry scans, plus a welcome panic→error conversion in the tx indexer's match/matchRange. Code is correct and clean; the main gap is that the new safety feature ships with no dedicated tests.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap (also raised by Codex): the core safety mechanism in sei-tendermint/internal/state/indexer/budget.go has no dedicated tests. Nothing verifies that Visit returns ErrScanBudgetExceeded at the threshold, that Release returns the charge to the shared pool, that a nil/zero/negative max behaves as unlimited, that the budget is actually shared across the tx and block indexers, or that a Search aborts cleanly with the error under a low budget. All updated sink tests pass nil (unlimited), so the feature and its wiring are effectively unexercised. Recommend adding unit tests for ScanBudget/ScanLease plus at least one indexer Search test that exercises budget exhaustion.
- Cursor's second-opinion pass produced no output (cursor-review.md is empty); REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.
- The soft-cap overshoot under concurrency is documented in budget.go, but consider whether returning ErrScanBudgetExceeded to legitimate broad queries under concurrent load (vs. the prior behavior of returning partial results) is the desired client-facing tradeoff; the default 100_000 mitigates this but it is a behavior change worth confirming.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| default: | ||
| panic("other operators should be handled already") | ||
| return nil, fmt.Errorf("other operators should be handled already") |
There was a problem hiding this comment.
[nit] Nit: fmt.Errorf with a constant string and no format directives; errors.New("other operators should be handled already") is the idiomatic form (some linters flag this).
There was a problem hiding this comment.
Adds a process-wide shared scan budget bounding index entries visited by in-flight tx_search/block_search, plus a helpful panic→error conversion in the tx KV scanner. The implementation is correct and well-tested at the unit level; remaining notes are non-blocking (integration coverage, upgrade default, minor contention/UX tradeoffs).
Findings: 0 blocking | 6 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Testing gap (also raised by Codex, P2): budget_test.go only exercises the ScanBudget/ScanLease counter mechanics. There is no end-to-end test that drives a real budgeted tx/block Search to verify per-entry Visit charging in the fast and fallback paths, ErrScanBudgetExceeded propagation up through Search, lease Release on the abort path (no over-charge left behind), and that the tx and block indexers actually share one budget. An integration test seeding an index and running a search against a tiny budget would lock in the wiring.
- Upgrade default: an existing node's config.toml that predates this change has no
max-search-scan-budgetline, so mapstructure leaves it at the Go zero value (0), which the code treats as 'unlimited'. Only freshly generated configs get the 100_000 default, so upgrading nodes are not protected until an operator adds the line. This mirrors the existingmax-tx-search-resultsbehavior, but it's worth calling out in release notes. - Shared-budget tradeoff: because the cap is process-wide and checked per visited entry, once heavy concurrent queries saturate the pool, cheap/legitimate queries that happen to Visit at that moment also fail with ErrScanBudgetExceeded. This is the intended safety-valve behavior but can surface as sporadic search failures under load; consider documenting/operationalizing (e.g. metric on InFlight) so operators can tune the ceiling.
- Minor performance note: Visit(1) issues an atomic.Add on a single shared counter for every index entry walked across all concurrent searches, adding cache-line contention in hot scan loops. Almost certainly negligible against per-entry DB/iterator cost, but noting it since it's on the innermost path.
- Consider confirming ErrScanBudgetExceeded surfaces to RPC clients as an intelligible error (overload/narrow-your-query) rather than a generic 500, so callers can distinguish it from a server fault.
- The Cursor second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty.
There was a problem hiding this comment.
Adds a process-wide shared scan budget bounding KV index entries visited by concurrent tx_search/block_search, plus a welcome panic→error conversion in the tx indexer. The implementation is correct and consistently wired; the only notable gap is missing end-to-end tests that the Search paths actually enforce and release the budget.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap (also raised by Codex): budget_test.go only exercises the ScanBudget/ScanLease counter primitive. No test verifies that tx or block
Searchactually opens a lease, callsVisitper entry, aborts withErrScanBudgetExceeded, and releases the charge on return. A wiring regression — e.g.WithScanBudgetno longer called inNewEventSink, or aVisitcall dropped from amatch/matchRangeloop — would pass unnoticed. Recommend at least one test driving a real KV Search against a tiny budget and asserting the error plus thatInFlight()returns to 0 afterward. - Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex's review (one P2 test-coverage note) is incorporated above.
- The budget is genuinely process-wide across both search types and only released when a search returns, so
heldaccumulates over a search's full lifetime rather than tracking instantaneous peak. This is a reasonable design, but on a busy node a single very broad query can consume most of the shared budget for its whole duration and cause unrelated concurrent searches to fail with ErrScanBudgetExceeded. Worth confirming the default (100_000) is sized with that fan-out interaction in mind. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // order_by order and capped at opts.Limit during the scan, so a broad query | ||
| // does not materialize and sort the full match set. Otherwise the intersection | ||
| // is materialized as before, then ordered and capped. | ||
| // The entries the scan visits are charged against the |
There was a problem hiding this comment.
[nit] This doc-comment rewrite drops the previously documented behavior of opts/the bounded fast path (streaming in order_by order, capping at opts.Limit, avoiding full materialization) and replaces it with only the budget note, so that context is lost from the Search doc. The new sentence also has an awkward mid-phrase line break ("charged against the / shared scan budget"). Consider keeping the opts/fast-path description and appending the budget note.
| break | ||
| } | ||
|
|
||
| // Charge each driver entry the scan walks against the shared budget. | ||
| if err := lease.Visit(1); err != nil { |
There was a problem hiding this comment.
🔴 Range-only block_search (e.g. block.height <= 100) and tx_search (e.g. tx.height >= N) can now hard-fail with ErrScanBudgetExceeded on chains larger than the 100k default budget when the scan direction is opposite to where matches live. In searchBounded/matchRange, lease.Visit(1) is charged before the range filter and there is no early break on the range bound, so on a 1M-block chain a query for block.height <= 100 with the default OrderDesc=true walks ~100k out-of-range heights before ever reaching a match and returns (nil, err) — zero results. Even 'give me recent N blocks' (e.g. block.height >= H-100) can collect its ~100 matches and then still error out because the loop keeps walking past the last match until opts.Limit (default 10k) is reached, discarding the collected results. Fix: add an early break in searchBounded/matchRange when direction+range prove no further matches are possible, or return partial results on Visit failure.
Extended reasoning...
What happens
For a range-only query like block.height <= 100 on a large chain with the default settings (MaxSearchScanBudget=100_000, MaxTxSearchResults=10_000, OrderBy="" → OrderDesc=true), the bounded fast path enters searchBounded with plan.driverEquality=nil, so it drives off the entire types.BlockHeightKey prefix (block/kv/kv.go:274). Because orderedcode.Append(nil, BlockHeightKey, height) preserves int64 numeric order, the reverse iterator walks from the highest indexed height downward.
Each loop iteration calls lease.Visit(1) before candidateMatches evaluates HeightInRange (kv.go:295 vs 304). When the height is out of range candidateMatches returns false, nil and the loop just continues — no break/seek on the range bound. On a chain with 1M blocks, iterations 1..999_900 walk heights 1_000_000..101, every one failing HeightInRange but each charging Visit(1). Iteration 100_001 makes inFlight = 100_001 > 100_000; Visit returns ErrScanBudgetExceeded; searchBounded returns (nil, err) — the client receives zero results and the misleading error "narrow the query or retry later" for a query that already matches exactly 100 blocks.
The symmetric ascending case fails identically: block.height >= 999_900 with OrderDesc=false walks 1..999_899 out-of-range before reaching any match.
A subtler case affects the "recent N blocks" pattern that Sei operators/tools actually run: block.height >= 999_900 with default OrderDesc=true collects 101 matches at heights 1_000_000..999_900, but because opts.Limit=10_000 >> 101, the len(results) >= opts.Limit break is unreachable. The scan continues down past h=999_899, 999_898, ..., every iteration charging Visit(1). After ~100k such iterations, budget exhausts and searchBounded returns (nil, err) — the 101 already-collected matches are discarded.
The tx path is affected via a different code path with the same root cause: tx/kv.go:planBounded rejects range-only queries (if len(equalities) == 0 { return boundedPlan{}, false }), so tx.height >= N with no equality drops into intersect → matchRange. matchRange charges Visit(1) per iterator step (line 743) with no range-bound early-break, iterating the full tx.height composite prefix. Because that secondary index stores height as a decimal string (fmt.Sprintf("%d", height)), its key order is lexicographic — not numeric — so even a range-aware early-break would be trickier there; seeking the iterator or returning partial results on budget error is the cleaner fix on the tx side.
Why existing safeguards do not save it
opts.Limitbreaks after collecting Limit matches; when the match set < Limit (the common case for narrow queries against a 10k default limit), the break is never reached.ctx.Err()only breaks on client-side cancellation; a synchronous RPC waiting on its own reply does not cancel.planBoundedcorrectly identifies the query as bounded (finite match set), butsearchBounded's iteration cost is bounded by the driver-prefix cardinality, not by the match set.
Why this is PR-material, not pre-existing
Before this PR, the same "walk the whole prefix, filter per candidate" behavior existed (from PLT-748), and the same query would have been slow-but-correct — walking N entries once and returning results. This PR wires MaxSearchScanBudget with a conservative 100_000 default and makes any Visit past that ceiling return ErrScanBudgetExceeded. That converts legitimate narrow queries whose implementation happens to be a broad scan from slow-successes into hard failures. On production Sei chains (400ms blocks, mainnet has orders of magnitude more than 100k indexed blocks) this is reachable on default config.
Addressing the "already accepted via seidroid P1" refutation
The seidroid P1 comment on tx/kv.go:344 flags a distinct concern: in the equality-driven fast path, memory is bounded by opts.Limit but the driver-entry count that consumes budget is not, so an equality query can consume budget disproportional to its memory footprint. The maintainer accepted that tradeoff for equality drivers because the driver prefix is at least scoped to the equality — some semantic relationship to the query.
The range-only failure here is functionally different in two important ways: (1) there is no equality driver, so the "driver" is the entire block.height prefix — every indexed block, unrelated to the query semantics; (2) the concern is not "fairness across concurrent queries" but a single query on an idle node failing on default config for its own sole scan. The PR description frames the budget as a backstop for "broad" fallback queries, but by this PR's own definition a block.height <= 100 query is narrow — it just has a driver implementation that walks broadly. The two conditions are addressable by the same class of fixes (early break / iterator seek / partial-result return) but they are distinct issues; the range-only case is not covered by the accepted P1 tradeoff.
Step-by-step proof (concrete)
- Client submits
block_searchwithquery="block.height <= 100",order_by=""on a node with 1_000_000 indexed blocks andMaxSearchScanBudget=100_000(default). rpc/core/blocks.go:329-331:req.OrderBy = ""→orderDesc = true.opts.Limit= 10_000 (defaultMaxTxSearchResults).block/kv/kv.go:Search→LookForRangesreturns{block.height: {UpperBound:100, IncludeUpperBound:true}}, no equalities.planBounded:len(equalities)==0,len(heightRanges)==1→ falls intocase len(plan.heightRanges) > 0, returnsplan{driverEquality:nil, heightRanges:[<=100]}, true.searchBoundedline 274:prefix = orderedcode.Append(nil, types.BlockHeightKey).prefixIterator(prefix, desc=true)→ ReverseIterator starting at h=1_000_000.- Loop iteration 1:
Visit(1)→inFlight=1.h=1_000_000.candidateMatches:HeightInRange(1_000_000, ≤100)= false →continue. - Iterations 2..100_000: same pattern, each charging
Visit(1)and continuing. - Iteration 100_001:
Visit(1)→inFlight.Add(1)= 100_001 > max=100_000 → returnsErrScanBudgetExceeded. searchBoundedreturns(nil, ErrScanBudgetExceeded)at line 296.- Client receives error "kv indexer scan budget exceeded; narrow the query or retry later" — for a query that already matches exactly 100 blocks.
Suggested fixes (any one sufficient)
- In
searchBounded, when driving off the primary block.height range withdriverEquality==nil, break the loop as soon asHeightInRangefails in a direction-consistent way (OrderDesc=trueandh < lowerBound→ break;OrderDesc=falseandh > upperBound→ break). Since orderedcode preserves int64 order onBlockHeightKey, this is safe. - On
Visitfailure, ifresultsis non-empty, return(results, nil)— matches thectx.Done()semantics elsewhere in the file and preserves already-collected matches for the "recent N blocks" case. - Seek the iterator to the range's inclusive bound key so out-of-range entries are skipped at the storage layer rather than walked one Visit at a time.
On the tx side, option 2 (return partial results on budget error) is the cleanest fit because the tx.height secondary index is lex-ordered and does not admit a simple numeric early-break.
🔬 also observed by seidroid
1 participant
Summary
Adds a process-wide scan budget that bounds the total number of KV index
entries all in-flight
tx_searchandblock_searchrequests may visit atonce. Where
max-tx-search-resultscaps the result set per query, this capsthe work done across every concurrent scan — a complementary safety valve
that bounds peak memory (and scan CPU) under broad or highly concurrent search
load.
Builds on the earlier tx/block KV search bounding work (PLT-786): the fast
paths already stream and cap results, but a broad fallback query — or many of
them at once — can still walk a large slice of the index. The shared budget is
the backstop for that case.
How it works
indexer.ScanBudget(budget.go): a single atomic-counter ceiling onconcurrently-charged entries.
nil/ zero / non-positive max all meanunlimited.
Searchopens aScanLeaseand callslease.Visit(1)at every indexentry it walks (driver-entry scans in the bounded fast path; each iterator
step in the
match/matchRangefallbacks). When the aggregate in-flightcharge exceeds the ceiling, the in-flight search aborts with
ErrScanBudgetExceededrather than continuing to accumulate.lease.Release()(viadefer) returns the charge to the shared pool whenthe search returns, so the budget tracks concurrent work, not cumulative.
sink.EventSinksFromConfig), so the cap is truly process-wide across bothsearch types.
Config
New RPC config
max-search-scan-budget(default100_000;0disables,not recommended on public nodes). Validated as non-negative in
ValidateBasicand documented in the generated
config.toml.Other changes
match/matchRangenow return errors instead ofpanic-ing on iterator/DB failures, so a scan failure (budget exceeded orotherwise) surfaces cleanly to the caller instead of crashing the node.
kv.NewEventSinkgains a*indexer.ScanBudgetparameter. Call sites thatonly write (event reindexing) and tests pass
nil(unlimited).Testing
go test ./sei-tendermint/internal/state/indexer/...