Bound /store/*/subspace ABCI queries - #4009
Conversation
Reject empty prefixes, cap pair/byte accumulation during iteration, and limit concurrent SS fast-path scans so unauthenticated callers cannot OOM a node with wide subspace queries. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview On the SS fast path, a separate Three new Reviewed by Cursor Bugbot for commit 172be6a. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
==========================================
- Coverage 61.26% 60.29% -0.97%
==========================================
Files 2153 2055 -98
Lines 188485 176850 -11635
==========================================
- Hits 115477 106640 -8837
+ Misses 62265 60430 -1835
+ Partials 10743 9780 -963
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.
Solid, well-tested DoS hardening of the storev2 /subspace ABCI path: pair/byte caps, an empty-prefix rejection, an SS-path semaphore, and complete config-characterization wiring (flags, defaults, app.toml template, both goldens, and the fuzz seed re-indexing all line up). Three non-blocking issues: the scan ignores the repo's context-aware iterator helper, the empty-prefix guard is duplicated at callers instead of the shared choke point, and the 0-means-default cap semantics are undocumented and contradict the neighbouring rate-limit key.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new in-flight semaphore only covers the SS fast path. Since
RequireProof("/subspace")is false,/subspacereaches the commitment path wheneverrs.ssStore == nil(SS disabled), and that path gets no concurrency limit — only the pair/byte caps. Either extendtryAcquireSubspaceQueryPermitto cover it, or state the SS-only scope in a comment onsubspaceQuerySemso the gap reads as deliberate. - [suggestion] This is a fail-closed behavior change on a public query surface: a
/store/{name}/subspacequery that previously returned 5k pairs now returnsErrSubspaceCapExceededinstead of data. Worth an explicit release note so operators running indexers or tooling on wide prefixes know to narrow their prefixes or raisesc-subspace-max-pairs/sc-subspace-max-bytesbefore upgrading. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
Thread request context through IteratorOn on the SS path, centralize the empty-prefix guard in ScanSubspace, and document <=0 default semantics for subspace limits in app.toml. Co-authored-by: Cursor <cursoragent@cursor.com>
Cancelled or timed-out /subspace queries were panicking on the SS path because state.Store fatally handles iterator-construction failures. Co-authored-by: Cursor <cursoragent@cursor.com>
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 260ccf0. Configure here.
An SS iterator that stops mid-prefix must surface its error instead of marshaling a truncated result as success. Register the three subspace keys in config characterization and document that the in-flight semaphore applies only on the SS fast path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
Well-scoped DoS hardening of the storev2 /subspace ABCI path: the empty-prefix guard now lives in the single ScanSubspace choke point, iteration is context-aware via IteratorOn, aborted scans are no longer marshaled as successful truncated results, and the config wiring (flags, defaults, template, both goldens, fuzz seed re-indexing, key-count comment) is complete and internally consistent. Two non-blocking points: the pair/byte defaults are now defined twice with nothing tying them together, and no test proves an operator-configured non-default cap actually reaches the SS query path.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| ) | ||
|
|
||
| const ( | ||
| DefaultMaxSubspacePairs = 1_000 |
There was a problem hiding this comment.
[suggestion] DefaultMaxSubspacePairs/DefaultMaxSubspaceBytes duplicate DefaultSCSubspaceMaxPairs/DefaultSCSubspaceMaxBytes in sei-db/config/sc_config.go, and nothing ties the two pairs together.
That matters because effective() is the runtime authority for a <= 0 value, not the config package. The app.toml comment this PR adds ("<=0 resolves to default 1000") and the CHANGELOG upgrade note both promise a specific number; if someone later retunes DefaultSCSubspaceMaxPairs, toml_test.go will be updated to match the new template output while a node with sc-subspace-max-pairs = 0 silently keeps resolving to the stale value here — and no test compares the two.
sei-db/config has no dependency on sei-cosmos, so sei-cosmos/storev2/query can import it (sei-cosmos/storev2/state already imports sei-db/db_engine/types) and define these as = config.DefaultSCSubspaceMaxPairs / = config.DefaultSCSubspaceMaxBytes, leaving one source for the documented number.
| histProofSem: make(chan struct{}, maxInFlight), | ||
| histProofLimiter: limiter, | ||
| subspaceQuerySem: make(chan struct{}, subspaceMaxInFlight), | ||
| subspaceLimits: query.Limits{ |
There was a problem hiding this comment.
[suggestion] This is the only place SubspaceMaxPairs/SubspaceMaxBytes cross from scConfig into the query path, and no test covers it: the rootmulti tests exercise the semaphore, the empty prefix, and a narrow-prefix success, but none sets a non-default cap and observes it take effect.
Because Limits.effective() substitutes the package defaults for any non-positive field, a mis-wiring here (dropping the fields, passing query.Limits{}, or swapping pairs/bytes in a future edit) leaves every existing test green — the scans just fall back to 1000 / 4 MiB. A rootmulti-level test in the shape of TestQuery_SubspaceNarrowPrefixAndKeyUnaffected with scCfg.SubspaceMaxPairs = 1, two seeded keys, and an assertion on query.IsCapExceededResponse(resp) would pin the wiring and cover the reason=cap_exceeded metric branch at the same time.

Summary
Closes PLT-799. The
/store/{name}/subspaceABCI path on storev2 SS nodes iterated an entire KV prefix into memory with no empty-data guard, no result cap, and no concurrency limit.This PR makes
/subspacefail closed on unbounded work:storev2/query.ScanSubspacebefore any iteration (covers state and commitment paths).storev2/queryhelper: stop once pair count or accumulated key+value bytes exceed configured limits (defaults: 1,000 pairs / 4 MiB). Returns typedstore.ErrSubspaceCapExceeded— no silent truncation.IteratorOnso SS MVCC skip loops honor request cancellation and deadlines (not just between yielded pairs).subspaceQuerySem, separate fromhistProofSem): saturated callers getErrConflict, same as historical proof.subspace_query_rejected{reason=semaphore|cap_exceeded}for operator visibility.Query-only change — no AppHash or chain-upgrade impact.
Operator note: wide
/subspacequeries that previously returned large result sets may now fail withErrSubspaceCapExceeded. Narrow prefixes or raisesc-subspace-max-pairs/sc-subspace-max-bytesbefore upgrading if indexers or tooling depend on wide scans.New config (
[state-commit])sc-subspace-query-max-inflight<=0resolves to defaultsc-subspace-max-pairs<=0resolves to default; no unlimited settingsc-subspace-max-bytes<=0resolves to default; no unlimited settingWired through
parseSCConfigs,app.tomltemplate, and config characterization goldens.Test plan
go test ./sei-cosmos/storev2/query/...— pair/byte caps, ctx cancel, typed error + ABCI code preservationgo test ./sei-cosmos/storev2/commitment/...— empty prefix rejected, narrow prefix succeeds,/keyunaffectedgo test ./sei-cosmos/storev2/rootmulti/...— semaphore saturation, integration paths, empty prefixgo test ./app/ -run 'TestDefaultsMatchTheRecordedValues|TestKeyNamesMatchTheRecordedNames|FuzzParseSCConfigs'go test ./sei-db/config/...