PLT-980: Populate validator-indexed delegation store at the v6.7 upgrade - #3979
PLT-980: Populate validator-indexed delegation store at the v6.7 upgrade#3979amir-deris wants to merge 11 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>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3979 +/- ##
==========================================
- Coverage 61.23% 60.17% -1.07%
==========================================
Files 2177 2069 -108
Lines 190746 178244 -12502
==========================================
- Hits 116812 107252 -9560
+ Misses 62910 61009 -1901
+ Partials 11024 9983 -1041
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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.
Phase-1 validator→delegator index is well-structured and mirrors the existing UBD index, but the dual-write into the committed staking store is ungated (changing the app hash despite the non-app-hash-breaking label), and the CLI's --write path calls CommitMultiStore().Commit(false), which unconditionally panics under SeiDB.
Findings: 2 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test exercises the
--writepath ofseid tools staking backfill-delegation-index. The keeper-level backfill is covered, but the CLI wiring (app load, uncached context, commit) is untested — which is why theCommit(false)panic is latent. Even a smoke test over a temporary home directory would catch it. - [suggestion] Dual-writing without a backfill leaves the index partially populated on existing chains (only delegations touched after the upgrade get an entry), with no completion marker in state. Phase 2 must not treat a missing
0x37entry as "no delegation"; consider landing the migration-state flag in the same release as the dual-write so the two can never be observed out of sync. - [suggestion]
DelegationByValIndexKey = 0x37is declared out of numeric order between0x33and0x34in the prefix block. Since the surrounding block is ordered by byte value and is the only place collisions can be spotted by eye, place it afterRedelegationByValDstIndexKey = 0x36. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
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>
|
@seidroid review |
There was a problem hiding this comment.
The validator→delegator index is correctly keyed, gated with the same tracing/semver pattern x/distribution already uses, and all five previously flagged blockers/suggestions are genuinely resolved. What remains is non-blocking: the benchmark CLI's reported throughput excludes the commit that does the actual work, and its --write test runs against a genesis with zero delegations.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Turning the dual-write on at the upgrade without a backfill in the same release leaves the index permanently incomplete for pre-upgrade delegations that are never touched again. Phase 2 must not read
0x37as authoritative until a completion marker exists in state; landing the migration-state flag in the same release as the read-path switch is what makes the two impossible to observe out of sync. (Repeat of an earlier non-blocking note — the PR body acknowledges this as phase-2 scope, so it is recorded rather than pressed.) - 2 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
…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>
amir-deris
changed the title
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>
PR SummaryHigh Risk Overview At v6.7, the upgrade runs A new Note: pagination Reviewed by Cursor Bugbot for commit d761729. Bugbot is set up for automated code reviews on this repo. Configure here. |
| b := types.MustMarshalDelegation(k.cdc, delegation) | ||
| store.Set(types.GetDelegationKey(delegatorAddress, delegation.GetValidatorAddr()), b) | ||
| store.Set(types.GetDelegationKey(delegatorAddress, valAddr), b) | ||
| if k.DelegationByValIndexReady(ctx) { |
There was a problem hiding this comment.
Genesis never activates the index
Medium Severity
SetDelegation only dual-writes after DelegationByValIndexReady, and that marker is set solely by the v6.7 upgrade handler. InitGenesis never sets the marker, and ExportGenesis does not persist the marker or 0x37 keys, so new chains and genesis imports never activate the index and validatorDelegations keeps taking the capped full scan.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 09f745d. Configure here.
|
@seidroid review |
There was a problem hiding this comment.
The marker-keyed index design is sound — the readiness key makes the read-path flip atomic with the upgrade block and correct for historical replay — and all previously flagged threads are resolved. However, the new ValidatorDelegationsIndexed calls query.Paginate without the sdk.Context argument this fork's signature requires, so the staking keeper package does not compile.
Findings: 1 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The readiness marker is written only by the v6.7 upgrade handler, so any chain that does not pass through that upgrade never gets it: fresh-genesis chains (local devnets, new testnets) and genesis-export restarts both leave
0x38unset,SetDelegationstops dual-writing, andValidatorDelegationsIndexedpermanently falls back to the capped scan — reintroducing the exactMaxScanLimitrevert this PR fixes once such a network passes 10k delegations. It also means the indexed read path is exercised only by unit tests, never by the integration/e2e chains. Setting the marker inx/stakingInitGenesis(after theSetDelegationloop at sei-cosmos/x/staking/genesis.go:74) would close both gaps. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| valPrefix := types.GetDelegationsByValIndexKey(valAddr) | ||
| indexStore := prefix.NewStore(store, valPrefix) | ||
|
|
||
| pageRes, err := query.Paginate(indexStore, req.Pagination, func(key []byte, _ []byte) error { |
There was a problem hiding this comment.
[blocker] query.Paginate in this fork takes ctx sdk.Context as its first argument (sei-cosmos/types/query/pagination.go:51), which is how every other call site in this same file does it (lines 214, 330, 402, 487, 569, 590). As written the sei-cosmos/x/staking/keeper package does not compile, so the test plan in the PR description cannot have run green against this merge. Pass ctx before indexStore.
Passing ctx is also semantically correct here: scanLimitParamsFromContext returns an unenforced budget when !ctx.IsABCIQuery(), so consensus execution stays uncapped on the per-validator prefix, and ABCI queries get the same limits the other staking paginated queries observe.
|
|
||
| pageRes, err := query.Paginate(indexStore, req.Pagination, func(key []byte, _ []byte) error { | ||
| storeKey := types.GetDelegationKeyFromValIndexKey(append(valPrefix, key...)) | ||
| delegation, err := types.UnmarshalDelegation(k.cdc, store.Get(storeKey)) |
There was a problem hiding this comment.
[suggestion] store.Get(storeKey) returns nil for an index entry with no matching delegation, and UnmarshalDelegation(cdc, nil) succeeds with a zero-valued Delegation. That value then reaches DelegationToDelegationResponse, whose first statement is k.GetValidator(ctx, del.GetValidatorAddr()) — and Delegation.GetValidatorAddr() panics on an empty address string (sei-cosmos/x/staking/types/delegation.go:66). Because this is on the staking precompile's consensus path, a single orphaned index key turns into a panic rather than a returned error.
The paired writes in SetDelegation/RemoveDelegation plus the migration should make orphans impossible, and TestMigrateDelegationByValIndexNoOrphans pins that. Still, a nil check that returns status.Error(codes.Internal, ...) keeps a state inconsistency from being a halt-shaped failure, and costs one branch per row.
| } | ||
|
|
||
| result.TotalDelegations++ | ||
| indexKey := types.GetDelegationByValIndexKey(delAddr, delegation.GetValidatorAddr()) |
There was a problem hiding this comment.
[suggestion] The delegator address is parsed with sdk.AccAddressFromBech32 and its error returned, but the validator address goes through delegation.GetValidatorAddr(), which panics on a parse failure (sei-cosmos/x/staking/types/delegation.go:66-72). So the "returns an error rather than panicking on a malformed proto or address" property the PR describes holds for one of the two addresses only, and a malformed ValidatorAddress in state panics inside the upgrade handler instead of surfacing as an upgrade error. Use sdk.ValAddressFromBech32(delegation.ValidatorAddress) with the same error wrapping for symmetry.
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 d761729. Configure here.
| } | ||
| delegations = append(delegations, delegation) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Indexed query omits pagination context
High Severity
ValidatorDelegationsIndexed calls query.Paginate without the ctx argument that the helper requires and that ValidatorUnbondingDelegations already passes. The call does not match the current pagination API, so the indexed precompile path cannot build or run.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d761729. Configure here.
1 participant
Problem
MaxScanLimit(10,000) is enforced byquery.FilteredPaginateV66, whichQuerier.ValidatorDelegationsselects wheneverctx.IsABCIQuery()is false — that is,on consensus execution. The staking precompile's
validatorDelegationsfilters over thewhole
0x31delegation prefix to find one validator's rows, so at pacific-1's ~82kdelegations it scans past the limit and reverts.
Plain gRPC/LCD queries take the uncapped
FilteredPaginatepath and were never affected.Approach
Populate a validator→delegator secondary index (
0x37) in one pass inside the v6.7upgrade handler, and switch the current staking precompile to read it. The indexed
prefix holds only one validator's delegations, so iteration tracks page size instead of
the total delegation count and the cap is never approached.
Gating is a readiness marker key (
0x38) written by the migration, not a versioncomparison. Because the marker is versioned state,
DelegationByValIndexReady(ctx)iscorrect for historical queries, re-traced blocks, and state-synced nodes without the
caller supplying an upgrade name or height. This matters because the read switch changes
transaction results: if nodes flipped it at different heights they would diverge. The
marker makes the flip atomic with the upgrade block by construction.
Below the upgrade height the marker is absent, the read falls through to the existing
scan, and the query reverts exactly as it does at those heights today — which is what
historical replay requires.
What changed
Migration —
Keeper.MigrateDelegationByValIndexwrites an index key per delegationthen sets the marker. Returns an error rather than panicking on a malformed proto or
address, and is a no-op once the marker is set. Called from the
v6.7handler on aninfinite gas meter.
Gating —
DelegationByValIndexReadyKey(0x38) plusDelegationByValIndexReady.The dual-write in
SetDelegation/RemoveDelegationgates on it. The previousdelegationByValIndexActivesemver/IsTracinggate is removed.Read path — new
Querier.ValidatorDelegationsIndexed. With the marker set itpaginates
GetDelegationsByValIndexKey(valAddr)and resolves entries throughGetDelegationKeyFromValIndexKey, matching the shapeValidatorUnbondingDelegationsalready uses; without it, it delegates to
ValidatorDelegations. That method itself isunchanged.
Precompile wiring — only
precompiles/staking/staking.gocalls the indexed query.Removed — the
seid tools staking backfill-delegation-indexbenchmark CLI. Itssizing job is answered (below), and its audit job is now a keeper invariant test.
Compatibility notes
validatorDelegations,next_keybecomes an indexkey rather than a delegation key, so a cursor issued before the upgrade will not
resolve after it.
legacy/v65,legacy/v630and the rest keepcalling the unindexed querier and keep their current behavior at their eras — zero
diff in those files.
legacy/v67is changed deliberately. It is a committed generated snapshot ofstaking.gothatsetup.godoes not yet reference (latestUpgradestill routes tothe live file). The next
scripts/bump_versionrun will wire it in as the v6.7-eraprecompile, and the pre-change snapshot called the unindexed querier — so the fix
would have regressed at that bump, and v6.7-era heights would behave differently
across binaries. v67 is this era's snapshot, not a prior era. Regenerating it instead
of hand-editing would be equivalent; flagging since the file is marked
DO NOT EDIT.Sizing
max(heatseeker_client_state_analysis_prefix_num_keys{module="staking", prefix="31"})on pacific-1 gives ~82k delegations:
One linear scan with no nested lookups over 82k rows, writing ~43-byte keys, is
comfortably within the upgrade block's budget — negligible next to the pacific-1
evmmodule's ~710M keys at ~2M keys/min. No chunked/EndBlocker migration is needed.
Test plan
TestGRPCQueryValidatorDelegationsIndexedBeyondScanLimit— seedsMaxScanLimit + 1delegations plus one to the target validator under a
0xff…ffdelegator that sortslast, so the scan must traverse the whole keyspace. Pins that the scan reverts both
before and after the migration, and that the indexed query answers it afterwards.
TestGRPCQueryValidatorDelegationsIndexedMatchesScan— both paths agree on avalidator small enough for either to answer.
TestMigrateDelegationByValIndex/…NoOrphans— migration counts, marker,idempotency, and the both-directions invariant: every delegation is indexed and every
index entry resolves to a delegation.
TestDelegationByValIndexNotReadyNoDualWrite/…DualWriteAfterMigration— thewrite gate follows the marker.
TestV67PopulatesDelegationByValIndex— seeds a delegation through the raw store,applies the v6.7 plan, asserts the marker and index key exist.
Linear: https://linear.app/seilabs/issue/PLT-980/