evmigration + upgrades: validator-migration scaling, correctness fixes, and state-driven v1.20.1 EVM bring-up#184
Merged
Conversation
…gration Two hot-path optimizations for MigrateValidatorDelegations, layered on the scoped-iteration work already on this branch: - Thread the delegations/unbondings/redelegations already read by the MaxValidatorDelegations pre-check into MigrateValidatorDelegations rather than re-reading them. This removes a second O(N) scan of the same staking records (plus a second validator-scoped redelegation index scan) on the migration hot path, which dominated block time for validators with many delegations. Passing nil redelegations still triggers the internal scan so tests can exercise it directly. - Replace the reset-to-1 + increment-per-delegation refcount pattern with a single setHistoricalRewardsReferenceCount write of base(1)+len(delegations). All re-keyed delegations share one historical-rewards period, so the count is deterministic. New validatorHistoricalReward does an O(1) keyed store.Get of a single (validator, period) row; adjustHistoricalRewardsReferenceCount now uses it too. Eliminates the N+1 full-chain IterateValidatorHistoricalRewards scans the old path incurred per migrated validator. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0836de5805
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The wired redelegationsForValidator built its result by ranging a Go map, so the order was process-dependent. MigrateValidatorDelegations replays that slice via InsertRedelegationQueue, which appends to shared queue timeslices — redelegations sharing a completion time could therefore commit in different byte order on different nodes and diverge the app hash (chain halt). Sort the deduped store keys before emitting. This is deterministic and reproduces the store-key order the IterateRedelegations fallback already yields, so wired and unwired paths agree. Addresses the P1 raised in the Codex review of #184. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Locks in the fix for the map-iteration nondeterminism in redelegationsForValidator: the scoped scan now emits redelegations in store-key order, so MigrateValidatorDelegations replays them deterministically across nodes. The test writes 12 redelegations, forces the internal scoped scan (nil reds), and asserts the replay order matches the canonical store-key order. A reverted fix (raw map range) fails this essentially every run (12! possible orderings). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address code-review findings on PR #184: - Add the 12 validator-scoped-iteration tests to unit-evmigration.md and bump the EVMigration keeper count 124+ -> 136+ in tests.md, per the CLAUDE.md rule that new EVM tests be tracked in the test inventory. - Correct the stakingStoreHandle comments in keeper.go: it is no longer used exclusively by DeleteValidatorRecordNoHooks. redelegationsForValidator now reads it for validator-scoped redelegation index scans (read/write). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…alidator-scoped-iteration
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the evmigration validator-migration hot path so its cost scales with the migrating validator’s own state (delegations, redelegations, distribution rows) rather than full-chain iteration, and updates related tooling/docs plus registers a migration-only v1.20.1 hotfix upgrade.
Changes:
- Add validator-scoped KV iteration helpers for staking redelegations and distribution historical rewards/slash events, and use them in migration + migration-estimation query paths.
- Reuse pre-fetched staking records during
MigrateValidator(avoid redundant O(N) reads) and replace N+1 historical-rewards refcount updates with a single deterministic write. - Update
chain-helper.shto support--networkendpoint defaults, switch devnet uploader default gRPC port to15051, and register thev1.20.1upgrade handler (no store changes).
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| x/evmigration/keeper/query.go | Uses validator-scoped redelegation lookup in MigrationEstimate. |
| x/evmigration/keeper/query_test.go | Adds coverage asserting scoped redelegation counting in estimate query. |
| x/evmigration/keeper/msg_server_migrate_validator.go | Switches redelegation counting to scoped lookup and threads preloaded slices into V4. |
| x/evmigration/keeper/msg_server_migrate_validator_test.go | Updates validator-migration tests for scoped redelegations + refcount single-write behavior. |
| x/evmigration/keeper/msg_server_claim_legacy_test.go | Wires isolated staking store service in tests and adjusts V4 failure-path coverage. |
| x/evmigration/keeper/migrate_validator.go | Changes MigrateValidatorDelegations signature to accept preloaded staking records; uses single refcount set. |
| x/evmigration/keeper/migrate_validator_scoped.go | New scoped store iteration helpers for redelegations, historical rewards, and slash events. |
| x/evmigration/keeper/migrate_test.go | Adds extensive unit coverage for scoped iteration, redelegation determinism, and refcount optimization. |
| x/evmigration/keeper/migrate_distribution.go | Replaces Iterate-based historical-rewards lookup with keyed O(1) lookup helper. |
| x/evmigration/keeper/keeper.go | Adds wiring for distribution KVStoreService handle and expands staking handle usage to scoped reads. |
| tests/scripts/chain-helper.bats | Adds tests for network defaults / overrides behavior in chain-helper tooling. |
| scripts/chain-helper.sh | Introduces --network flag and network endpoint defaults; raises default cap to 2500. |
| docs/evm-integration/user-guides/tune-guide.md | Updates max_validator_delegations guidance to 2,500. |
| docs/evm-integration/user-guides/migration.md | Adds guidance about using Keplr + MetaMask post-migration; minor edits. |
| docs/evm-integration/user-guides/main.md | Adds a mainnet-readiness bullet about Keplr/MetaMask dual representation post-migration. |
| docs/evm-integration/testing/tests/unit-evmigration.md | Documents new/expanded unit tests for the optimizations. |
| docs/evm-integration/testing/tests.md | Updates aggregate unit test counts for evmigration keeper. |
| docs/evm-integration/evmigration/legacy-migration.md | Updates max_validator_delegations default value to 2500. |
| docs/devnet/main.md | Updates exposed ports list for devnet image (50051 → 15051). |
| docs/devnet/lumera-uploader.md | Updates uploader default gRPC port to 15051 and related examples. |
| docs/devnet/configuration.md | Updates devnet configuration docs for uploader gRPC port change. |
| devnet/scripts/lumera-uploader-setup.sh | Changes uploader default gRPC port to 15051 and config handling. |
| devnet/generators/docker-compose_test.go | Adds test ensuring default uploader gRPC port mapping is 15051. |
| devnet/generators/config.go | Updates default uploader gRPC port constant to 15051. |
| devnet/dockerfile | Updates EXPOSE ports (50051 → 15051). |
| devnet/config/validators.json | Updates uploader gRPC port to 15051. |
| devnet/config/config.json | Updates uploader gRPC port to 15051. |
| devnet/config/config-no-hermes.json | Updates network-maker/uploader gRPC port to 15051. |
| app/upgrades/v1_20_1/upgrade.go | Adds v1.20.1 migration-only upgrade handler. |
| app/upgrades/upgrades.go | Registers v1.20.1 in upgrade names and setup routing. |
| app/upgrades/upgrades_test.go | Adds test asserting v1.20.1 is migration-only (no store upgrades). |
| app/app.go | Wires distribution KVStoreService into evmigration keeper post-build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…no shadowing Resolves three Copilot review comments on PR #184: - query.go: MigrationEstimate now propagates a redelegationsForValidator error instead of swallowing it. A corrupt src/dst index would otherwise undercount redelegations, shrink totalRecords, and could flip WouldSucceed to true for a validator whose real footprint exceeds MaxValidatorDelegations. - chain-helper.sh: LUMERA_* env values (documented as overrides) now count as explicitly set, so passing --network alongside them fills only the endpoints the caller left unset rather than clobbering the env values. Adds a bats test. - migrate_validator_scoped.go: rename redelegation loop variables that shadowed the Keeper receiver `k`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MigrateActions — and MigrateValidatorActions, which delegates to it — scanned the entire action store on every account/validator migration, burning gas proportional to the global action count rather than the address's own footprint. On testnet this drove ClaimLegacyAccount txs to ~1.5B gas each; under a mainnet block gas cap they would be unminable. Resolve the affected actions through the existing creator and supernode secondary indexes instead, unioning the two result sets and deduping by action ID so an action is re-keyed exactly once even when the legacy address is both its creator and a supernode. Adds ActionKeeper.GetActionsByCreator / GetActionsBySuperNode backed by the index prefixes, mirroring the scoped-iteration approach PR #184 already applied to the staking and distribution steps. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DelegatorStartingInfo.Stake must hold tokens (TokensFromSharesTruncated),
matching the SDK's initializeDelegation. Both migration re-key paths wrote
raw shares. For any ever-slashed validator (exchange rate < 1, which passes
the only-currently-jailed pre-check), shares overstate the real stake, so the
next WithdrawDelegatorReward/undelegate/redelegate from any delegator panics
("calculated final stake greater than current stake") — permanently bricking
those operations for every affected delegator.
Fetch the target validator and convert shares -> tokens in both
MigrateValidatorDelegations (once, outside the loop — all delegations share the
re-keyed validator) and migrateActiveDelegations (per delegation, since each
targets a different third-party validator).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A validator that fell out of the active set purely on stake weight (Unbonded, not jailed) was permanently deadlocked: MsgMigrateValidator rejected both Unbonding and Unbonded, while MsgClaimLegacyAccount rejects any validator operator. Such an operator could recover neither legacy keys, funds, nor rewards, and had no economic path back across the active-set threshold. Narrow the guard to reject only Unbonding, whose live entry in the SDK unbonding-validator queue (keyed by completion time -> operator address) would be orphaned by Step V8's DeleteValidatorRecordNoHooks and halt the chain at maturity, since the module's StakingKeeper interface has no validator-queue methods. Unbonded is safe and is the recovery path: that queue entry was already dequeued (the transition that makes it Unbonded), so nothing is orphaned. MigrationEstimate now reports would_succeed=true for Unbonded non-jailed. Jailed stays blocked (checked first; recovery is unjail). Also in this change: - Remove MigrateClaim: claim records are no longer re-keyed during migration. The claim DB is frozen (claiming ended 2025-01-01) and kept for reference only; re-keying DestAddress was cosmetic (funds already moved) and scanning the ~18K-record store per migration was unbounded gas with no benefit. Drops the ClaimKeeper interface, keeper field, and depinject wiring. - Docs: record Bug #27 (delegation stake stored as tokens, not shares) and the 2026-07-03 testnet gas calibration; update the migration guides and the evmigration unit/integration test-index docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a realistic testnet-scale scenario for MsgMigrateValidator (~6000 delegation/unbonding/redelegation records in the observed val1 mix, seeded via the real StakingKeeper): - TestMigrateValidatorAtScale: pipeline correctness gate (~1.8s) asserting full re-keying at scale; guards the PR #184 hot-path optimizations against scale-only regressions. No timing assertion, so it is deterministic and does not flake in CI. - BenchmarkMigrateValidatorScaling: on-demand curve (1000/2000/4000/6000) measuring how cost scales; excluded from the pipeline by construction (go test runs no Benchmark* without -bench). Confirms linear ~38us/record scaling. Seeding advances block time per unbonding/redelegation record so their completion times land in distinct staking-queue buckets, matching a real chain. Without this, all records share one completion-time bucket and the migration's InsertUBDQueue / InsertRedelegationQueue re-serialize the whole bucket per entry (O(N^2)) — a seeding artifact that would mask the true linear per-record cost. Widens four shared sign/msg test helpers from *testing.T to testing.TB so both the Test and the Benchmark reuse them. Documents both in the EVM test index. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Release notes for the migration-only hotfix hardening x/evmigration for large validators and live-network conditions: validator/account migration performance (scoped index reads, single-write refcount, secondary-index action re-keying), correctness & safety fixes (tokens-not-shares stake, Unbonded-validator recovery, claim-record removal, deterministic redelegation replay), and the v1.20.1 upgrade handler. No store changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lts + analysis Adds the measured BenchmarkMigrateValidatorScaling results (1000/2000/4000/6000 records) and analysis to two places: - Test index (integration-evmigration.md): a Representative results table plus a linear-scaling read (~38us/record, slope ~1.0) with the gas-meterless-harness caveat and a cross-link to the gas design doc. - Gas design doc (2026-06-22-validator-migration-gas-design.md): a dated section noting the benchmark independently confirms the linear per-record scaling that the base + per_record*N formula assumes — the live devnet/testnet numbers were single-point and could not distinguish linear from super-linear. Includes the ns/op-vs-gas caveat and the completion-time-bucket O(N^2) worst case. PR #184 description updated with the same table + analysis. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mainnet now skips the v1.20.0 upgrade entirely and performs the EVM store additions + Lumera EVM param finalization through v1.20.1 instead, reusing v1.20.0's StoreUpgrades and handler verbatim so everything v1.20.0 does is guaranteed to run. Testnet and devnet (which already ran v1.20.0) keep it there and treat v1.20.1 as a migration-only hotfix. Mirrors the existing v1.8.0/v1.8.4 mainnet-skip precedent (setup-time params.ChainID gating). Adds a behavioral test that boots the app on a mainnet context and asserts v1.20.1 applies the Lumera EVM params and the migration deadline. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bilaltanveer38
approved these changes
Jul 6, 2026
Bilaltanveer38
left a comment
Bilaltanveer38
left a comment
There was a problem hiding this comment.
LGTM, just a minor comment, some integration test comments are stale. Example: tests/integration/evmigration/migration_test.go still says the old validator row is “orphaned / left dangling”, but current V8 deletes it via DeleteValidatorRecordNoHooks. Not a behavior issue, but it should be corrected to avoid misleading future reviewers.
mateeullahmalik
requested review from
Bilaltanveer38
and removed request for
Bilaltanveer38
j-rafique
previously approved these changes
Jul 6, 2026
…or-scoped-iteration
- Makefile.devnet: rename devnet-upgrade-1200 -> devnet-upgrade-1201 and point it (and the scripted devnet-evm-upgrade flow) at v1.20.1; drop the stale devnet-new-1111 target. - binaries.json: pin v1.20.0 to its final tag (was -rc5) and add a v1.20.1 entry (v1.20.1-rc1, bin-v1.20.1, supernode v2.6.0-rc1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Design for making the v1.20.1 upgrade carry the EVM bring-up based on chain state (fromVM + committed stores) rather than the IsMainnet chain-id branch, so a direct 1.12.0 -> 1.20.1 one-hop works on any network. Add-only store loader (no delete branch) + fromVM-gated handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Route the v1.20.1 EVM bring-up by chain STATE instead of the IsMainnet chain-id branch, so a direct 1.12.0 -> 1.20.1 upgrade works on any network (e.g. a devnet rehearsal of the mainnet one-hop), while remaining a no-op migration-only hotfix on chains that already ran v1.20.0. - New app/upgrades/v1_20_1 package: fromVM-gated handler. When the EVM module is absent from fromVM (EVM never brought up) it delegates to the full v1.20.0 bring-up; otherwise it runs migrations only. evmAlreadyInitialized predicate is unit-tested. - AddOnlyStoreLoader / computeAddOnlyStoreUpgrades: mount only the declared EVM store keys missing from committed state; never delete or rename. This is the safe half of AdaptiveStoreLoader (no Deleted branch), so a store-registration regression can never wipe a live store on mainnet. - SetupUpgrades: v1.20.1 declares the EVM store set + the state-driven handler on every network (IsMainnet branch removed). StoreLoaderForUpgrade routes v1.20.1 to the add-only loader regardless of the adaptive-store-manager env flag. - Tests: add-only computation, add-only routing (adaptive and non-adaptive), handler predicate, and SetupUpgrades routing across mainnet/testnet/devnet. TestV1201MigrationOnlyOnNonMainnet (asserted StoreUpgrade==nil) replaced by TestV1201CarriesEVMBringupOnAllNetworks; the mainnet full-bring-up integration test still passes through the new handler. Design: docs/design/v1_20_1-state-driven-evm-bringup-design.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
akobrin1
changed the title
- AddOnlyStoreLoader fails closed if it cannot read committed store names, instead of falling back to a blind UpgradeStoreLoader that could re-add already-present EVM stores and halt an already-upgraded chain during load. - v1.20.1 handler bases its decision on ALL four EVM bring-up modules, not just evm: none present -> full bring-up, all present -> migration-only hotfix, and partial state -> hard error (an impossible state that must not silently skip EVM param finalization). - Add a real rootmulti store-loader test: commit a pre-EVM store set, run AddOnlyStoreLoader, and assert the EVM stores get mounted while existing data survives (plus a no-op-when-present case) — exercising the loader through a genuine committed multistore rather than only the pure diff. - Handler tests updated for the all-present hotfix path and the partial-state fail-closed path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The devnet-evm-upgrade recipe still pinned EVM_VERSION=v1.20.0; bump it to v1.20.1 to match the renamed devnet-upgrade-1201 target and the v1.20.1 release wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The dependabot merge (#185, #186) re-tidied the root and tests/systemtests modules but not the nested devnet module. go mod tidy in devnet re-resolves the shared golang.org/x/* deps (crypto 0.49->0.51, net 0.52->0.55, sys, term, text) to the same versions now in root, carrying the x/net security bump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two comments in migration_test.go still described the old validator record as "orphaned / left dangling" with the rationale that RemoveValidator can't run on a bonded validator. V8 now deletes the old record via DeleteValidatorRecordNoHooks (a raw KV delete that avoids those hooks), so the record is gone, not orphaned. Comment-only; matches the existing V8 phrasing already used elsewhere in the file. Raised in PR review (Bilaltanveer38). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a-ok123
approved these changes
Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch started as a validator-migration performance fix and has grown into the v1.20.1 hotfix for the EVM migration rollout. It bundles four workstreams:
1.12.0 → 1.20.1one-hop performs the full EVM bring-up on any network; a chain that already ran v1.20.0 gets a no-op migration-only hotfix.chain-helper.sh --networkdefaults, devnet v1.20.1 wiring, uploader gRPC port, plus PR-review fixes and a dependabot merge.1. Validator-migration performance
MigrateValidator→MigrateValidatorDelegations/MigrateValidatorDistributionre-keys a validator from its legacy to its new address (delegations, unbonding, redelegations, distribution state). The original code walked full-chain indexes and re-read the same records multiple times — costs that blow up for a large validator on a busy chain.IterateValidatorHistoricalRewards/IterateValidatorSlashEvents/IterateRedelegations(O(total chain state)) with target-validator key-prefix reads via wired distribution/staking store handles. Falls back to theIterate*path when the store service is unwired (mock unit tests), so behavior is identical there.TestMigrateValidatorScopedIteration_SimulatesGlobalStateImprovementmodels 25 unrelated validators × 20 records and asserts a ~215× reduction in KV keys touched (1507 → 7).MaxValidatorDelegationspre-check already reads delegations/unbondings/redelegations; V4 no longer re-reads them (they are threaded in).1 + len(delegations), written once instead of N+1 full-chain scans.redelegationsForValidatorsorts the deduped store keys before returning, matching theIterateRedelegationsfallback byte-for-byte, soInsertRedelegationQueuereplay order can't diverge app hashes across nodes. Regression test:TestMigrateValidatorDelegations_RedelegationReplayIsDeterministic.Live-measured impact and scaling analysis:
test(evmigration): add validator-migration scaling test + benchmark+ the benchmark results doc.2. evmigration correctness fixes
BOND_STATUS_UNBONDEDvalidators to migrate — only jailed or still-BOND_STATUS_UNBONDINGvalidators are blocked (an unbonding validator still holds a live unbonding-validator-queue entry that re-keying would orphan). A validator that fell out of the active set on stake weight is now directly migratable. Estimate/rejection messages and the validator-migration guide updated to match.MigrateActionsvia the action secondary indexes instead of a full scan.3. State-driven v1.20.1 upgrade routing
ctx.ChainID()), not the--chain-idflag default.app/upgrades/v1_20_1): the handler runs the full v1.20.0 EVM bring-up when the EVM module is absent fromfromVM(never brought up) and is migration-only otherwise. This makes a direct1.12.0 → 1.20.1one-hop work on any network (e.g. a devnet rehearsal of the mainnet one-hop).AddOnlyStoreLoader) mounts only the declared EVM store keys missing from committed state and never deletes or renames a store — the safe half ofAdaptiveStoreLoader, so a store-registration regression can't wipe a live store on mainnet. Routed for v1.20.1 regardless of the adaptive-store-manager env flag.docs/design/v1_20_1-state-driven-evm-bringup-design.md.4. Tooling / devnet / housekeeping
chain-helper.sh:--network devnet|testnet|mainnetendpoint defaults;LUMERA_*env values are treated as explicit and are not clobbered by--network; bats coverage.devnet-upgrade-1200→devnet-upgrade-1201(+devnet-evm-upgradeflow);binaries.jsonpins v1.20.0 and adds a v1.20.1 entry; uploader default gRPC port moved.816c1d83):MigrationEstimatefails loud on a redelegation-count error instead of silently under-counting (which could flipWouldSucceedto true); env-vs---networkprecedence; loop-variable shadowing of the keeper receiver.master, picking up thegolang.org/x/netdependabot bumps (chore(deps): bump golang.org/x/net from 0.51.0 to 0.55.0 in /tests/systemtests #185, chore(deps): bump golang.org/x/net from 0.52.0 to 0.55.0 #186).Correctness & safety
1+N; new path writes1+Ndirectly.Iterate*returned; wired path used in production,Iterate*fallback covered by mock tests.Deleted/Renamed, so it can only add stores.fromVMpresence stay consistent.Testing
go test ./app/upgrades/...(incl. the mainnet full-bring-up integration test with-tags=test) — all pass.go test ./x/evmigration/keeper/...— pass.make lint— 0 issues.go build ./...— clean.