Repository navigation
PLT-980: Index delegations by validator at the v6.8 upgrade - #4518
amir-deris wants to merge 24 commits into
Conversation
Dual-write a validator→delegator secondary index on delegation mutations and expose a dev CLI to benchmark one-shot backfill before choosing migration strategy. Co-authored-by: Cursor <cursoragent@cursor.com>
Use Commit(true) so storev2 does not panic, reject --write with --height, and close the app so SeiDB flushes before exit. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep SetDelegation and RemoveDelegation app-hash neutral before the upgrade by writing the validator index only once ClosestUpgradeName reaches v6.7. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the subcommand PersistentPreRunE that shadowed the root seid hook and collapsed duplicate app.New paths into one call with optional LoadHeight. Co-authored-by: Cursor <cursoragent@cursor.com>
Place 0x37 after the other delegation prefixes and smoke-test dry-run, --write commit, and incompatible flag handling for the backfill tool. Co-authored-by: Cursor <cursoragent@cursor.com>
Dual-write on DeliverTx and gate pre-v6.7 behavior only for tracing replay via ClosestUpgradeName, matching the distribution keeper pattern. Co-authored-by: Cursor <cursoragent@cursor.com>
…ress. Clarify that the CLI is for analysis only, require confirmation before running, and emit stderr heartbeats while scanning delegations so long mainnet runs are observable. Co-authored-by: Cursor <cursoragent@cursor.com>
Backfill the secondary index in the upgrade handler so operators never need a separate migration step, gate dual-writes on a store marker, and remove the CLI backfill command that could mutate live state. Co-authored-by: Cursor <cursoragent@cursor.com>
CI Go/Lint and lint jobs failed the gofmt -s -l check because the x/upgrade import was out of alphabetical order relative to x/staking/types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ond-index Resolve the app/upgrades.go conflict by keeping main's v6.7 handler and moving the delegation-by-validator index migration into the v6.8 handler, since v6.7 has already shipped. The simulation decoder was deleted on main, so its index cases go with it. The legacy v6.7 staking precompile is restored to main's frozen copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
InitGenesis now populates the index and sets its ready marker, so chains started from genesis read validator delegations from the index rather than falling back to the full scan for their whole life. The migration no longer probes for an existing entry before each write: no entry can exist while the index is not ready. ValidatorDelegationsIndexed builds each lookup key in a fresh slice and wraps pagination errors as ValidatorDelegations does. The v6.8 upgrade tests cover the handler populating one index entry per delegation, and the offline target phase accepts the index keys as part of the upgrade block's specified writes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v6.7 fixture held no delegations, so the target phase's index check had nothing to compare. The source phase now stores delegations to two validators through the v6.7 keeper. The staking store is not among the snapshotted retained stores, so the touched-key list does not need an index entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 #4518 +/- ##
==========================================
+ Coverage 57.29% 57.33% +0.03%
==========================================
Files 2140 2141 +1
Lines 167395 167502 +107
==========================================
+ Hits 95914 96030 +116
+ Misses 71476 71467 -9
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The index readiness check on every delegation write was a metered read, and genesis marks the index ready, so EVM regression replays of v6.0.x transactions that touch delegations reported extra gas. Read the marker without charging gas, and clear it in the regression setup for versions before v6.8, alongside the incremental-tally marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR SummaryHigh Risk Overview New store prefixes ( Tests cover keeper/genesis backfill edge cases, in-process v6.8 upgrade + EndBlocker completion, and offline upgrade fixtures that seed pre-index delegations and assert a 1:1 index after migration. EVM regression setup clears the ready marker for simulated heights before v6.8. Reviewed by Cursor Bugbot for commit ed8034f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a delegation-by-validator index to staking. The v6.8 upgrade handler and InitGenesis build it, and the staking precompile's validatorDelegations now pages through it, which fixes the scan-limit reverts. One blocker: the frozen v6.8 precompile snapshot under legacy/v68 still calls the unindexed query, so it no longer matches what v6.8 runs. Codex's single finding is kept as a nit: the byte overflow is real, but the address verifier only allows 20- or 32-byte addresses, so it cannot be reached on Sei.
Blocking
1 finding on the changed lines, as inline comments.
1 nit, not posted on the code
sei-cosmos/x/staking/types/keys.go:210— (from codex)valAddrLenis a byte, so1+valAddrLenwraps to 0 at a 255-byte length andvalAddrLen+2wraps at 254. Sei's address verifier only accepts 20- or 32-byte addresses, so this can't happen on chain, and the code copies the existing UBD/RED index helpers. Converting the length tointbefore slicing would make it robust.
seidroid review · decision request_changes · session be088953e8794fc89a0309b055de7471 · turn resp_claude_b25d91efca8b363bd86a9631676e2fe9 · item 13d743706fc659838f58d96385b5bc9e
Findings: 1 blocking | 0 non-blocking | 1 posted inline
There was a problem hiding this comment.
The only change since my last review is that the frozen v6.8 snapshot (precompiles/staking/legacy/v68/staking.go) now also calls ValidatorDelegationsIndexed, so v6.8 heights keep the indexed behaviour after the next version bump; that resolves my one blocker, and nothing else blocks. Codex's finding is still kept as a nit: the byte overflow is real, but the address verifier only allows 20- or 32-byte addresses, so it cannot be reached on Sei.
1 nit, not posted on the code
sei-cosmos/x/staking/types/keys.go:210— (from codex)valAddrLenis a byte, so1+valAddrLenwraps to 0 at a 255-byte length andvalAddrLen+2wraps at 254. Sei's address verifier only accepts 20- or 32-byte addresses, so this can't happen on chain, and the code copies the existing UBD/RED index helpers. Converting the length tointbefore slicing would make it robust.
seidroid review · decision approve · session be088953e8794fc89a0309b055de7471 · turn resp_claude_b6dc99eeb08693602e805b9a989d017a · item da76bc00a60e51fd942017b35d43ae39
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Superseded: the latest review found nothing blocking in this change.
Fold the indexed read into ValidatorDelegations behind DelegationByValIndexReady instead of a separate ValidatorDelegationsIndexed query, so the precompiles, REST and gRPC share one path. Heights before the index is ready still take the scan and reproduce its results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
The PR now has Querier.ValidatorDelegations itself switch to the index once the ready marker exists, so the v6.6, v6.7, v6.8 and current precompiles, and REST/gRPC, all take the scan below v6.8 and the index from v6.8. That routing also settles my earlier legacy/v68 blocker, nothing blocks, and I verified both of codex's findings and kept them: the cursor finding as a suggestion and the keys.go overflow as a nit, since only 20- and 32-byte addresses pass the address check.
Non-blocking
1 finding on the changed lines, as inline comments.
1 nit, not posted on the code
sei-cosmos/x/staking/types/keys.go:796— (from codex)valAddrLenis a byte, so1+valAddrLenwraps to 0 at a 255-byte length andvalAddrLen+2wraps at 254. Sei's address verifier only accepts 20- or 32-byte addresses, so this can't happen on chain, and the code copies the existing UBD/RED index helpers. Converting the length tointbefore slicing would make it robust.
seidroid review · decision approve · session be088953e8794fc89a0309b055de7471 · turn resp_claude_a9026a53ec5ce8c8bfb8128b65962b31 · item d05f98094900531e88bb10e52a0b248e
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Populating the delegation-by-validator index in the v6.8 handler made the upgrade block's work and memory grow with the delegation count, which anyone can inflate ahead of a known upgrade height. The handler now only starts a backfill: a cursor under 0x39 marks it in progress, delegation writes maintain the index from then on, and the staking EndBlocker indexes 1000 delegations per block after the cursor until it reaches the end and marks the index ready. Running the batch in EndBlock keeps its pending writes out of the staking cache while transactions iterate it. ValidatorDelegations keeps the scan until the index is ready. Genesis still indexes in one pass. A non-positive backfill limit is rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
The v6.8 handler now only starts the index build, and the staking EndBlocker indexes 1,000 delegations per block behind a cursor at 0x39; delegation writes keep the index current once the build starts, so changes on either side of the cursor still leave it exact, and nothing blocks. I kept both of codex's findings after checking them: the stale scan cursor still silently skips one delegation instead of failing as the description says (suggestion), and the keys.go byte overflow can't happen with Sei's 20- or 32-byte addresses (nit).
Non-blocking
1 finding on the changed lines, as inline comments.
1 nit, not posted on the code
sei-cosmos/x/staking/types/keys.go:211— (from codex)valAddrLenis a byte, so1+valAddrLenwraps to 0 at a 255-byte length andvalAddrLen+2wraps at 254. Sei's address verifier only accepts 20- or 32-byte addresses, so this can't happen on chain, and the code copies the existing UBD/RED index helpers. Converting the length tointbefore slicing would make it robust.
seidroid review · decision approve · session be088953e8794fc89a0309b055de7471 · turn resp_claude_8dda07b79368f4c794a5ab3ee43d914e · item 3686865114ac52d08155a8a7383c28bf
Findings: 0 blocking | 1 non-blocking | 1 posted inline
| valPrefix := types.GetDelegationsByValIndexKey(valAddr) | ||
| indexStore := prefix.NewStore(store, valPrefix) | ||
|
|
||
| pageRes, err := query.Paginate(ctx, indexStore, req.Pagination, func(key []byte, _ []byte) error { |
There was a problem hiding this comment.
suggestion — (from codex) The description says a scan cursor from before the switch "will not resume", but it does resume, one entry too late. The scan cursor is len|del|len|val, and the index key len|del is a prefix of it, so it sorts first and the cursor's own delegator is dropped from the page with no error. Any client holding a cursor when the index takes over loses one delegator. Fix it by trimming a cursor longer than the delegator segment back to len|del, or by rejecting it.
This branch has not been deployed
2 participants
The staking precompile's
validatorDelegationsreverts for most validators on atlantic-2, including bonded ones with thousands of delegators (PLT-1447). Delegations are stored by delegator, so listing one validator's delegations scans the whole delegation store. The precompile path keeps the v6.6 paginator for consensus safety, and that paginator fails once it has scanned 10,000 entries without collecting a full page of 100. Any validator whose delegations are sparse in that keyspace can never return a page, and the caller gets a bare revert with no cursor.This adds a delegation-by-validator index under prefix
0x37(validator | delegator, empty value), a ready marker at0x38, and a backfill cursor at0x39. Once the marker is present,Querier.ValidatorDelegationspages over a single validator's prefix, so each page costs about one read per result; before that it keeps the full scan. The marker is versioned state, so replaying or tracing heights before the index is ready still takes the old path and reproduces the old result, and the frozen v6.6/v6.7 precompiles behave as before. The query serves the staking precompile and REST/gRPC alike, so REST/gRPC callers also get the index once it is ready.Populating the index. The v6.8 upgrade handler does not build the index; it only writes the cursor, which starts a backfill. From then on
SetDelegationandRemoveDelegationmaintain the index (their marker reads are gasless, so pre-upgrade delegation gas is unchanged), and the stakingEndBlockerindexes the next 1,000 delegations after the cursor each block. When the cursor reaches the end it is deleted and the ready marker is set. Building it in one pass inside the handler would make the upgrade block's work and memory grow with the delegation count, which anyone can inflate ahead of a known upgrade height. Pending writes also slow every staking iterator created in that block, since the cache store rescans them per iterator. The batch runs in EndBlock rather than BeginBlock so that no transaction iterates the staking store while a batch is pending.InitGenesisstill builds the index in one pass withMigrateDelegationByValIndex, so fresh chains start with it ready.Pagination across the switch. The index path returns
next_keyas a length-prefixed delegator rather than a full delegation key. A forward cursor obtained from the scan before the index was ready resumes at the delegation it names, so a client paging across that height neither skips nor repeats a delegator. A reverse page cannot resume from such a cursor and is rejected withInvalidArgument.This changes consensus at v6.8: from the upgrade height, delegation writes also write index entries, each block runs a backfill batch until the index is ready, and precompile results change once it is. Coverage: staking keeper and genesis tests for the index, the batched backfill (including delegations written and removed on either side of the cursor) and scan-cursor resumption;
upgrade_v68in-process tests that the handler only starts the backfill and the EndBlocker completes it; andmake upgrade-test-offline FROM_REF=release/v6.7 TO_REF=<this branch>, whose v6.7 source phase now seeds delegations and whose target phase requires exactly one index entry per delegation after the upgrade block.The existing size of the index seems to be around 80k, which the backfill covers in about 80 blocks:
