Add v6.7 retired module upgrade tests - #4040
Draft
alexander-sei wants to merge 10 commits into
Draft
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4040 +/- ##
==========================================
- Coverage 61.24% 60.25% -0.99%
==========================================
Files 2153 2054 -99
Lines 188393 176716 -11677
==========================================
- Hits 115373 106487 -8886
+ Misses 62289 60462 -1827
+ Partials 10731 9767 -964
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 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
1 participant
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