feat(indexer): push result cap and ordering into KV tx search (PLT-748)#3708
feat(indexer): push result cap and ordering into KV tx search (PLT-748)#3708amir-deris wants to merge 10 commits into
Conversation
amir-deris
changed the title
PR SummaryMedium Risk Overview The KV tx indexer gains the same bounded pattern as block search: a fast path scans a driver equality’s Reviewed by Cursor Bugbot for commit ce16702. 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).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3708 +/- ##
==========================================
- Coverage 59.91% 58.44% -1.47%
==========================================
Files 2288 2188 -100
Lines 190180 178182 -11998
==========================================
- Hits 113952 104147 -9805
+ Misses 66064 64800 -1264
+ Partials 10164 9235 -929
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 refactor that pushes the result cap and (height, index) ordering into the KV tx indexer (fast path for equality/tx.height-range queries, materialize-then-bound fallback otherwise) and hoists the shared bounding helpers into the indexer package. No blocking issues found; a few non-blocking notes, including an assessment of Codex's deprecated-key concern.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's only finding (regression on deprecated slash-delimited rows like
sender/addr1/height/indexforsender = 'addr1') is most likely NOT a regression: both the oldmatchpath and the newsearchBoundedderive the scan prefix from the sameprefixFromCompositeKeyAndValue(tag, value), which is an orderedcode encoding (sender\x00\x01addr1\x00\x01). The manually-written legacy raw-byte keysender/addr1/...diverges at byte 6 (0x00vs'/'), so it never shared the orderedcode prefix and was never visited by either the old or new iterator — the query returned empty before this PR too. The real issue is thatTestTxSearchDeprecatedIndexingasserts vacuously (it ranges overresultswithout arequire.Len), so it cannot distinguish empty from non-empty and never actually exercised deprecated support. Recommend strengthening that test with an explicit length assertion to remove the ambiguity Codex flagged. - Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - RPC
TxSearchre-sorts and re-caps results that the KV indexer has already ordered and bounded. This is intentional and documented as a safety net for sinks that ignore the limit, so it's fine, though for the KV fast path it is redundant work. - Behavioral tightening (acceptable/likely an improvement): invalid
order_bynow errors up front before the sink loop, whereas previously the error was produced inside the per-sink sort switch. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, well-tested follow-up that bounds the KV tx-search scan path (fast point-probe path + materialize-then-cap fallback) and de-duplicates shared bounding helpers into the indexer package. The logic is correct and the fast/fallback paths are consistent; no blocking issues found, only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review (cursor-review.md) produced no output; the Codex review (codex-review.md) reported no material findings but noted it could not run the tests because the sandbox couldn't fetch the Go 1.25.6 toolchain. This review's correctness assessment is therefore based on static analysis, not a local test run.
- Test coverage suggestion: TestTxSearchBounded's "bounded cap matches unbounded prefix" invariant compares each query against its own unbounded run, but there is no direct cross-check that the fast path and the fallback path return identical ordered results for a query where both could in principle apply. Given HeightInRange mirrors matchRange's comparisons this is low risk, but an explicit fast-vs-fallback equivalence assertion would harden against future drift between the two paths.
- The searchBounded
seenmap is defensive — under the current single-driver-prefix scan each tx yields exactly one key, so duplicates cannot occur unless a tx re-emits the identical indexed attribute (deduped at the DB key level anyway). It is harmless but worth a one-line comment clarifying it guards a theoretical case rather than an observed one. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // sort results (must be done before cap and pagination) | ||
| switch req.OrderBy { | ||
| case DescendingOrder, "": | ||
| if orderDesc { |
There was a problem hiding this comment.
[nit] Minor: the KV indexer now already returns results ordered and capped to MaxTxSearchResults, so this RPC-layer re-sort is redundant work for the KV sink. It's intentional and documented as a safety net for sinks that ignore the limit/ordering, and the data set is bounded by MaxTxSearchResults, so the cost is negligible — noting only for awareness. No change required.
There was a problem hiding this comment.
A well-structured, well-tested follow-up that bounds the KV tx-search scan path (fast point-probeable path + materialize-then-cap fallback) and de-duplicates shared bounding helpers into the indexer package. I found no correctness, security, or performance blockers; only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor second-opinion pass produced no output. Codex (codex-review.md) reported no material findings (it could not rungo testbecause the sandbox lacked the Go 1.25.6 toolchain).- Minor behavior change in
TxSearch: an invalidorder_byis now rejected up front, before the sink loop, instead of only inside the KV-sink branch. This is strictly better (consistent withBlockSearch) but means a malformedorder_bynow errors even on nodes without a KV sink. Worth a mention only; no action needed. - The RPC layer still sorts results after the indexer already sorts and caps them. This is intentional (documented as a safety net for sinks that ignore the limit/ordering) and cheap since the set is already bounded — no change requested.
- Verified the moved helpers (
BoundedCap,HeightInRange,PrefixUpperBound,maxBoundedPrealloc) are byte-identical to the formerblock/kvlocals, andblock/kv/util.gono longer references theindexerpackage after dropping the import — no dangling import. Could not rungo build/go testhere (sandbox denied), so a CI green run is the final confirmation.
| cancel() | ||
| results, err := idx.Search(ctx, query.MustCompile(`app.name = 'sei'`), indexer.SearchOptions{Limit: 5, OrderDesc: true}) | ||
| require.NoError(t, err) | ||
| require.Empty(t, results) |
There was a problem hiding this comment.
This cancels the context before calling Search and then asserts the result is empty, so it exercises the top-level guard that returns early rather than the in-loop path in searchBounded that gathers rows and returns them on cancellation. The behavior the name describes, partial results before the cancel, never runs here, and the test would still pass if that path were broken. Worth refining it to cancel mid-scan after a few rows are gathered and assert a non-empty ordered prefix, so it actually guards the partial-return path.
There was a problem hiding this comment.
A clean, well-documented refactor that pushes the result cap/ordering into the KV tx indexer, but it removes the RPC-layer sort in TxSearch without updating the existing TestTxSearchCapAppliedAfterSort, whose mock sink does not sort — so that test now fails and CI will break.
Findings: 2 blocking | 2 non-blocking | 1 posted inline
Blockers
- sei-tendermint/internal/rpc/core/tx_test.go: TestTxSearchCapAppliedAfterSort will fail. Its mock EventSink.SearchTxEvents returns makeTxResults(20) (heights 1..20 ascending) regardless of SearchOptions, and TxSearch no longer re-sorts at the RPC layer. For OrderBy=desc the cap now keeps [1..5] instead of the expected [20,19,18,17,16]. Fix by making the mock honor OrderDesc, or relocate this top-N correctness assertion to the indexer-level tests (the new tx/kv tests already cover ordering). This is confirmed by Codex's P1.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- The fallback path still fully materializes the intersection before applying the cap; this is clearly documented and tracked as a follow-up (PLT-786), and the RPC layer still bounds the returned set, so it is an acceptable transient memory/CPU bound rather than an unbounded result.
| default: | ||
| return nil, fmt.Errorf("expected order_by to be either `asc` or `desc` or empty: %w", coretypes.ErrInvalidRequest) | ||
| } | ||
| // Results already arrive ordered by (height, index) per orderDesc: |
There was a problem hiding this comment.
[blocker] Removing the RPC-layer re-sort breaks the existing TestTxSearchCapAppliedAfterSort (rpc/core/tx_test.go). That test's mock EventSink returns results in ascending height order regardless of SearchOptions and relied on TxSearch to sort before capping. With the sort gone, an OrderBy=desc request now caps the unsorted prefix ([1..5]) instead of the top-N ([20,19,18,17,16]), so the test's assertion fails and CI breaks. Update the test's mock to honor OrderDesc (or move the top-N assertion to the indexer-level tests) as part of this PR.
There was a problem hiding this comment.
The PR cleanly pushes the result cap and ordering into the KV tx indexer with a solid fast/fallback split and good test coverage, but removing the RPC-layer sort in TxSearch breaks the existing, unchanged TestTxSearchCapAppliedAfterSort and diverges from the BlockSearch pattern it claims to mirror.
Findings: 2 blocking | 2 non-blocking | 1 posted inline
Blockers
- TxSearch removes the RPC-layer sort but the existing (unmodified) TestTxSearchCapAppliedAfterSort in sei-tendermint/internal/rpc/core/tx_test.go still uses a mock sink that returns ascending heights 1..20 and ignores SearchOptions. With OrderBy=desc and no re-sort, the cap now keeps results[:5]=[1,2,3,4,5] instead of the asserted [20,19,18,17,16], so this test fails in CI. The sibling BlockSearch (blocks.go:348-353) keeps its RPC-layer sort before the cap and its analogous test passes for exactly that reason; TxSearch should mirror it (re-sorting an already-bounded set is cheap) or the test must be updated. (Confirmed; also raised by Codex.)
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- REVIEW_GUIDELINES.md and cursor-review.md were empty/missing, so no repo-specific guidelines and no Cursor second-opinion pass were available for this review.
- The in-code comment at tx.go:92-95 and the PR description both say TxSearch 'mirrors BlockSearch', but BlockSearch deliberately keeps the post-search sort as defense-in-depth while TxSearch dropped it — the two are no longer consistent. Worth aligning the behavior and the comment.
| default: | ||
| return nil, fmt.Errorf("expected order_by to be either `asc` or `desc` or empty: %w", coretypes.ErrInvalidRequest) | ||
| } | ||
| // Results already arrive ordered by (height, index) per orderDesc: |
There was a problem hiding this comment.
[blocker] Removing the RPC-layer sort breaks the existing (unchanged) TestTxSearchCapAppliedAfterSort: its mock sink returns ascending heights 1..20 and ignores SearchOptions, so with order_by=desc and no re-sort the cap now keeps [1,2,3,4,5] instead of the asserted [20,19,18,17,16]. The sibling BlockSearch you cite as the model (blocks.go:348-353) actually keeps its sort before the cap for exactly this reason. Re-sorting here is cheap since the set is already bounded to MaxTxSearchResults, and it restores the 'cap keeps the top-N by order_by, not an arbitrary prefix' invariant for any sink that ignores the limit/ordering. Please re-add the sort (mirroring BlockSearch) or update the test.
There was a problem hiding this comment.
Well-structured, thoroughly-tested follow-up that bounds the KV tx search path (fast equality-driven scan + materialized fallback) and de-duplicates shared helpers into the indexer package. No blockers; one low-risk semantic divergence and a couple of minor observations noted.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The bounded fast path (
candidateMatches) probes non-driver equalities withstore.Has(secondaryKey(tag, value, height, index)), which only proves a secondary key exists at that (height, index) — not that it belongs to the driver candidate's hash. The prior intersection logic matched by hash, so it was robust to orphaned index keys. SinceindexEventsnever deletes stale secondary keys, re-indexing a different tx at the same (height, index) could leave stale keys that make the fast path return a tx the old path rejected. On a canonical/committed chain (height, index) uniquely maps to one tx and reindex reproduces identical keys, so this is edge-case only — but worth a comment or a hash-equality guard. (Raised by Codex.) - Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - Docstring nit:
TestTxSearchBounded's 'cancelled context mid-scan' subtest comments that a cancelled context returns partial results 'without an error'; the accompanyingcountingDB/cancelAfterGetDBhelpers are clear, but consider a brief note thatsearchBounded's partial-return relies onctx.Err()being checked at the top of each iteration (after the append), so the last appended row is retained by design. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| for i := range plan.equalityProbes { | ||
| c := plan.equalityProbes[i] | ||
| ok, err := txi.store.Has(secondaryKey(c.Tag, c.Arg.Value(), height, index)) |
There was a problem hiding this comment.
[suggestion] Has(secondaryKey(c.Tag, c.Arg.Value(), height, index)) only confirms a secondary key exists at this (height, index) — it does not confirm the key maps to the driver candidate's hash. The old intersection path matched by hash, so it rejected candidates whose probe attribute belonged to a different tx. Because indexEvents never deletes stale secondary keys, a re-index that places a different tx at the same (height, index) could leave orphaned event keys that make this probe a false positive. On a finalized chain (height, index) is unique per tx and reindex reproduces identical keys, so this is edge-case only — but consider verifying it.Value()/probe hash identity, or documenting the assumption. (Flagged by Codex, P2.)
Superseded: latest AI review found no blocking issues.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
The blocker I flagged previously (removing the RPC-layer re-sort broke TestTxSearchCapAppliedAfterSort) is now fixed: commit 7926505 updates the mock EventSink to honor SearchOptions (ordering + limit), matching the new contract that the KV indexer returns pre-ordered, pre-capped results. I re-ran go test ./internal/rpc/core/... and go test ./internal/state/indexer/... and both pass.
Extended reasoning...
The only outstanding blocker on this PR was my own prior finding (and seidroid's duplicate) that removing the RPC-layer sort in TxSearch left TestTxSearchCapAppliedAfterSort red, since its mock sink ignored SearchOptions and returned ascending-height results regardless of order_by. Commit 7926505 ("Fix tx_test to respect SearchOptions") updates the mock to honor opts.OrderDesc/opts.Limit, which restores the invariant the test checks. I reproduced this by running the affected test suites directly and confirmed both pass now. No new bugs were found in this run, and the driver-equality preference fix (preferring a tx.height equality as the plan driver) from an earlier round is still in place. I am not approving outright since this is still a nontrivial indexer/RPC change with a history of subtle correctness issues (the driver-selection bug and the sort-removal regression), so a human should give it a final pass, but wanted to record that the last known blocker is resolved.
There was a problem hiding this comment.
Sound, well-tested optimization that bounds the KV tx-search scan path (fast point-probe path + materializing fallback) and de-duplicates the shared bounding helpers into the indexer package. No correctness or security issues found; only minor non-blocking notes.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's second-opinion review reported no material issues (it noted it could not run targeted tests because the Go 1.25.6 toolchain download was network-blocked). The Cursor review file (cursor-review.md) was empty — that pass produced no output.
- REVIEW_GUIDELINES.md was empty, so no repo-specific standards were applied beyond general review criteria.
- Consider adding a targeted test/benchmark asserting the fast path does NOT fully materialize a large match set (the current tests assert correctness and probe-count, but the core memory-bound claim of the PR is only exercised indirectly).
- The unconditional up-front order_by validation in TxSearch is now stricter than before (it previously only ran inside the KV-sink branch). This is more correct and mirrors BlockSearch, but is a slight behavior change worth noting for any non-KV deployment.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // parseHeightIndexFromKey extracts the (height, index) suffix of a secondary | ||
| // (event) key. The tx index encodes each event key as | ||
| // orderedcode(compositeKey, value, height, int64(index)). | ||
| func parseHeightIndexFromKey(key []byte) (int64, uint32, error) { |
There was a problem hiding this comment.
[nit] Nit: parseHeightIndexFromKey duplicates the orderedcode parse in parseValueFromKey (kv.go:808) — both decode the same (compositeKey, value, height, index) layout, differing only in which fields they return. Consider a single parse helper returning all fields to avoid drift if the key format ever changes.
| // Keys under the driver (tag, value) prefix are always well-formed | ||
| // secondary keys, so a parse error is unreachable; skip defensively. | ||
| height, index, err := parseHeightIndexFromKey(it.Key()) | ||
| if err != nil { |
There was a problem hiding this comment.
[nit] Nit: on a parse error here the candidate is silently continued (and similarly res == nil is silently skipped at the Get below). The comment correctly notes the parse error is unreachable for well-formed keys, but a silent skip would mask any real index/primary-store inconsistency. Matches the fallback's tolerance, so non-blocking — just flagging the silent drop.
There was a problem hiding this comment.
The blockers previously raised (driver-preference regression for equality AND tx.height=N queries, and the RPC re-sort removal breaking TestTxSearchCapAppliedAfterSort) are now resolved in commits be4680a/759d1b3/7926505/ce16702 — planBounded now prefers a tx.height equality as the driver, the test mock was updated to honor SearchOptions instead of restoring the sort, and candidateMatches now verifies hash identity on probes rather than relying on Has alone. I re-ran the affected tests (TestTxSearchCapAppliedAfterSort, TestTxSearchBoundedPrefersHeightDriver, TestTxSearchBounded) and the full internal/rpc/core and internal/state/indexer/... suites, all passing. No new bugs found this run. Given this PR rewrites a core query-planning/scan path for tx search (a change that has already required several rounds of subtle correctness fixes), I'd still like a human to sign off before merge rather than relying solely on automated review.
Extended reasoning...
Overview
This PR pushes the result cap and ordering for KV tx search down into the indexer scan path itself, mirroring the block indexer bounding done in the predecessor PR (#3689). It adds a fast path (planBounded/searchBounded) that drives an ordered scan off a single equality condition and point-probes the rest, a fallback path for query shapes that can't be driven this way (CONTAINS/MATCHES/EXISTS, non-height ranges, height-range-only), hoists shared bounding helpers (BoundedCap, HeightInRange, PrefixUpperBound) into the indexer package, and simplifies TxSearch in rpc/core/tx.go to rely on the indexer's ordering instead of re-sorting at the RPC layer.
Security risks
None identified. This is read-path query/indexing logic with no auth, crypto, or external input trust boundary changes — it only affects what order/how many results a tx search RPC call returns.
Level of scrutiny and other factors
This touches a production RPC query path (tx_search) and its underlying KV indexer, which is exactly the kind of critical/complex logic that warrants human sign-off rather than autonomous approval. That caution is reinforced by the review history here: multiple real correctness bugs were found and fixed over several iterations — a driver-selection regression that would have silently reintroduced a full-table-ish scan for "attr = X AND tx.height = N" queries, a test/mock mismatch masking a behavior change, and a hash-identity gap in the fast-path probe (Has alone could false-positive against stale/orphaned secondary keys). All of these are now fixed in the tree as of ce16702, and I re-ran the relevant test suites (internal/rpc/core, internal/state/indexer/...) with everything passing, including the new TestTxSearchBounded and TestTxSearchBoundedPrefersHeightDriver regression tests. There is also a documented, intentional known limitation (PLT-786): the fallback path still fully materializes its intersection before capping for query shapes that can't be driven by an in-order scan — this is called out explicitly as out of scope and tracked separately, not a bug.
Given the density of prior findings on this exact diff and the fact that it changes core query semantics for a widely-used RPC endpoint, I'm deferring rather than approving, even though I found no new issues this run.
2 participants
Summary
Follow-up to #3689 (PLT-748), which pushed the result cap and ordering down
into the KV block indexer but left the tx scan path unbounded, marked
TODO(PLT-748).TxSearchstill fetched the entire match set, sorted it, andonly then applied
MaxTxSearchResultsat the RPC layer — so a broad tx querymaterialized and sorted far more results than the caller would ever see.
This PR bounds the tx scan path the same way the block indexer is bounded, and
de-duplicates the shared bounding helpers into the
indexerpackage so bothindexers reuse one implementation.
What changed
tx/kv) — bounded search:planBounded/searchBounded): eligible when every conditionis an equality (point-probeable) or a
tx.heightrange (evaluable from thecandidate height), with at least one equality to drive the scan. It scans the
driver equality's secondary-index prefix
(tag, value)— which orders by(height, index)— inorder_byorder, point-probes the remainingequalities per candidate via
store.Has(secondaryKey(...)), evaluatesheight ranges from the candidate height, dedups by hash, and stops at
Limit. Memory is bounded by results kept, not by total match cardinality.intersect+collectBounded): queries withCONTAINS/MATCHES/EXISTS, non-height ranges, or only atx.heightrange cannot drive an in-order point-probeable scan — the
tx.heightsecondary index stores the height as a decimal string, so its key order is
not numeric. These materialize the intersection as before, then order by
(height, index)and cap.TxSearch: validatesorder_byup front, pushes the cap and orderinginto the indexer, and keeps the post-sort cap as a safety net for sinks that
ignore the limit (mirrors
BlockSearch). Removes theTODO(PLT-748).BoundedCap,HeightInRange, andPrefixUpperBound(plus themaxBoundedPreallocconst) fromblock/kvintothe
indexerpackage as exported helpers, and updated bothblock/kvandtx/kvto use them.parseHeightIndexFromKeystays tx-local (tx keys carry anindexcomponent; block keys do not).Tests
TestTxSearchBoundedcovers the fast path (equality driver, multi-equalityprobe, equality +
tx.heightrange,tx.heightequality driver) and thefallback path (
tx.height-range-only,CONTAINS), across asc/desc andvarious limits. It also asserts a cancelled context returns partial results
without error, and that the bounded top-N equals the first N of the same query
run unbounded — guarding against divergence between the fast and fallback
paths.
TestTxSearch*suites are unchanged (behavior for the zero-value,unbounded opts is preserved).
Known limitation / follow-up (PLT-786)
The fallback path still fully materializes its intersection before the cap is
applied — this covers
CONTAINS/MATCHES/EXISTS, non-height value ranges, andtx.height-range-only queries. These query shapes can't drive an in-order,point-probeable scan in the current index format: the
tx.heightsecondaryindex stores the height as a decimal string (
orderedcode(TxHeightKey, "N", …)),so its key order is lexicographic, not numeric, and there is no equality to drive
off. The RPC layer still caps the returned set, so this is a transient
memory/CPU bound (scales with match cardinality), not an unbounded result.
Bounding these internally needs a height-ordered secondary index
(
orderedcode(compositeKey, height, value, index)), which is a storage-formatchange (write amplification + reindex/migration on upgrade) and out of scope
here. That work is tracked in PLT-786, which also makes numeric
tx.heightranges seekable for free. This PR intentionally bounds only the equality /
tx.height-point cases that the existing index can serve in order.