PLT-980: Add validator-indexed delegation store (phase 1) - #3979
PLT-980: Add validator-indexed delegation store (phase 1)#3979amir-deris wants to merge 9 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3979 +/- ##
==========================================
- Coverage 59.05% 57.58% -1.47%
==========================================
Files 2297 2229 -68
Lines 196538 186699 -9839
==========================================
- Hits 116074 107519 -8555
+ Misses 69722 69354 -368
+ Partials 10742 9826 -916
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>
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 24c0ccc. Configure here.
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.
| return nil | ||
| } | ||
|
|
||
| commitID := seiApp.CommitMultiStore().Commit(true) |
There was a problem hiding this comment.
[suggestion] elapsed / delegations_per_second are printed before this line, so they exclude the commit — which is where nearly all the work happens on the --write path.
NewUncachedContext hands out app.cms, and storev2/rootmulti.GetKVStore returns the raw commitment.Store (no cache wrap). Its Set only appends to an in-memory changeset (sei-cosmos/storev2/commitment/store.go:80-86); the memiavl insertion and the SC/SS flush all happen inside Commit(true) here. So the --write number the tool reports is effectively the dry-run number plus some appends, and the decision gate in the PR description ("does elapsed fit the upgrade halt budget?") would be sized against the wrong figure.
Time the commit as well and print it (e.g. commit_elapsed= plus a total), so the halt-budget comparison uses persist-inclusive numbers. Worth noting in the same breath: because every write is buffered until this point, peak RSS scales with the delegation count — useful to report alongside the timing.
Separately, Commit(true) writes version LastBlockHeight()+1 with no corresponding block, so the data copy is not resumable as a node afterwards. The Long help says to use a copy, but calling that out explicitly there would save someone a confusing restart failure.
| }) | ||
|
|
||
| require.Contains(t, stdout, "dry_run=false") | ||
| require.Contains(t, stdout, "committed_version=") |
There was a problem hiding this comment.
[suggestion] This test never exercises a write. seedCommittedSeiDBApp runs InitChain with NewDefaultGenesisState and no gentxs, so the staking genesis has no delegations — the command reports total_delegations=0 index_written=0 and Commit(true) flushes an empty changeset. The assertions here only prove the command reached the commit call.
That does guard the previous Commit(false) panic, which is the main thing. But the behaviour the flag exists for — index keys surviving the commit — is unverified. Seed at least one delegation into the staking store before committing, then after cmd.Execute() reopen the app from home and assert store.Has(types.GetDelegationByValIndexKey(del, val)). That is also the only test that would catch the commitment-store buffering semantics (Set invisible to Get/Has until commit) regressing.
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>
1 participant
Summary
0x37) for staking delegations, following the existing unbonding-delegation index pattern (0x33).SetDelegation/RemoveDelegationso new mutations stay indexed.BackfillDelegationByValIndexkeeper helper for one-shot backfill of existing delegations.seid tools staking backfill-delegation-indexdev CLI to benchmark backfill duration before choosing halt vs incremental migration.This is phase 1 only. It does not yet switch
ValidatorDelegationsto the fast query path, add migration state, EndBlocker incremental migration, or an upgrade handler.Linear: https://linear.app/seilabs/issue/PLT-980/
Test plan
RPC node backfill benchmark (manual)
Use this to measure mainnet-scale backfill time and decide between one-shot upgrade backfill vs incremental EndBlocker migration.
Build this branch
Prepare state (do not run against a live writing node)
$HOME/datawhen using--write.Dry-run first (read-only; safe on a copy)
Note
total_delegations,elapsed, anddelegations_per_second.Optional write benchmark (mutates state; copy only)
This writes missing index keys and commits state. Expect output including
index_writtenandcommitted_version.Optional: pin height
Decision gate
Made with Cursor