Restore LCD pagination while preserving v6.6 precompile semantics - #3867
Conversation
The query hardening introduced in #3494 (PLT-361) broke previously valid LCD/gRPC pagination requests: - limits above 1,000 and offsets above 10,000 are rejected with InvalidArgument instead of being served - count_total=true fails with a "scanned more than 10,000 entries" error whenever the underlying store is larger than the scan cap, so totals cannot be retrieved from large stores at all - filtered queries (e.g. validators by status) fail mid-pagination when the filter has to scan more than 10,000 entries to fill a page, leaving no way to enumerate sparse result sets by offset Clients that paginate with large limits, follow deep offsets, or rely on pagination.total regressed against every release prior to v6.6. Restore the pre-v6.6 pagination contract for all valid uint64 inputs: - accept any uint64 limit and offset; remove MaxScanLimit/MaxOffset, the 1,000-per-page MaxLimit cap, and the scan-cutoff errors in Paginate, FilteredPaginate and GenericFilteredPaginate - compute the page end with uint64 saturation (paginationEnd) so offset+limit can no longer wrap around on adversarial inputs - preserve the first next_key found while a count_total scan walks the remainder of the store, instead of overwriting it on every subsequent iteration (long-standing upstream bug) - GetBlockWithTxs caps its slice pre-allocation at the number of txs actually in the block rather than trusting the requested limit - CollectAllTotalSupply pages the supply store at a fixed page size of 1,000 so genesis export keeps bounded per-page memory Deliberately NOT restored: upstream's implicit count_total=true when limit is omitted. EVM precompiles (precompiles/staking) invoke these query handlers during transaction execution with Limit: 0, and their store reads are metered against the transaction's gas. An implicit full-store count would change GasUsed (LastResultsHash) and gas refunds (AppHash) across versions. For the same reason this change is consensus-safe: on every code path reachable from block execution the paginators iterate exactly as before (page requests with limit 0->100, offset 0, count_total false break at the same iteration), and all remaining behavioural changes are confined to ABCI Query handlers, which are committed to neither AppHash nor LastResultsHash. Regression coverage: uint64-max limits, offsets past 10,000, accurate count_total on stores larger than 10,000 entries, sparse filters over large stores, next_key preservation, and multi-page supply collection.
|
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 #3867 +/- ##
==========================================
- Coverage 61.74% 60.87% -0.87%
==========================================
Files 2381 2287 -94
Lines 201667 191186 -10481
==========================================
- Hits 124513 116392 -8121
+ Misses 66074 64547 -1527
+ Partials 11080 10247 -833
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
changed the title
PR SummaryHigh Risk Overview Pagination (ABCI / LCD / gRPC): Removes tight Dual code paths: Adds Reviewed by Cursor Bugbot for commit 9540a22. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f19d52f. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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.
The pagination repair and the V66 gating are well-targeted (every FilteredPaginate/GenericFilteredPaginate call site reachable from a precompile is converted, and plain Paginate's key-branch never had a scan cap so it correctly needs no variant), but the IsABCIQuery discriminator also flips EVM simulation/tracing contexts onto the repaired paginator, so eth_call/eth_estimateGas/debug_trace* at a historical height will diverge from real execution — and the V66 variants silently dropped the v6.6 limit/offset validation. There is also zero test coverage for the V66 paginators or the branch selection.
Findings: 3 blocking | 7 non-blocking | 4 posted inline
Blockers
- No test coverage for the consensus-critical half of this change. Every scan-limit test was deleted and replaced with unrestricted-behavior tests; there is not a single test exercising
FilteredPaginateV66,GenericFilteredPaginateV66, or thectx.IsABCIQuery()branch selection in the six converted keepers (grep -rn "V66\|IsABCIQuery" --include=*_test.goreturns nothing). Given the PR's own framing — that getting this wrong produces AppHash/LastResultsHash divergence — a refactor could delete the V66 branch and the suite would stay green. Please add: (a) direct tests that the V66 variants still return thescanned more than 10000 entrieserror on both the key-based and offset-based paths, and (b) a keeper-level test asserting an ABCI-query context takes the unrestricted path while a plain execution context takes the V66 path. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Both second-opinion passes produced no output:
codex-review.mdandcursor-review.mdare empty files. All findings here are from this pass alone. - Security/DoS regression on public endpoints is intentional but unmitigated.
CreateQueryContextbuilds the context with an infinite gas meter, so the repairedPaginate/FilteredPaginatenow allow an unauthenticated LCD/gRPC caller to force a full-store walk (count_total: true,offsetin the billions, or a sparse filter that never fills the page and scans to the end of the store looking fornext_key). That is precisely what #3494 was hardening against. Consider a node-operator-configurable scan cap or a query-context gas meter/timeout rather than removing the bound outright, and note the operational exposure in the release notes. Paginatehas no V66 variant. That is correct today only because its key-based branch never had a scan cap and every precompile passes a key-onlyPageRequestwithLimit/Offset/CountTotalunset (verified acrossprecompiles/, including thelegacy/v5xx–v66copies, which all dispatch into these same keepers). This is load-bearing but undocumented — a future precompile that setsLimitorCountTotalon aPaginate-backed handler (stakingValidatorUnbondingDelegations/DelegatorDelegations/Redelegations, govVotes/Deposits, feegrantAllowances, bankSpendableBalances/TotalSupply) would silently change consensus. Worth a comment inpagination.goplus a test asserting precompile page requests stay key-only.- Inconsistent next-key condition: the filtered paginators were changed to
numHits > endwith anextKey == nilguard, butPaginatestill usescount == end+1(pagination.go:128). Both are correct since the counters increment by one, but the asymmetry invites a future reader to "fix" one of them. Consider aligning them. CollectAllTotalSupply's page size moved fromquery.MaxLimitto the newsupplyPageSize— both 1,000, so no behavior change. Good, and the keeper_test comment update is accurate.- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| ).WithMinGasPrices(app.minGasPrices).WithBlockHeight(height) | ||
| ).WithMinGasPrices(app.minGasPrices). | ||
| WithBlockHeight(height). | ||
| WithIsABCIQuery(true) |
There was a problem hiding this comment.
[blocker] CreateQueryContext is also the context source for EVM simulation and tracing, so this flag makes precompiles behave differently under eth_call than under real execution.
app.RPCContextProvider(i) (app/app.go) returns GetCheckCtx() for LatestCtxHeight — abciQuery == false → V66 paginator — but calls CreateQueryContext(i, false) for any explicit height → abciQuery == true → the unrestricted paginator. evmrpc/simulate.go uses that provider for eth_call/eth_estimateGas at a block number (sdkCtx = b.ctxProvider(tmBlock.Block.Height)) and for debug_trace* replay. x/evm's Querier.StaticCall is on the same path.
Concrete divergence: staking ValidatorDelegations filters by validator over the whole delegator-keyed DelegationKey store, so on mainnet it can easily scan >10,000 entries. In a real tx the precompile call errors out (V66 MaxScanLimit); replayed under debug_traceTransaction or queried via eth_call at that same height it now returns results. Same call also behaves differently at latest vs. an explicit height. That breaks trace fidelity and makes eth_estimateGas optimistic relative to execution.
Suggested fix: keep the flag narrow to non-EVM queries — either select on ctx.IsABCIQuery() && !ctx.IsEVM() at the six keeper call sites, or append .WithIsABCIQuery(false) in RPCContextProvider after the WithIsEVM(true).WithTraceMode(true) chain.
| return nil, err | ||
| } | ||
|
|
||
| // Note: unlike upstream cosmos-sdk, limit == 0 must NOT implicitly enable |
There was a problem hiding this comment.
[blocker] The VerifyPaginationOffset / VerifyPaginationLimit calls were removed unconditionally rather than gated on enforceV66ScanLimit, so FilteredPaginateV66 is not actually v6.6-equivalent.
Under released v6.6 a request with Limit > 1_000 or Offset > 10_000 returned InvalidArgument here, before any iteration. The V66 variants now accept it. Related: the phase-1 guard below became paginationEnd(offset, MaxScanLimit), which saturates at MaxUint64 for a large offset and therefore never fires — in v6.6 such an offset was rejected outright, so this is a second silent difference on the same path.
Not reachable today (every precompile passes a key-only PageRequest with Limit/Offset unset), but the function's own doc comment promises exact v6.6 preservation for consensus execution, and this is a cheap invariant to hold by construction: move both Verify* calls under if enforceV66ScanLimit, and use offset + MaxScanLimit (unsaturated) in the V66 branch.
| const MaxLimit = uint64(1_000) | ||
| // MaxLimit is the maximum limit the paginate function can handle | ||
| // which equals the maximum value that can be stored in uint64 | ||
| const MaxLimit = uint64(math.MaxUint64) |
There was a problem hiding this comment.
[suggestion] MaxLimit = math.MaxUint64 makes the constant a no-op, which quietly weakens the tests that still reference it: TestGetPaginatedTotalSupplyMaxLimit, keeper/genesis_test.go:123, and filtered_pagination_test.go:46 now assert "unbounded works" rather than a boundary. Since VerifyPaginationLimit is gone and nothing enforces it any more, consider deleting MaxLimit outright (forcing call sites to state their own intent, as supplyPageSize now does) instead of leaving a public constant that reads like a cap but isn't one.
| } | ||
| // Cap the allocation by the number of txs actually in the block so a huge | ||
| // user-supplied limit cannot trigger a huge allocation. | ||
| txs := make([]*txtypes.Tx, 0, min(limit, blockTxsLn)) |
There was a problem hiding this comment.
[nit] The cap is the right fix. Minor: it doesn't account for offset, so a request with offset = blockTxsLn - 1, limit = 10_000 still preallocates blockTxsLn entries. min(limit, blockTxsLn-offset) is exact and offset < blockTxsLn is already guaranteed by the check above.
masih
added
the
ai: skip-review
accepting the risk of missing tests
There was a problem hiding this comment.
Not approving — this PR restructures ABCI query context routing and EVM precompile pagination semantics for AppHash safety, which warrants human review regardless of outstanding findings.
Since my last review, commit ae04df3 ("Fix simulation path by explicit flag") addresses the blocker I and another reviewer raised: CreateQueryContext no longer sets abciQuery=true unconditionally, so EVM simulation/tracing paths (RPCContextProvider, eth_call, debug_trace*) now correctly retain the v6.6-capped paginators instead of diverging from consensus behavior. The flag is now set only at the two genuine ABCI-query entry points (handleQueryGRPC, the gRPC server interceptor). I verified this against the current tree and confirmed sei-cosmos/app build cleanly.
One smaller, previously-flagged item remains open: FilteredPaginateV66/GenericFilteredPaginateV66 still don't gate VerifyPaginationOffset/VerifyPaginationLimit behind enforceV66ScanLimit, so they aren't byte-for-byte v6.6-equivalent on offset/limit bounds — flagged as not currently reachable (precompiles only pass key-only PageRequests) but worth tightening for the doc comment's own invariant.
Extended reasoning...
This PR reworks ABCI query-context tainting and adds dual pagination implementations (unrestricted for LCD/gRPC, v6.6-capped for EVM-precompile-reachable queries) across six modules, specifically to avoid AppHash/LastResultsHash divergence. That is exactly the class of change (consensus-critical, cross-cutting context flag) that should get a human look independent of whether every finding is resolved.
The main correctness concern from this thread — CreateQueryContext unconditionally tainting EVM simulation/trace-replay contexts with abciQuery=true, causing eth_call/debug_trace* to diverge from consensus pagination behavior — is fixed by the latest commit, which narrows WithIsABCIQuery(true) to only the two ABCI-query entry points. I confirmed this by reading the current abci.go/grpcserver.go and building the affected packages.
A secondary, previously flagged nit (VerifyPagination* not gated by enforceV66ScanLimit in the V66 paginators) remains, but both the original reporter and I agree it is not reachable today since precompiles only pass key-only PageRequests. No new issues surfaced in this pass.
There was a problem hiding this comment.
This PR touches AppHash/LastResultsHash-sensitive pagination across staking, gov, distribution, feegrant, and authz query handlers plus the EVM precompile call paths, so I'm leaving it for a human rather than shadow-approving.
The abciQuery-leaking-into-EVM-RPC/simulation concern raised earlier (cursor[bot], seidroid[bot], and my own prior review) is resolved by the follow-up commits ae04df3 and 79850d0 — CreateQueryContext no longer sets abciQuery itself (only the genuine ABCI-query/gRPC call sites do), and StaticCallEVM now forces it off so precompile static calls always mirror consensus behavior regardless of entry path.
No new bugs found this run. Beyond the reported findings, I also checked:
- The plain
Paginate(no V66 variant) call sites (ValidatorUnbondingDelegations,DelegatorDelegations,Votes,Deposits,Allowances,Redelegations) — none are reachable from precompiles, so the missing V66 cap there isn't a live gap. StaticCallEVM's unconditionalWithIsABCIQuery(false)— intentional per the "match consensus semantics" commit, not a regression for genuine LCD/gRPCStaticCallqueries.
seidroid[bot]'s three remaining nit-level comments (gating Verify* under enforceV66ScanLimit, the now-unenforced MaxLimit constant, and the GetBlockWithTxs allocation off-by-one) are still open and unaddressed on the thread.
Extended reasoning...
No bugs were found by the bug hunting system this run. The two candidates it raised (missing PaginateV66 variant; StaticCallEVM forcing v6.6 semantics for genuine LCD/gRPC StaticCall) were both verified as non-issues: the former because no precompile reaches the plain-Paginate call sites, the latter because forcing v6.6 semantics in StaticCallEVM is the deliberate intent of commit 79850d0 (static precompile calls must reproduce consensus behavior regardless of entry path).
This PR is a fix for a real, previously-shipped regression (#3494 broke LCD/gRPC pagination) and re-introduces v6.6-compatible scan-limit paginator variants specifically to keep AppHash/LastResultsHash stable for precompile-reachable query handlers. That is exactly the kind of consensus-critical, cross-cutting change (new sdk.Context field, dual paginator variants selected via ctx.IsABCIQuery() in five keeper packages, EVM keeper changes) that warrants human sign-off even when no new bugs are found.
Multiple independent reviewers (cursor[bot], seidroid[bot], and my own earlier review) flagged the same blocker across this PR's history: contexts built via CreateQueryContext for historical EVM RPC/trace replay (app.RPCContextProvider) were inheriting abciQuery=true, which would have caused precompile query handlers to diverge from consensus behavior during debug_trace*/eth_call replay. I verified this is now fixed: CreateQueryContext itself no longer sets abciQuery (commit ae04df3), and StaticCallEVM independently forces it false (commit 79850d0), so RPCContextProvider-derived contexts default to v6.6-compatible behavior.
Three lower-severity seidroid[bot] comments remain open and unaddressed: (1) VerifyPaginationOffset/VerifyPaginationLimit were deleted rather than gated under enforceV66ScanLimit — the reporter themselves noted this is unreachable today since precompiles pass key-only PageRequests, but it's a latent divergence from documented v6.6-exact-preservation intent; (2) MaxLimit is now math.MaxUint64, making it a no-op constant that misleadingly still reads as a cap; (3) GetBlockWithTxs's allocation cap doesn't subtract offset, allowing a slightly larger-than-necessary allocation. None of these are functional bugs, but they are unresolved reviewer feedback on a sensitive PR, which per review guidance should not be waved through with an approval.
There was a problem hiding this comment.
This PR touches consensus-critical pagination behavior across many query handlers (staking, gov, feegrant, distribution, authz) with dual code paths meant to keep precompile execution byte-for-byte compatible with released v6.6 semantics while relaxing LCD/gRPC pagination — that split is exactly the kind of change where a subtle mistake causes AppHash or LastResultsHash divergence, so I think this warrants a human look regardless of whether an active bug is found.
Note: the IsABCIQuery leak into RPCContextProvider/historical EVM RPC/debug_trace* replay that I (and other reviewers) flagged on an earlier revision looks fixed by the follow-up commits ae04df3 and 79850d0 — CreateQueryContext no longer sets the flag itself, and StaticCallEVM now explicitly forces WithIsABCIQuery(false). Still open from a prior reviewer comment: in filtered_pagination.go, the VerifyPaginationOffset/VerifyPaginationLimit calls were dropped unconditionally rather than gated on enforceV66ScanLimit, so FilteredPaginateV66 isn't byte-for-byte v6.6-equivalent for offset/limit validation (currently unreachable since precompiles only pass key-based PageRequests, but worth a human decision on whether to close that gap by construction).
Extended reasoning...
Overview
The PR reworks Cosmos SDK pagination (sei-cosmos/types/query) to restore permissive LCD/gRPC behavior while adding v6.6-locked *V66" paginator variants for EVM-precompile-reachable query handlers in staking, gov, feegrant, distribution, and authz, routed via a new sdk.Context.IsABCIQuery()flag set only on ABCI/gRPC query contexts. It also capsGetBlockWithTxsallocation, re-pagesCollectAllTotalSupply` at 1,000, and saturates offset+limit math to avoid uint64 overflow.
Security risks
No injection/auth-bypass surface. The real risk class here is consensus safety: if the IsABCIQuery/V66 routing is wrong on any reachable path, precompile gas consumption or revert behavior can diverge from what was actually committed, corrupting AppHash or LastResultsHash. A significant instance of exactly this (RPCContextProvider/historical EVM RPC inheriting the unrestricted paginator) was flagged on an earlier revision and appears fixed by the follow-up commits ae04df3/79850d0, which moved WithIsABCIQuery(true) out of CreateQueryContext and into the two ABCI/gRPC query call sites, and made StaticCallEVM force the flag off.
Level of scrutiny
High. This is a targeted fix to a previous consensus-hardening regression, spanning 17 files including baseapp query plumbing, five module query handlers, and the EVM keeper's precompile entrypoints. The correctness argument depends on every precompile-reachable call site consistently choosing the V66 variant, which is not enforceable by the type system — a missed call site is a silent AppHash risk. That argues for human sign-off even with no active bug found this run.
Other factors
A previous reviewer (seidroid) flagged a second, lower-severity gap that remains open: FilteredPaginateV66/GenericFilteredPaginateV66 no longer perform the pre-v6.6 VerifyPaginationOffset/VerifyPaginationLimit checks at all (they were removed unconditionally rather than gated on enforceV66ScanLimit), so the V66 paginators aren't a complete behavioral match for released v6.6 on that axis. It's currently unreachable since every precompile call site passes a key-only PageRequest, but it undermines the exact-preservation claim in the function's own doc comment. Test coverage for the new saturation/next-key/count-total semantics looks reasonably thorough (large-store and sparse-filter cases for both Paginate and FilteredPaginate).
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-3867-to-release/v6.6
git worktree add --checkout .worktree/backport-3867-to-release/v6.6 backport-3867-to-release/v6.6
cd .worktree/backport-3867-to-release/v6.6
git reset --hard HEAD^
git cherry-pick -x 8e06e535e91a293b2d4624862deda76096925af0
git push --force-with-lease |
* main: test(config): complete the GetConfig read-site coverage (PLT-893) (#3870) Remove interchain swagger API and protos (#3881) fix(flatkv): preserve empty misc values and reject malformed empty node imports (#3869) fix(evm): count post-admission apply failures in dynamic base-fee gas (CON-359) (#3871) scripts: load generator for arctic-1 and atlantic-2 (#3850) Update go-releaser heading with experimental notice (#3879) fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780) (#3836) Remove unused interchain accounts implementation (#3875) test(config): extend golden value test coverage (PLT-893) (#3861) Update v6.6 changelog in prep to cut patch release (#3876) Close temporary rootmulti store in connection types setup (#3872) Restore LCD pagination while preserving v6.6 precompile semantics (#3867)
4 participants
The query hardening introduced in #3494 broke previously valid LCD and gRPC pagination requests. Large limits and offsets were rejected, sparse filtered queries failed after scanning 10,000 entries, and count_total could not calculate totals for large stores.
Restore the pre-v6.6 client-facing pagination behavior:
range
Do not restore the upstream behavior that implicitly enables count_total when limit is omitted.
EVM precompiles invoke several Cosmos query handlers during transaction execution. Changing their filtered scan limits after v6.6 can alter gas consumption and success/revert control flow, producing AppHash and
LastResultsHash divergence.
Add explicit v6.6-compatible filtered paginator variants and select them for EVM execution in the transaction-reachable staking, governance, feegrant, distribution, and authz query handlers. LCD and gRPC requests continue using the repaired unrestricted implementations, while precompiles retain the released v6.6 consensus behavior.