From 8738a9bee247096da1dc72e2d2ba9c338a0cb739 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Fri, 12 Jul 2024 01:53:41 +0000 Subject: [PATCH 1/2] fix(cosmos): don't rerun store migrations on upgrade --- golang/cosmos/app/app.go | 9 +++-- golang/cosmos/app/upgrade.go | 67 ++++++++++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/golang/cosmos/app/app.go b/golang/cosmos/app/app.go index 1c1911f8218..52eb932eaa0 100644 --- a/golang/cosmos/app/app.go +++ b/golang/cosmos/app/app.go @@ -873,18 +873,23 @@ func NewAgoricApp( app.SetBeginBlocker(app.BeginBlocker) app.SetEndBlocker(app.EndBlocker) - for name := range upgradeNamesOfThisVersion { + for _, name := range upgradeNamesOfThisVersion { app.UpgradeKeeper.SetUpgradeHandler( name, unreleasedUpgradeHandler(app, name), ) } + // At this point we don't have a way to read from the store, so we have to + // rely on data saved by the x/upgrade module in the previous software. upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk() if err != nil { panic(err) } - if upgradeNamesOfThisVersion[upgradeInfo.Name] && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { + // Store migrations can only run once, so we use a notion of "primary upgrade + // name" to trigger them. Testnets may end up upgrading from one rc to + // another, which shouldn't re-run store upgrades. + if isPrimaryUpgradeName(upgradeInfo.Name) && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { storeUpgrades := storetypes.StoreUpgrades{ Added: []string{}, Deleted: []string{}, diff --git a/golang/cosmos/app/upgrade.go b/golang/cosmos/app/upgrade.go index 6fcd8996ad4..c142b027c6c 100644 --- a/golang/cosmos/app/upgrade.go +++ b/golang/cosmos/app/upgrade.go @@ -1,6 +1,8 @@ package gaia import ( + "fmt" + "github.com/Agoric/agoric-sdk/golang/cosmos/vm" swingsetkeeper "github.com/Agoric/agoric-sdk/golang/cosmos/x/swingset/keeper" sdk "github.com/cosmos/cosmos-sdk/types" @@ -8,15 +10,61 @@ import ( upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) -var upgradeNamesOfThisVersion = map[string]bool{ - "UNRELEASED_BASIC": true, // no-frills - "UNRELEASED_A3P_INTEGRATION": true, - "UNRELEASED_main": true, - "UNRELEASED_devnet": true, +var upgradeNamesOfThisVersion = []string{ + "UNRELEASED_BASIC", // no-frills + "UNRELEASED_A3P_INTEGRATION", + "UNRELEASED_main", + "UNRELEASED_devnet", + "UNRELEASED_REAPPLY", +} + +// isUpgradeNameOfThisVersion returns whether the provided plan name is a +// known upgrade name of this software version +func isUpgradeNameOfThisVersion(name string) bool { + for _, upgradeName := range upgradeNamesOfThisVersion { + if upgradeName == name { + return true + } + } + return false +} + +// validUpgradeName is an identity function that asserts the provided name +// is an upgrade name of this software version. It can be used as a sort of +// dynamic enum check. +func validUpgradeName(name string) string { + if !isUpgradeNameOfThisVersion(name) { + panic(fmt.Errorf("invalid upgrade name: %s", name)) + } + return name } +// isPrimaryUpgradeName returns wether the provided plan name is considered a +// primary for the purpose of applying store migrations for the first upgrade +// of this version. +// It is expected that only primary plan names are used for non testing chains. +func isPrimaryUpgradeName(name string) bool { + if name == "" { + // An empty upgrade name can happen if there are no upgrade in progress + return false + } + switch name { + case validUpgradeName("UNRELEASED_BASIC"), + validUpgradeName("UNRELEASED_A3P_INTEGRATION"), + validUpgradeName("UNRELEASED_main"), + validUpgradeName("UNRELEASED_devnet"): + return true + case validUpgradeName("UNRELEASED_REAPPLY"): + return false + default: + panic(fmt.Errorf("unexpected upgrade name %s", validUpgradeName(name))) + } +} + +// isFirstTimeUpgradeOfThisVersion looks up in the upgrade store whether no +// upgrade plan name of this version have previously been applied. func isFirstTimeUpgradeOfThisVersion(app *GaiaApp, ctx sdk.Context) bool { - for name := range upgradeNamesOfThisVersion { + for _, name := range upgradeNamesOfThisVersion { if app.UpgradeKeeper.GetDoneHeight(ctx, name) != 0 { return false } @@ -34,6 +82,13 @@ func unreleasedUpgradeHandler(app *GaiaApp, targetUpgrade string) func(sdk.Conte // These CoreProposalSteps are not idempotent and should only be executed // as part of the first upgrade using this handler on any given chain. if isFirstTimeUpgradeOfThisVersion(app, ctx) { + // The storeUpgrades defined in app.go only execute for the primary upgrade name + // If we got here and this first upgrade of this version does not use the + // primary upgrade name, stores have not been initialized correctly. + if !isPrimaryUpgradeName(plan.Name) { + return module.VersionMap{}, fmt.Errorf("cannot run %s as first upgrade", plan.Name) + } + // Each CoreProposalStep runs sequentially, and can be constructed from // one or more modules executing in parallel within the step. CoreProposalSteps = []vm.CoreProposalStep{ From d5c3380c9c22b7843bb21973092985284a3d8221 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Fri, 12 Jul 2024 02:10:03 +0000 Subject: [PATCH 2/2] doc: align with new upgrade.go refactoring --- MAINTAINERS.md | 2 +- a3p-integration/README.md | 2 +- a3p-integration/proposals/e:upgrade-next/README.md | 11 +++++++---- packages/deploy-script-support/README.md | 3 +-- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 7ef3fcbf856..dd38dbf27e2 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -46,7 +46,7 @@ The Release Owner and other appropriate stakeholders must agree on: - [ ] When a new release is planned, create a new branch from branch `release-mainnet1B` with a name like `dev-$releaseShortLabel` (example: `dev-upgrade-8`). This can be done from the command line or the [GitHub Branches UI](https://github.com/Agoric/agoric-sdk/branches). - [ ] Initialize the new branch for the planned upgrade: - - [ ] In **golang/cosmos/app/app.go**, update the `upgradeName` constants and the associated upgrade handler function name to correspond with the [_**upgrade name**_](#assign-release-parameters). + - [ ] In **golang/cosmos/app/upgrade.go**, update the `upgradeName` constants and the associated upgrade handler function name to correspond with the [_**upgrade name**_](#assign-release-parameters). Remove from the function any logic specific to the previous upgrade (e.g., core proposals). - [ ] Ensure that **a3p-integration/package.json** has an object-valued `agoricSyntheticChain` property with `fromTag` set to the [agoric-3-proposals Docker images](https://github.com/Agoric/agoric-3-proposals/pkgs/container/agoric-3-proposals) tag associated with the previous release (example: `use-upgrade-7`). diff --git a/a3p-integration/README.md b/a3p-integration/README.md index 0fd7dbfc5e6..0ef49b18dca 100644 --- a/a3p-integration/README.md +++ b/a3p-integration/README.md @@ -66,7 +66,7 @@ For a chain software upgrade proposal, the `type` is `"Software Upgrade Proposal particular, it can have a `coreProposals` field which instructs the chain software to run other core proposals in addition to the one configured in the chain software's upgrade handler (see `CoreProposalSteps` in - `/golang/cosmos/app/app.go`). + `/golang/cosmos/app/upgrade.go`). - See **Generating core-eval submissions** below for details. For an (evolving) example, see `a:upgrade-next` in master. diff --git a/a3p-integration/proposals/e:upgrade-next/README.md b/a3p-integration/proposals/e:upgrade-next/README.md index ff63376893f..e4f2a2e1f16 100644 --- a/a3p-integration/proposals/e:upgrade-next/README.md +++ b/a3p-integration/proposals/e:upgrade-next/README.md @@ -1,11 +1,14 @@ # Proposal to upgrade the chain software -The `UNRELEASED_A3P_INTEGRATION` software upgrade may include core proposals defined in -its upgrade handler. See `CoreProposalSteps` in the `unreleasedUpgradeHandler` -in [golang/cosmos/app/app.go](../../../golang/cosmos/app/app.go). +The `UNRELEASED_A3P_INTEGRATION` software upgrade may include core proposals +defined in its upgrade handler. See `CoreProposalSteps` in the +`unreleasedUpgradeHandler` in +[golang/cosmos/app/upgrade.go](../../../golang/cosmos/app/upgrade.go). This test proposal may also include `coreProposals` in its `upgradeInfo`, which are executed after those defined in that upgrade handler. See `agoricProposal` in [package.json](./package.json). -The "binaries" property of `upgradeInfo` is now required since Cosmos SDK 0.46, however it cannot be computed for an unreleased upgrade. To disable the check, `releaseNotes` is set to `false`. +The "binaries" property of `upgradeInfo` is now required since Cosmos SDK 0.46, +however it cannot be computed for an unreleased upgrade. To disable the check, +`releaseNotes` is set to `false`. diff --git a/packages/deploy-script-support/README.md b/packages/deploy-script-support/README.md index 3be02f0d117..048cff205fa 100644 --- a/packages/deploy-script-support/README.md +++ b/packages/deploy-script-support/README.md @@ -70,8 +70,7 @@ objects. The script describes how to build the core proposal. For `agoric-3-proposals` and uploading to the chain, the script must be named in the -`CoreProposalSteps` section in -[`app.go`](https://github.com/Agoric/agoric-sdk/blob/b13743a2cccf0cb63a412b54384435596d4e81ea/golang/cosmos/app/app.go#L881), +`CoreProposalSteps` section in [`upgrade.go`](../../golang/cosmos/app/upgrade.go), and its `defaultProposalBuilder` will be invoked directly. Script files should export `defaultProposalBuilder` and a `default` function