Add v6.7 retired module upgrade tests - #4040
Merged
Merged
Conversation
Cover the v6.7 removal of capability, feegrant, ibc and transfer, plus the oracle handler deprecation, at levels that trade control for realism. The existing tests call ApplyUpgrade directly, which never writes upgrade-info.json and never reloads the stores, so App.SetStoreUpgradeHandlers — the code deciding whether a store is dropped at an upgrade height — was reached by nothing. * Assert that transactions aimed at retired modules are rejected identically on both sides of the upgrade, that a rejected oracle transaction still pays its fee and occupies gas, and that retained store state survives the upgrade while being absent from an exported genesis. * Guard the whole module version map rather than the names v6.7 drops, so a later removal that forgets DeleteModuleVersion fails there instead of leaving an entry on chain for the life of the chain. Require every mounted store to be owned by a registered module or declared retained with a reason. * Drive a real governance upgrade on one throwaway node, reaching the halt, upgrade-info.json and store-reload path in seconds without docker. * Run the upgrade across two real binaries, seeding a spendable fee allowance and tallied oracle rates with v6.6.2 where those modules are still alive. This is the only layer whose pre-upgrade state is real, because the current binary can no longer create it. * Record how a live network answers before an upgrade and verify it after, as two phases with an artifact carried between them. * Add the four-node docker suite and its CI matrix row, and check every YAML suite parses without needing a cluster. processblock gains RunBlockDetailed and SignWithFeeGranter. Both are additive; existing callers are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Extend the four-validator two-binary runner with real feegrant and oracle state plus v6.7 retirement assertions. Remove the overlapping local, YAML, and live-network harnesses. Co-authored-by: Cursor <cursoragent@cursor.com>
Rely on the merged end-to-end test for upgrade orchestration and keep this branch limited to focused v6.7 in-process coverage and its test helpers. 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 #4040 +/- ##
==========================================
- Coverage 61.26% 60.15% -1.12%
==========================================
Files 2188 2082 -106
Lines 192239 179671 -12568
==========================================
- Hits 117779 108078 -9701
+ Misses 63298 61516 -1782
+ Partials 11162 10077 -1085
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Coordinate tagged app assertions with real two-binary release runs so each minor boundary can validate both handler behavior and persisted state. Co-authored-by: Cursor <cursoragent@cursor.com>
Compile phase-specific tests against v6.6 and v6.7 so CI verifies that the upgrade handler can reopen and migrate committed application state without starting nodes. Co-authored-by: Cursor <cursoragent@cursor.com>
The v6.7 coverage asserted the upgrade's bookkeeping over one clean pass:
that four names left the module version map, with the retired stores
seeded by invented keys. That says nothing about the state and funds the
upgrade leaves behind, and nothing about the paths an operator actually
hits when an upgrade goes wrong.
The persisted layer now builds its fixture through the v6.6 keepers, so a
real fee allowance, capability owner set, IBC client, connection, channel
and transfer denom trace cross the boundary, and the assertions name the
key that vanished rather than reporting that a map differs. Escrowed
native coins and ibc/ voucher balances are covered too, since both live
in the bank module and outlive the transfer module that created them.
Removal is now checked against the stores themselves: the full committed
key/value set of every retired store, its presence in the commitment set
behind the application hash, and the version-map bytes in the upgrade
store, so a later StoreUpgrades{Deleted} cannot pass as a bookkeeping
change.
Three failure paths gain coverage. An upgrade interrupted before its
commit replays to a byte-identical application hash. An un-upgraded
binary halts at the plan height instead of committing under the old
module set. A new reopen phase runs the v6.6 code against the migrated
database and records what an operator rolling back will see: the
database opens and the retained stores are readable, but the application
hash has already diverged and the first block panics on the missing
v6.7 handler.
UPGRADE_TEST_SNAPSHOT_HOME points the retained-state assertions at a real
node home, since no fixture substitutes for a snapshot.
Co-authored-by: Cursor <cursoragent@cursor.com>
The cross-version test waited for each validator to reach a height and then asserted it kept producing blocks. That is liveness: four nodes that each keep building while disagreeing on state is exactly a chain split, and every assertion passed on whichever node the callback queried. The after callback now requires all four validators to report the same application hash and the same block hash at the applied height and the three above it, so a migration that is not deterministic shows up as a disagreement naming the nodes and heights involved. A header carries its parent's resulting hash, so the upgrade block's own output is the height above the applied one, and the range covers it. The plan name went unchecked because every test scheduled its plan from the same constant the handler was registered from, which makes a mismatch unrepresentable. A proposal naming v6.7.0 where the binary registers v6.7 halts every validator with no binary able to resume, since BeginBlocker resolves handlers by exact name. Names built from app/tags are now required to resolve to handlers, and near-misses derived from the real name must halt instead of running it. That check is version-independent, so it sits in the untagged file and covers future boundaries. The persisted phases read the name back from the committed plan rather than a shared literal, and the live callback takes it from the proposal the chain recorded rather than the environment variable the runner intended. Co-authored-by: Cursor <cursoragent@cursor.com>
Nothing after the upgrade sent a transaction, so the suite could pass on a chain unable to execute one: every post-boundary assertion was a query, a store read, or a rejection, and liveness was block production, which continues on an empty chain. Worse, the harness could not have noticed. Every seid broadcast here uses --broadcast-mode sync, whose JSON is the CheckTx result, and the helper checking it treated code 0 as success. A transaction admitted to the mempool and then failing execution read as passing. The helper now takes the hash from the broadcast, waits for the transaction to appear in a block, and requires the delivered result, and the pre-upgrade seed transactions go through it too. Ordinary bank and EVM sends now run on both sides of the handler, so a failure separates an upgrade regression from something that never worked here. The harness configuration is pinned because it does not match the fleet. Layers one and two run with OCC disabled and DefaultConcurrencyWorkers: NewBaseApp never reads occ-enabled, and only the production command wires SetOccEnabled, which app.Setup does not. The application-hash agreement those layers establish is therefore agreement under a configuration nobody operates, and that is now asserted at each construction site rather than left to be rediscovered. The live validators are checked against the file each container boots, on every node, and rechecked after the binary swap. Pruning stays "nothing", which the suite's historical queries depend on; the assertion records that as a known divergence from a pruning validator rather than implying parity. Co-authored-by: Cursor <cursoragent@cursor.com>
Running the four-validator boundary for the first time failed twice, in ways no compile or single-process run could reach. Coin amounts were parsed as int64. The localnet carries 5e21 usei, so reading total supply aborted the before callback outright with a range error, and the balance and fee-allowance readers had the same latent fault waiting for any chain whose figures exceed an int64. Amounts now flow as sdk.Int, which is the type the chain itself uses; the int64 helper stays only for block heights, where it belongs. Restarting a validator diverted its output to a log of its own. The orchestrator recognises an upgrade halt by grepping a node's original log, so a validator this suite stopped and restarted before the upgrade appeared to exit without halting, and the run died there. A restart now appends to the log the cluster started the node with. An observed start keeps a separate log, truncated per launch, because that path greps for a halt and the node's original log already contains one from the coordinated upgrade; sharing a file there would let a stale line stand in for the halt under test. Co-authored-by: Cursor <cursoragent@cursor.com>
CrossVersion.ReleaseBinary advertises the path at which a validator keeps the binary it was running before the upgrade, but the runner created that copy on the primary node alone. That held while the only caller exported genesis through the node it already queried. It stops holding as soon as a test puts the old binary back on some other validator, which is what observing a rollback and an un-upgraded halt both require, and the run fails inside the callback with a missing file rather than at the point the assumption was made. Staging it on each node makes the runner match what the harness offers. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
This is plenty good! ship it alex! |
alexander-sei
marked this pull request as ready for review
September 3, 2026 18:39
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large, well-structured addition of v6.7 upgrade coverage (in-process, persisted offline two-process, and live four-validator paths) plus boundary discovery so CI selects the test set from app/tags instead of a hard-coded version. No blocking correctness problems found; the notes below are about diagnosability of the new discovery steps and a compile-coverage gap for the offline phase files.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
.github/scripts/release-upgrade-test.shdrops the previous cross-validator height-spread assertion (maximum - minimum <= 3) fromverify_post_upgradeand now only records each node's height. The replacement —CrossVersion.RequireBlockAgreement— is stronger, but it lives in the per-boundary tagged test, so a futureTest*CrossVersionthat forgets to call it loses the orchestrator's generic post-upgrade convergence check entirely. Consider keeping a cheap spread check in the orchestrator alongside the per-boundary assertions. - [suggestion]
.github/workflows/offline-upgrade-test.ymlpaths:omitsapp/app.go, whilerelease-upgrade-test.ymlincludes it. The offline test is the one that proves the retired stores stay mounted and readable across the boundary, and the mount list (kvStoreKeyNames) lives inapp/app.go, so a change that unmounts a retained store will not trigger this workflow. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This commit hardens upgrade validation by surfacing failures in app/test discovery, enforcing post-upgrade validator synchronization, and compiling historical offline upgrade phases against their release refs to avoid API mismatches. It also makes the offline proof check validate the lexicographically first snapshot entry and resets the upgrade registry after asserting orphaned module versions are absent.
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.
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 e1fa6ad. Configure here.
This change keeps offline source fixtures under app/testdata so current-branch module discovery does not resolve APIs removed by the target upgrade. The upgrade scripts, scaffolding, and CI workflows were updated to copy and validate the staged source fixture correctly, and the current boundary checks now account for the new layout. It also tightens ABCI response parsing to reject out-of-range uint32 values and adds regression tests for the fixture pathing and validation behavior.
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
Re-review of the v6.7 upgrade coverage PR: all three previously-flagged inline findings (discarded stderr on empty test listings, upgrade-test-vet skipping the offline phase files, and the non-deterministic single-key proof check) are addressed, as are the two non-inline notes (the cross-validator height-spread assertion is back in verify_post_upgrade, and app/app.go is now in the offline workflow's paths). No blocking correctness problems found in the current diff.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
make upgrade-test-vetnow runs on the criticalRace Detectionjob ingo-test.yml, and its cost grows linearly with every shipped minor release:upgradetest/compile_offline.shdiscovers everyapp/testdata/upgrade_v*_offline_source_test.go/app/upgrade_v*_offline_target_test.gopair and, for each historical one, creates arelease/vX.Yworktree and does a full-racecompile of./appon both sides (plus one more compile perapp/upgrade_v*_test.goin the first loop). With v6.7 alone that is ~3 extra app builds; nothing prunes older pairs, so each release adds ~3 more to the required PR job. Consider movingupgrade-test-vet(and thefetch-depth: 0it forces on that job) into its own workflow/job, or bounding how far back offline phases are recompiled.
masih
approved these changes
Sep 4, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
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
2 participants
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
app/tagsinstead of being hard-coded in CI.Cross-version coverage
release/v6.6, commits an application database, then compiles its target phase onrelease/v6.7and reopens that same database.seidper ref, creates fixtures with v6.6, performs the governance halt and binary replacement across four validators, and runs the tagged v6.7 assertions against the same node homes.Test plan
go test ./app -count=1 -run 'Test(V67|LatestUpgradeLeavesNoOrphanedModuleVersions|MountedStoresAreOwnedOrExplicitlyRetained|RetainedStoresRemainMounted|RetiredOracle)'go test ./app -count=1 -run '^TestOracleKeepsWritingStateAfterV67$'go test ./testutil/processblock/... -count=1go test ./upgradetest/...make upgrade-test-vetFROM_REF=release/v6.6 TO_REF=release/v6.7 bash .github/scripts/offline-upgrade-test.shmake fmtcheckMade with Cursor