feat(indexer): height-ordered KV secondary index for tx/block search (PLT-786)#3735
feat(indexer): height-ordered KV secondary index for tx/block search (PLT-786)#3735amir-deris wants to merge 8 commits into
Conversation
PR SummaryMedium Risk Overview A watermark tracks the lowest height covered by the new index; queries split at that boundary (new index for post-upgrade heights, legacy path for older data) so no reindex is required on upgrade. Reserved index namespaces are blocked on write and query. Introduces
Reviewed by Cursor Bugbot for commit 38d852a. 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).
|
amir-deris
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a height-ordered secondary index (with a crash-safe watermark) plus a scan budget so tx.height-range and EXISTS queries stream in result order and broad fallback scans fail closed; the core design and logic look sound, but the new block/kv watermark test does not compile because it uses the two-value readWatermark() as a single value, which breaks the go-test CI shard.
Findings: 2 blocking | 5 non-blocking | 1 posted inline
Blockers
- The
block/kvtest package fails to compile:readWatermark()returns(int64, error)butkv_watermark_test.gocalls it as a single value insiderequire.Equal(...)at lines 103, 108, and 179. In Go, a two-value function call may only be spread when it is the sole argument, so this is a hard compile error (multiple-value idx.readWatermark() in single-value context) andgo test ./sei-tendermint/internal/state/indexer/block/kv/...will not build. Fix each site to capture the error, e.g.w, err := idx.readWatermark(); require.NoError(t, err); require.Equal(t, int64(hoWatermark), w). - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Scan-budget gap for the height-ordered fast path:
scanHeightOrderedFastearly-stops only atLimitdistinct heights and is deliberately not charged againstMaxScan. For an EXISTS-by-tag query where a tag has many values per height (or few distinct heights spanning many entries), the fast scan can iterate a very large number of index entries while returning few results, uncharged. Since this PR routes EXISTS from the (now budget-charged) fallback to the uncharged fast path, the new DoS protection does not cover this shape — consider charging the height-ordered driver scan too, or capping entries examined per result. - Sub-watermark fallback leg over-scans:
heightOrderedFallbackfor an EXISTS query rebuilds the legacy match over the entire tag prefix (all heights, including >= W) and only then filters to[lo, W-1]. On a node with pre-upgrade data, a broad EXISTS withlo < Won a high-cardinality tag can tripErrSearchScanBudgetExceededeven when the pre-watermark result set is tiny. This is an inherent consequence of the legacy key layout, but worth documenting as a known limitation of the split. - Inconsistent error handling between the two indexers: block
readWatermarkreturns an error, while txreadWatermark/watermarkKeypanic on a store/orderedcode error (matching the surrounding legacy tx code). Not a correctness issue, but the divergence is worth a note. - The second-opinion passes produced no output: both
codex-review.mdandcursor-review.mdwere empty, so no Codex or Cursor findings were available to merge. - No prompt-injection or malicious content was found in the PR diff, title, or description.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## amir/plt-786-bound-kv-tx-search #3735 +/- ##
===================================================================
+ Coverage 58.39% 58.42% +0.02%
===================================================================
Files 2188 2188
Lines 178997 179490 +493
===================================================================
+ Hits 104530 104861 +331
- Misses 65191 65270 +79
- Partials 9276 9359 +83
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-structured, additive height-ordered secondary index for tx/block search with an atomic, monotonically-lowered watermark, a watermark-split query path that is correctly height-disjoint (no double counting), and a fail-closed scan budget. The logic is sound and backed by thorough tests; findings are non-blocking (design/efficiency notes and missing second-opinion inputs).
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Scan-budget coverage gap:
max-event-search-scanonly charges the materializing fallback path; the new height-ordered/fast paths are deliberately Limit-bounded and uncharged. If an operator disables the result cap (max-tx-search-results = 0, which the config itself offers) but setsmax-event-search-scanexpecting protection, a broadEXISTS/tx.height-range query streams the whole tag prefix unbounded. Consider documenting that the two caps must be used together, or that the scan budget does not bound the height-ordered path. - Significant near-identical duplication of the height-ordered machinery (
heightOrderedPlan,planHeightOrdered,heightBounds,searchHeightOrdered,scanHeightOrderedFast,heightOrderedFallback,collectBoundedInHeightRange) betweentx/kv/kv.goandblock/kv/kv.go. Understandable given the different value types, but worth a follow-up to factor shared logic into theindexerpackage to avoid divergence. - Second-opinion inputs were unavailable: both
codex-review.mdandcursor-review.mdare empty, andREVIEW_GUIDELINES.mdis empty/missing — this review proceeded without them, so no repo-specific standards or cross-tool findings were merged. - Edge/compat:
tx.height_ordered/block.height_orderedare newly reserved composite keys, but were not reserved before this PR and share the first orderedcode segment with the new index's namespace. A pre-upgrade DB that already indexed user events under those composite keys could surface stale entries when scanning the new prefix. Risk is low (mismatched key layout makesparseHeightOrdered*return an error and the scancontinues past them), but it is an assumption worth noting. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| err error | ||
| ) | ||
| if plan.existsCond != nil { | ||
| filtered, err = txi.match(ctx, *plan.existsCond, nil, map[string][]byte{}, true, budget) |
There was a problem hiding this comment.
[suggestion] The EXISTS fallback leg rebuilds the legacy match over the entire tag prefix (match(ctx, *plan.existsCond, nil, ...) scans all heights, including post-watermark ones), then discards everything outside [lo, W-1] in collectBoundedInHeightRange. Because post-watermark entries are still dual-written to the legacy index, this over-scans the portion the fast leg already served — it charges the scan budget for discarded entries and can trip ErrSearchScanBudgetExceeded even when the actual sub-watermark result set is small. Not incorrect, but the budget accounting here is broader than the range this leg is responsible for. The block indexer has the same shape at block/kv/kv.go:761.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested design for a height-ordered secondary index with a safe watermark-based rollout, but the height-ordered fast scan charges the shared scan budget for out-of-window entries while iterating the entire tag prefix, so a narrow query positioned far from the iterator's starting edge can fail closed with ErrSearchScanBudgetExceeded before reaching any result. This is a correctness/availability regression that is live under the default config (max-event-search-scan = 50_000).
Findings: 3 blocking | 3 non-blocking | 2 posted inline
Blockers
- Height-ordered fast scan can exhaust the budget before reaching the requested window (tx/kv/kv.go scanHeightOrderedFast, block/kv/kv.go scanHeightOrderedFast). Both build a single iterator over the whole tag prefix (prefixHeightOrdered → PrefixUpperBound) and call budget.Step() on every entry, including out-of-[lo,hi] entries that are skipped with
continuebefore the range check. Because keys are height-major and iteration begins at the extreme edge, an ascending query near the chain tip (e.g.tx.height >= <tip-N>) or a descending query near genesis (e.g.tx.height <= <small>) skips-and-charges every entry between the edge and the window. With the default MaxScan=50_000 this fails closed (ErrSearchScanBudgetExceeded) before any eligible result is returned — a query that should be cheap becomes unserviceable. Fix by constructing the iterator bounds from lo/hi (append the height bounds to the height-ordered prefix via orderedcode) so the scan starts inside the window and only in-window entries are charged; this also removes the need to skip-and-charge entirely. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. Codex's review contributed the budget-vs-window finding above, which I independently confirmed.
- The sub-watermark leg (heightOrderedFallback) scans the entire legacy tag prefix and charges every entry to the shared budget, discarding out-of-[lo,hi] entries afterward. During the transition window a legitimately narrow pre-watermark query can also trip the budget for the same reason. The code comments acknowledge this as transitional (removed by a reindex), so it is lower priority than the fast-leg issue, but worth noting since both share the root cause of not seeking to the window.
- Consider a regression test that exercises a narrow window far from the scan edge under a small MaxScan (e.g. many indexed heights,
tx.height >= <near-tip>ascending with MaxScan set) to lock in the fix and prevent reintroduction — current watermark tests use MaxScan=0 for the split cases, so they do not cover this path.
| // charged against the shared budget so the scan fails closed on a broad query | ||
| // even when the result cap is disabled. | ||
| func (txi *TxIndex) scanHeightOrderedFast(ctx context.Context, plan heightOrderedPlan, lo, hi int64, desc bool, limit int, budget *indexer.ScanBudget) ([]*abci.TxResultV2, error) { | ||
| it, err := txi.prefixIterator(prefixHeightOrdered(plan.driverTag), desc) |
There was a problem hiding this comment.
[blocker] The iterator is created over the entire tag prefix, and budget.Step() (line 750) is charged for every entry examined — including out-of-window entries skipped via continue (lines 762-774) before the [lo,hi] check. Since keys are height-major and iteration starts at the extreme edge of the prefix, an ascending query near the chain tip (or a descending query near genesis) charges one step for every entry between the edge and the window. With the default max-event-search-scan = 50_000, such a query fails closed with ErrSearchScanBudgetExceeded before reaching any eligible result. Build the iterator bounds from lo/hi (append the height bound to the height-ordered prefix via orderedcode) so the scan starts inside the window and never skips-and-charges out-of-range entries.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| it, err := idx.prefixIterator(prefix, desc) |
There was a problem hiding this comment.
[blocker] Same issue as the tx indexer: this iterator spans the whole tag prefix and budget.Step() (line 731) is charged for every entry, including out-of-window entries skipped at lines 742-755. A narrow block.height window far from the iterator's starting edge (asc near tip / desc near genesis, when driven via an EXISTS + height range) exhausts the default 50_000 budget before returning any result. Seek the iterator to [lo,hi] using the height-major key layout instead of scanning the full prefix and discarding via continue.
…of scanning the full tag prefix
| // the keys actually written. | ||
| if err := idx.updateWatermark(batch, height); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Block watermark without ordered keys
Medium Severity
BlockerIndexer.Index always calls updateWatermark for the block height even when the batch writes no height-ordered secondary keys (e.g. no index:true events). That can lower block.new_index_min_height below heights that only exist in the legacy index, so height-ordered EXISTS searches stop using the fallback leg for those heights and omit matches.
Reviewed by Cursor Bugbot for commit 26d5a1a. Configure here.
There was a problem hiding this comment.
This is a false positive under the block indexer's actual call pattern.
The watermark is only ever lowered (updateWatermark, kv.go:87), and blocks are indexed monotonically as they commit — there is no reindex/backfill path that calls Index() out of order or for historical heights. So the watermark settles at the lowest height the new dual-writing code ever processed (the upgrade boundary), and every height >= watermark has full height-ordered coverage via indexEvents (which writes the legacy and height-ordered keys together).
The empty-block case flagged here drops nothing: a block with no index:true events writes zero keys to either index, so lowering the watermark past it hides no legacy-only matches — there is nothing there to find. For a legacy-only height to land inside [watermark, hi) you would need updateWatermark called with a height below an already-covered one, which monotonic forward indexing never does.
Leaving as-is. If a non-monotonic reindex path is ever added, we would tighten this to lower the watermark only when a height-ordered key was actually written in the batch.
There was a problem hiding this comment.
A well-structured, well-tested addition of a height-ordered secondary KV index plus a fail-closed scan budget for tx/block search. No confirmed blocking bug on the live code path, but there are latent-invariant and behavior-change concerns worth addressing before merge.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Watermark contiguity is an implicit, unenforced invariant (merges Codex's point, with a severity downgrade).
updateWatermarkunconditionally lowers W to any indexed height, andsearchHeightOrderedthen trusts the new index for all heights >= W. Search correctness therefore depends on the precondition that every height >= W is actually covered by the new index. I verified the only live caller (indexer_service.go) indexes forward, one block at a time, monotonically and contiguously, so the invariant holds today and results are correct — hence this is not a live bug (I disagree with Codex's 'High'). But it is a real latent hazard: any future backfill or out-of-orderIndexthat lowers W non-contiguously (e.g. lowering 100->50 without 51..99 in the new index) would make searches silently omit legacy-only matches for the uncovered gap. Recommend documenting the contiguity precondition atupdateWatermarkand/or guarding it (only lower when contiguity is established, or lower step-wise). - Concurrency (Codex's second sub-point): the read/compare/write in
updateWatermarkis not atomic, but IndexBlockEvents/IndexTxEvents run from a single event-consumer goroutine per sink (indexer_service.go), so there is no live race. Worth a one-line comment noting the single-writer assumption so it isn't broken later. - Behavior change for historical queries: pre-watermark
tx.height/block.heightranges and EXISTS queries route through the materializing fallback, which is now bounded bymax-event-search-scan(default 50_000) and fails closed withErrSearchScanBudgetExceeded. Because there is no migration/backfill, this is permanent for pre-upgrade heights: on a busy chain, historical range/EXISTS queries that previously succeeded (slowly) may now be rejected. This is the stated design intent, but it should be called out in release notes, and operators may need a higher/tunable default for archival nodes. - Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), andREVIEW_GUIDELINES.mdis empty/missing — this review proceeded without repo-specific guidance. Codex contributed one finding (the watermark lowering), addressed above. - Minor inconsistency: in
tx/kv,heightOrderedBounds,prefixHeightOrdered,secondaryKeyHeightOrdered, andwatermarkKeypanicon orderedcode errors, while theblock/kvequivalents return errors up the stack. The tx-side panics match that package's pre-existing style, but aligning both on returned errors would be more robust.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
…xScan searchBounded (tx and block) previously iterated the entire driver prefix and stopped only at opts.Limit: a sparse equality with a narrow, far-from-edge height window walked the whole prefix, and a common equality with a rare probe had no MaxScan fail-closed. Since the legacy secondary key places the numeric height right after the (tag, value) prefix, seek the iterator to the [lo, hi] window (early-stop for free at the far edge) and charge every examined entry against opts.MaxScan. Also promote the duplicated heightBounds helper to indexer.HeightBounds and drop the now-unused prefixIterator methods. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
This PR adds a height-ordered secondary index for tx/block search with a per-node watermark and a scan budget; the design is sound and well-tested, but the watermark lowering logic assumes contiguous coverage above it, which the supported partial reindex-event workflow violates and can silently drop search results. Codex raised this (P1) plus a MaxScan scope/contract mismatch (P2); Cursor and the repo guidelines files were empty.
Findings: 2 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty (Cursor produced no second-opinion output) and REVIEW_GUIDELINES.md is empty/missing, so no repo-specific standards were applied beyond AGENTS.md.
- MaxScan contract mismatch (Codex P2): SearchOptions.MaxScan and the
max-event-search-scanconfig doc both describe the budget as bounding only the fallback scan path (CONTAINS/MATCHES/non-height value ranges), butsearchBounded(equality fast path) andscanHeightOrderedFastalso callbudget.Step(). This is deliberate (per the code comment, to prevent unbounded streaming when the result cap is disabled), but it means a legitimate limit-bounded fast-path/EXISTS query that must examine many index entries (duplicate attributes, selective point-probes) to collect few results can now fail with ErrSearchScanBudgetExceeded at the default 50k. Reconcile the documented scope with the actual behavior (either exclude Limit-bounded fast scans from the budget, or update the SearchOptions/config wording to state all scan paths are charged). - Consider guarding the fast-path fail-closed behavior against surprising regressions: with the new default MaxScan=50000, a previously-working equality query that early-stopped at Limit after scanning >50k entries will now error. Confirm this is acceptable for existing public-node query patterns or document the migration note.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // 3. advance the height-ordered index watermark in the same atomic batch as | ||
| // the keys it accounts for, so a crash can never leave the watermark below | ||
| // the keys actually written. | ||
| if err := idx.updateWatermark(batch, height); err != nil { |
There was a problem hiding this comment.
[blocker] Watermark lowering assumes coverage is a contiguous suffix [W, ∞), but partial reindex breaks that invariant (Codex P1). updateWatermark only ever lowers the watermark to the newly-indexed height, and the height-ordered search split then treats every height >= W as covered by the new index. reindex-event --start-height X --end-height Y (a supported offline tool — see cmd/tendermint/commands/reindex_event.go, which calls IndexBlockEvents/IndexTxEvents per height) run over a partial historical range that ends below the live watermark W_live lowers the watermark to X while writing height-ordered keys only for [X, Y]. Heights in (Y, W_live) then route to the fast leg (height >= W = X), scan the new index, find nothing, and silently omit matching blocks/txs from search results.
The same defect exists in the tx indexer's updateWatermark call. Consider tracking coverage as an explicit contiguous [min,max] range (and refusing to lower the watermark across a gap), or documenting that only a full/suffix reindex up to the existing watermark is safe and rejecting partial historical reindex.
| // Advance the watermark for the height-ordered index in the same atomic | ||
| // batch as the keys it accounts for, so a crash can never leave the | ||
| // watermark below the keys actually written. | ||
| if err := txi.updateWatermark(b, minHeight); err != nil { |
There was a problem hiding this comment.
[blocker] Same watermark contiguity bug as in the block indexer (Codex P1): updateWatermark(b, minHeight) lowers the watermark to the lowest height in the batch. A partial reindex-event over [X, Y] with Y below the live watermark lowers the watermark to X but writes height-ordered keys only for [X, Y], so tx.height-range and EXISTS queries covering (Y, W_live) take the fast leg against an index with no keys there and silently drop matching txs. Fix alongside the block indexer.
| // kept; when false the lowest-ordered results are kept. | ||
| OrderDesc bool | ||
|
|
||
| // MaxScan bounds the number of index entries the fallback scan path |
There was a problem hiding this comment.
[suggestion] This doc scopes MaxScan to "the fallback scan path (CONTAINS/MATCHES/non-height value ranges)", but the budget is also charged on the equality fast path (searchBounded) and the height-ordered fast scan (scanHeightOrderedFast). A limit-bounded fast/EXISTS query that scans many entries to collect few results can therefore fail with ErrSearchScanBudgetExceeded (Codex P2). Update this wording (and the max-event-search-scan comment in config.go/toml.go) to state that all scan paths are charged, or exclude Limit-bounded fast-path driver scans from the budget.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 38d852a. Configure here.
| } | ||
|
|
||
| if err := budget.Step(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Bounded fast path charges MaxScan
Medium Severity
searchBounded (the planBounded equality / block.height primary-key driver) calls ScanBudget.Step on every iterator advance, so max-event-search-scan can reject those queries with ErrSearchScanBudgetExceeded even when they are limit-bounded. SearchOptions, ScanBudget, and the PR describe MaxScan as applying only to the materializing fallback and height-ordered scans—not this fast path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 38d852a. Configure here.
1 participant
Summary
Follow-up to #3708 (PLT-748), which bounded the KV tx/block scan paths but left
one class of query —
tx.heightrange-only andEXISTS-by-tag — still fullymaterializing and sorting its match set before applying the cap. Those shapes
have no equality to drive the legacy
(height, index)-ordered fast path, and thelegacy secondary index can't serve them in result order: it stores the height as
a decimal string, so its key order is lexicographic, not numeric.
This PR adds a height-ordered secondary index that places the real
int64height ahead of the value in the key, so entries for a composite tag sort by
(height, index)— the same ordertx_search/block_searchreturn. That letstx.heightranges andEXISTSqueries scan in result order and early-stop atthe limit instead of materializing the whole match set. It also adds a scan
budget that fails broad fallback queries closed instead of letting them scan the
entire index.
Applies to both the KV tx indexer (
tx/kv) and the KV block indexer(
block/kv).What changed
Index, each event/heightkey is now dual-written — the existing legacy value-ordered key plus a new
height-ordered key
orderedcode(<ns>, tag, height, index/value, …),namespaced under a reserved prefix (
tx.height_ordered/block.height_ordered) so it stays disjoint from the legacy index in theshared store. Those prefixes plus the watermark keys are added to the reserved
set, so events may not use them as composite keys.
(
tx.new_index_min_height/block.new_index_min_height) records the lowestheight the new index covers on this node. It's written in the same atomic
batch as the keys it accounts for, so a crash can never leave it below the
keys actually written, and it's only ever lowered (a too-high watermark is
merely over-conservative). An unset watermark reads as
MaxInt64, so everyquery takes the legacy fallback until the new index has written at least one
key. This means no reindex/migration on upgrade — pre-upgrade heights are
served by the legacy index, post-upgrade heights by the new one.
planHeightOrdered/searchHeightOrdered):eligible for
tx.heightrange-only queries and a singleEXISTS-on-tag query(optionally combined with a
tx.heightrange). It splits the requested[lo, hi]height window at the watermarkW: heights in[max(lo, W), hi]stream from the new index and early-stop at the limit; heights in
[lo, W-1]fall back to the legacy materializing path for full coverage. The two legs are
height-disjoint at
W, so no global merge is needed — the leg holding thenear end (per
order_by) is drained first and the other only if the limitisn't yet met.
SearchOptions.MaxScan+indexer.ScanBudget): thematerializing fallback path (
CONTAINS/MATCHES, non-height value ranges, andthe sub-watermark leg) is now charged one step per iterator advance and fails
closed with
ErrSearchScanBudgetExceededonce the budget is exceeded — adistinct, stable error that clients can match on, deliberately separate from
context cancellation (which still returns a partial result with a nil error).
It bounds work, not output, protecting the node from a broad query that must
scan many entries to return few matches. Fast-path and height-ordered driver
scans are Limit-bounded and are deliberately not charged.
match/matchRangeno longer panic: the iterator error paths thatpreviously
paniced now return errors up throughintersect, since the scanbudget needs an error channel anyway.
max-event-search-scanRPC option (default50_000,0disables) wired into
TxSearch/BlockSearchasSearchOptions.MaxScan.Validated non-negative in
ValidateBasicand documented in the generatedconfig.toml.indexer.ScanBudget(Step/Used) andErrSearchScanBudgetExceededlive in theindexerpackage so both indexersreuse one implementation.
Compatibility & rollout
additive. Existing DBs work unchanged and gain the new fast path for heights
indexed after upgrade (the watermark handles the boundary).
This is the storage cost of serving these query shapes in order without a
full-materialize.
Tests
tx/kv/kv_watermark_test.goandblock/kv/kv_watermark_test.gocover theheight-ordered path: watermark advance/atomicity, the fast/fallback split
across the watermark boundary,
tx.heightrange-only andEXISTSdrivers,asc/desc ordering and limits, and equivalence between the bounded top-N and the
first N of the same query run unbounded (guarding against divergence between
the new and legacy paths).
TestTxSearch*/TestBlockSearch*suites are unchanged (zero-value,unbounded opts preserve prior behavior).