Add v6.7 retired module upgrade tests - #4040
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.33% 60.16% -1.17%
==========================================
Files 2184 2082 -102
Lines 191492 179858 -11634
==========================================
- Hits 117457 108218 -9239
+ Misses 62951 61546 -1405
+ Partials 11084 10094 -990
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>
|
This is plenty good! ship it alex! |
![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.
| tag=$$(go run ./upgradetest/cmd/boundary tag) && \ | ||
| tmp=$$(mktemp -d) && trap 'rm -rf "$$tmp"' 0; \ | ||
| go test -list '^Test' ./app 2>/dev/null | awk '/^Test/ { print }' | sort > "$$tmp/base"; \ | ||
| go test -tags "$$tag" -list '^Test' ./app 2>/dev/null | awk '/^Test/ { print }' | sort > "$$tmp/tagged"; \ |
There was a problem hiding this comment.
[suggestion] Discarding stderr here means a compile failure in the tagged file is reported as no tests were added by build tag upgrade_v67 (line 657) with no compiler output. Since app/upgrade_v67_test.go is compiled by nothing else in a normal run, a build break in it is exactly the failure this target is most likely to hit, and the message points at the wrong cause.
The same pattern appears in .github/scripts/offline-upgrade-test.sh:138 ("$UPGRADE_TAG $phase phase selected no offline test") and .github/scripts/release-upgrade-test.sh:145 ("build tag $UPGRADE_TAG defines no Test*CrossVersion assertion").
Suggest capturing stderr and echoing it when the listing comes back empty, rather than sending it to /dev/null.
| @set -e; \ | ||
| for file in app/upgrade_v*_test.go; do \ | ||
| [ -f "$$file" ] || continue; \ | ||
| case "$$file" in *_offline_source_test.go|*_offline_target_test.go) continue ;; esac; \ |
There was a problem hiding this comment.
[suggestion] upgrade-test-vet is described as the target that keeps already-shipped version-specific tests compiling, but it skips the two offline phase files. Nothing else compiles them either: go test ./... and golangci-lint cannot see them behind upgrade_v67 && offline_upgrade && upgrade_source|target, and offline-upgrade-test.sh only ever builds the current boundary's pair. So once v6.8 becomes the boundary, upgrade_v67_offline_{source,target}_test.go silently stop being type-checked and will rot.
The skip is understandable (the filename does not imply the full three-tag set), but the tag set is derivable — e.g. tag=$(basename "$file" _offline_source_test.go) plus ,offline_upgrade,upgrade_source — so these could be compiled too rather than excluded.
| require.Equal(t, want, resp.Value, "query /%s/key returned a different value", storeName) | ||
| require.NotNil(t, resp.ProofOps, "%s is missing from the commitment set", storeName) | ||
| require.NotEmpty(t, resp.ProofOps.Ops, "%s is missing from the commitment set", storeName) | ||
| return |
There was a problem hiding this comment.
[suggestion] The unconditional return at the end of the first iteration means this proves exactly one key — and which key is Go map iteration order, so it differs run to run. The function reads as though it proves every entry in snapshot, and its name says nothing about sampling.
Either drop the return and prove all keys, or make the intent explicit (pick a deterministic key, e.g. the lexicographically smallest, and say so in the doc comment). As written it would also trip staticcheck SA4004 if run.tests were ever enabled.
![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.
| commit="$(resolve_release "$version")" || | ||
| die "unable to resolve release/$version" | ||
| git -C "$REPO_ROOT" worktree add --detach "$worktree" "$commit" >&2 | ||
| fi |
There was a problem hiding this comment.
Vet target needs unfetched release refs
Medium Severity
make upgrade-test-vet now builds offline phases in worktrees of release/vMAJOR.MINOR, but the Go Test job that runs that target still checks out with fetch-depth: 1 and never fetches those branches first. resolve_release then has to git fetch origin release/v6.6 from a shallow clone, which can fail on pull-request checkouts (especially forks) or become a full-history fetch, so the compile step errors even when the tests themselves are fine.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e1fa6ad. Configure here.
2 participants
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