Add gov proposal based migration trigger#3650
Conversation
PR SummaryHigh Risk Overview Adds a generic Removes Reviewed by Cursor Bugbot for commit a5cf528. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
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 #3650 +/- ##
==========================================
- Coverage 59.16% 58.69% -0.47%
==========================================
Files 2262 2214 -48
Lines 187009 182068 -4941
==========================================
- Hits 110643 106867 -3776
+ Misses 66430 65680 -750
+ Partials 9936 9521 -415
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // composite SC backend drives the in-flight memiavl->flatkv migration that | ||
| // BeginBlock paces via the migration gov param. Fail fast if the legacy | ||
| // root multistore is somehow in use. | ||
| rs, ok := app.CommitMultiStore().(*rootmulti.Store) |
There was a problem hiding this comment.
If we need to cast like this, would it make sense to change the type returned by app.CommitMultiStore()?
| genesisImportConfig genesistypes.GenesisImportConfig | ||
|
|
||
| stateStore seidb.StateStore | ||
| rs *rootmulti.Store |
There was a problem hiding this comment.
nit, would it make sense to use a more descriptive name like rootStore? Out of context, it's not obvious what rs stands for.
There was a problem hiding this comment.
good suggestion, will change
| if migrationBatchSize < 0 { | ||
| return nil, fmt.Errorf("migration batch size must not be negative, got %d", migrationBatchSize) |
There was a problem hiding this comment.
Could we make migration batch size an unsigned integer?
There was a problem hiding this comment.
Yeah, that's a good point
There was a problem hiding this comment.
Tried that, looks like it would require a big refactory, will actually do a fallback here to 0 if it's negative
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-structured, thoroughly-tested change that moves the SC migration rate from a node-local config to a consensus-deterministic governance param (migration.NumKeysToMigratePerBlock), correctly read once per block in BeginBlock and pushed down to the composite SC store. No blocking correctness/security issues found; a few non-blocking notes, chiefly that the new subspace value is not genesis-exportable.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Migration param is not genesis-exportable (confirms Codex P2).
x/paramsExportGenesis(sei-cosmos/x/params/keeper/genesis.go) only exportsFeesParams/CosmosGasParams, and the newmigrationsubspace has no owning module's InitGenesis/ExportGenesis. After aseid export/import while a migration is in flight,NumKeysToMigratePerBlockis dropped and BeginBlock re-seeds the0(paused) default, halting the drain until governance re-submits a proposal. Data isn't lost (the boundary cursor lives in flatkv) and it's recoverable, but worth either adding genesis plumbing for this subspace or documenting the operational caveat. - Operational behavior change worth calling out in release notes: because the param defaults to
0(paused) and is the sole source of the rate, any chain already running in a migrate write-mode will pause its drain at the upgrade height until a param-change proposal raises the rate. Intentional per the consensus-safety design, but operators must know to submit the proposal. - REVIEW_GUIDELINES.md and cursor-review.md were empty/absent, so no repo-specific guidelines or Cursor second opinion were available; only the Codex pass (one P2, addressed above) contributed.
- Integration test nit (integration_test/gov_module/gov_proposal_test.yaml): the verifier
NEW_PARAM == 12345compares an unquoted numeric against a value the pipeline produces as a string (jq -r .value | tr -d "\""), unlike the sibling assertionNEW_ABCI_PARAM == "true". If the eval engine is type-strict this could mis-compare; quoting"12345"would match the established pattern. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // composite SC backend drives the in-flight memiavl->flatkv migration that | ||
| // BeginBlock paces via the migration gov param. Fail fast if the legacy | ||
| // root multistore is somehow in use. | ||
| rs, ok := app.CommitMultiStore().(*rootmulti.Store) |
There was a problem hiding this comment.
[suggestion] This unconditional type assertion now panics in New() if the commit multistore is not *rootmulti.Store. New() is invoked by every seid subcommand that builds the app (export, query tooling, etc.), so please confirm no supported configuration (e.g. a legacy IAVL/storev2-disabled path) can reach here — otherwise this turns a previously-degraded path into a hard crash. If the legacy store is truly unsupported this is fine; a one-line note on why it's unreachable would help.
| } | ||
| numKeys := migration.DefaultNumKeysToMigratePerBlock | ||
| if subspace, ok := app.ParamsKeeper.GetSubspace(migration.SubspaceName); ok { | ||
| // The migration subspace has no owning module to seed it in InitGenesis, |
There was a problem hiding this comment.
[suggestion] The lazy seed correctly handles a fresh/never-set param deterministically, but because this subspace has no module ExportGenesis (x/params only exports Fees/CosmosGas params), the value is lost across a seid export/import: a mid-migration rate resets to the 0 default here and the drain pauses until governance re-raises it. Consider adding genesis import/export plumbing for this subspace, or documenting the caveat.
![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 2 potential issues.
❌ 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 91e1766. Configure here.
There was a problem hiding this comment.
The PR cleanly moves the SC migration rate from a node-local config to a consensus-safe governance param, with thorough plumbing and tests. However, an accidental rename of the receipt-store backend constant breaks an existing test (and decouples the test harness from the real config key), which blocks merge.
Findings: 1 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (cursor-review.md is empty); REVIEW_GUIDELINES.md is also empty, so no repo-specific standards could be applied.
- validateNumKeysToMigratePerBlock accepts any uint64. Because the value is cast to int before NextBatch (migration_manager.go:299), a governance value above math.MaxInt64 becomes negative and halts commit deterministically. Consider bounding the param to a sane operational maximum.
- app/abci_test.go's comment that math.MaxUint64 is 'forwarded verbatim with no conversion or clamping' is misleading — the app layer stores it verbatim, but migration_manager converts it to int downstream.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| const ( | ||
| receiptStoreBackendKey = "receipt-store.rs-backend" | ||
| receiptStoreBackendKey = "receipt-store.rootStore-backend" |
There was a problem hiding this comment.
[blocker] This key must stay receipt-store.rs-backend. The parser in sei-db/config/receipt_config.go reads flagRSBackend = "receipt-store.rs-backend" (and the rs-backend mapstructure tag), so renaming this constant to receipt-store.rootStore-backend decouples the app-layer key from what ReadReceiptConfig actually reads. Concretely, TestParseReceiptConfigs_RejectsInvalidBackend (app/seidb_test.go) now sets "rocksdb" under a key the parser ignores, the config falls back to the default pebbledb, and assert.Error fails — breaking CI. This looks like collateral damage from a global rs→rootStore rename (the comment at app/test_helpers.go:486 was hit too). Revert to receipt-store.rs-backend.
| if advanceMigration { | ||
| // Get the next batch of keys to migrate. | ||
| valuesToMigrate, newBoundary, err := m.iterator.NextBatch(m.migrationBatchSize) | ||
| valuesToMigrate, newBoundary, err := m.iterator.NextBatch(int(m.migrationBatchSize)) |
There was a problem hiding this comment.
[suggestion] int(m.migrationBatchSize) truncates a uint64 gov param. On 64-bit, a value above math.MaxInt64 becomes negative, NextBatch rejects size <= 0, and ApplyChangeSets returns an error during commit — a deterministic chain halt from a governance typo. Since validateNumKeysToMigratePerBlock accepts any uint64, consider bounding the param to a sane maximum (well under math.MaxInt) at validation time.
This reverts commit 91e1766.
There was a problem hiding this comment.
Converts the SC migration per-block rate from a consensus-unsafe node-local config into a deterministic governance param read in BeginBlock; the design is sound, AppHash-safe (all nodes read the same value, lazy-default-set runs identically), and very thoroughly tested at unit, store, and integration levels. Only minor, non-blocking observations.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- New() now hard-panics if the commit multistore is not *rootmulti.Store (app/app.go:560). This is intentional fail-fast, but it is a behavior change: any App-construction path that could use a non-storev2 store (offline/CLI commands, tooling) would now crash rather than degrade. Worth confirming every code path that builds App goes through the storev2 rootmulti store.
- The two later casts in app.go (~line 770 and ~2685) still re-derive the store via app.CommitMultiStore().(*rootmulti.Store) instead of reusing the new app.rootStore field; minor redundancy now that the type is guaranteed at construction.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported no material issues.
- Consider a brief note/log when the lazy default-set writes the param the first time (app/abci.go), purely for operator visibility on the upgrade block; not required for correctness.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
3 participants
Summary
The state-commitment (SC) store's in-flight
memiavl → flatkvmigration was previously paced by a node-local config (sc-keys-to-migrate-per-block). Because migration writes feed the AppHash, a per-node rate is consensus-relevant and a divergence risk: two validators draining at different rates fork the chain.This PR makes the per-block migration rate a module-agnostic governance parameter (
migration.NumKeysToMigratePerBlock) that every validator reads from chain state eachBeginBlockand applies identically. The gov param also serves as the migration trigger: it defaults to0(paused), and raising it via a param-change proposal starts the drain fleet-wide at a deterministic height.The old node-local config and its fallback are removed entirely — the gov param is now the sole source of the rate.
Key changes
New generic governance parameter
app/migration/params.go— new module-agnosticmigrationparams subspace definingNumKeysToMigratePerBlock(default0), itsKeyTable, and validation. Deliberately not EVM-specific so future module migrations can reuse it.app/app.go— registers themigrationsubspace; stores the*rootmulti.Storeon the app and fails fast if the (unsupported) legacy commit multistore is in use.app/abci.go—BeginBlockreadsNumKeysToMigratePerBlockfrom chain state and pushes it into the SC store before the block's first write (applyMigrationBatchSize).Plumbing: push the rate down to the migration router
sei-db/state_db/sc/types/types.go—Committerinterface gainsSetMigrationBatchSize(int) error.sei-cosmos/storev2/rootmulti/store.go— forwardsSetMigrationBatchSizeto the SC store; addsGetMigrationBatchSizefor observability/tests.sei-db/state_db/sc/composite/store.go— holds the gov-set batch size (atomic.Int64);buildRouterandSetMigrationBatchSizeuse it directly. Removed theeffectiveMigrationBatchSizeconfig fallback —0means paused, full stop.sei-db/state_db/sc/migration/*—Routerinterface gainsSetMigrationBatchSize; all router types implement it. Only the migration manager acts on it; every other router (module/passthrough/dual-write/thread-safe) treats it as a no-op.router_builder.gonow allows a0batch size (paused) instead of rejecting it.Removed the node-local config + fallback
sei-db/config/sc_config.go,sei-db/config/toml.go,docker/localnode/config/app.toml,app/seidb.go— dropped thesc-keys-to-migrate-per-blockfield, default, validation, TOML entry, and flag parsing.Tests
app/migration/params_test.go— unit tests for the new param (default, key-table registration, validation).app/abci_test.go— subspace registration,applyMigrationBatchSize(defaults/set/clamp), and cross-block "param set in block N takes effect in N+1" coverage.SetMigrationBatchSize(...)after construction instead of the removed config field. The random-migration framework re-applies the rate on every store (re)open, faithfully mirroring how production re-pushes the gov param after each restart.Integration test
integration_test/gov_module/— newParameterChangeProposaltest that raisesNumKeysToMigratePerBlockand asserts it takes effect.integration_test/contracts/verify_flatkv_evm_migrate.sh— rewritten to drive the migration via a governance param-change proposal (submit → deposit → quorum vote → poll forPASSED→ verify on-chain) instead of injecting the removed config.Docs
AGENTS.md— Code style now mandates running bothgofmtandgoimportson every touched.gofile.