Add migrations for app.toml

[anilcse]

Summary

Now with x/upgrade, upgrades became seemless and less burden for app developers and node operators. One thing that is missing from automatic upgrades is updating app.toml when there are new/updated configs.

• app.toml changes require manual update.
• This also forces node operators to use manual upgrade instead of cosmovisor

Proposal

Add migrations for app.toml changes. It should basically cover the following cases:

• Append new config settings
• Update/Rename old configs (Also set default values when empty? Something like min-gas-price?)
• Delete deprecated configs

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[amaury1093]

Good idea, ACK.

[spoo-bar]

Proposal

A new command called pre-upgrade. This would be called by the cosmovisor during migration process after the binaries have been updated.
This cli command would be part of x/genutil/client/cli/

func PreUpgradeCmd(modules []module.AppModule) *cobra.Command {
	cmd := &cobra.Command{
		Use:   "pre-upgrade [target-version]",
		Short: "",
		Long: ""
		Args: cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {
			clientCtx := client.GetClientContextFromCmd(cmd)

			for _, module := module range modules {
				module.PreUpgrade(clientCtx)
			}
			return nil
		},
	}
	return cmd
}

This command accepts a list of modules and target-version to carry out the pre-upgrade.

Each module would implement its own PreUpgrade which will have access to clientCtx and therefore will be able to modify app data (as the toml config files) as needed by it before executing the application.

Since each module can modify the settings as desired, it is possible that the settings could end up in an undesirable state. The developers implementing the PreUpgrade need to be careful about their implementation.

Further Discussion needed

Currently for the migration of the modules, the versioning is handled through the value ConsensusVersion. This value is hardcoded. As such, migration from Consensus Version 1 → 2 is hardcoded.

[jgimeno]

@AmauryM @aaronc @robert-zaremba do you have some feedback on what @spoo-bar described? Thanks.

[alexanderbez]

A few things:

1. The operator can still technically use cosmovisor, they just can't have automatic restarts. So this proposal is definitely a good idea, but just wanted to point this out.
2. Re @spoo-bar's proposal, a few thoughts:

• Why does this have anything to do with modules? The upgrade handler in the app already takes care of module updates, no?
• The major use-case we're aiming to solve here is to do arbitrary "stuff" prior to starting the new binary. This includes migrating app.toml and maybe executing any external scripts. I think the pre-upgrade hook handler should really just point to series of scripts to execute...or maybe just a single script. This work would also exist in cosmosvisor, not the SDK I imagine. So cosmovisor would be aware if a pre-upgrade value is defined, and if so, it executes it.

[anilcse]

I think we cannot make it configurable if we gonna do it via cosmovisor, if so app developers might be forced to add migrations for all SDK related changes too. May be it can be best handled via rootCmd. Server's InterceptConfigsPreRunHandler can have migrations for all SDK related changes.

[alexanderbez]

I don't follow? Why can't this be done via cosmovisor?

[spoo-bar]

@alexanderbez The current upgrade module is used to update app modules, so any config changes which are part of an upgrade, I assumed, would be relevant to some module's upgrade.
Would there be situations where such arbitrary configs need to modified which aren't related to the queued modules upgrade?

[aaronc]

The pre-upgrade hook should be unrelated to modules. I am not yet seeing a case yet where it would apply.

As far as I know this pre-upgrade command should live in apps, not the SDK and we just need a convention for how cosmovisor will interact with it.

Will it be some script specified in the upgrade info cosmovisor receives? Will it be a convention? Probably the former makes sense because we need to have well defined failure/retry behavior.

Each app will need to define pre-upgrade hooks for each upgrade. I think it is pre-mature to assume we can generalize any more than that.

[alexanderbez]

so any config changes which are part of an upgrade, I assumed, would be relevant to some module's upgrade.

This is not true, at least not what my intuition tells me. The upgrade handler upgrades module state. Configurations and other misc. items are non-state related, so they don't happen as part of the upgrade handler.

As far as I know this pre-upgrade command should live in apps, not the SDK and we just need a convention for how cosmovisor will interact with it.

Essentially this is what we need to do. The simplest approach is to just provide an additional flag, e.g. --pre-upgrade-hook=/path/to/some/script.sh, to cosmovisor that it will call before starting the new binary. I'm sure there are other approaches we can take too.

[jgimeno]

What about the case where the cosmos sdk adds a new field in the app.toml? For example, I have a running application and I want to upgrade. The new SDK upgrade adds a new field into app.toml but I need to define the pre upgrade hook.

Do I need to put the updates coming from the SDK by hand into the hook? Isn't there any other way to make it automatic so I only need to take care of the changes that I need for my app?

[anilcse]

I don't follow? Why can't this be done via cosmovisor?

I was thinking about something extendable. My intention was to handle all the SDK related app.toml changes in the SDK and then app devs will add their changes on top of this. Cosmovisor may not be a best place if we are planning something like this. But it can execute a script to do this, which looks like an easy solution. But app devs would need to provide all the changes (including SDK changes) in the script I believe. Also not sure how we gonna preserve node operator settings if we are using cosmovisor (we can still do it, but sounds complex to me)

[alexanderbez]

What about the case where the cosmos sdk adds a new field in the app.toml? For example, I have a running application and I want to upgrade. The new SDK upgrade adds a new field into app.toml but I need to define the pre upgrade hook.

The script updates the app.toml...it can do whatever you want or define. That's one option. Also, another option is the app can also automatically add these fields to app.toml in some command initialization call.

I was thinking about something extedable. My intention was to handle all the SDK related app.toml changes in the SDK and then app devs will add their changes on top of this. Cosmovisor may not be a best place if we are planning something like this.

cosmovisor wouldn't not do this -- this isn't what I'm proposing. What I'm proposing is a pre-upgrade-hook script that can do whatever you need before the binary starts, e.g. Tendermint key migration, updating app.toml, etc..., and I'm proposing cosmivisor to automatically call this script.

[spoo-bar]

Update on the Proposal

This would handle any migration changes required by the SDK as wells as the application

1. SDK updates which might require any changes are added via sdk.Migration1,..(and so on) for each upgrade.
2. customHooks are passed in which also allow an app to specify any migrations they might want to handle.

type PreUpgradeHooks interface {
    Run(clientCtx client.Context)
}

func PreUpgradeCmd(customHooks []PreUpgradeHooks) *cobra.Command {
	cmd := &cobra.Command{
		Use:   "pre-upgrade [target-version]",
		Short: "",
		Long: ""
		Args: cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {
			clientCtx := client.GetClientContextFromCmd(cmd)

			sdkUpgradeHooks := []PreUpgradeHooks {
				sdk.Migration1,
				sdk.Migration2,
				// ...
				// ...
			}

			preUpgradeHooks = merge(sdkUpgradeHooks, customHooks)

			for _, hook := hook range preUpgradeHooks {
				hook.Run(clientCtx)
			}
			return nil
		},
	}
	return cmd
}

Cosmovisor can call this command before performing the upgrade.

[alexanderbez]

@spoo-bar what does this buy you over making a simple script? Also, there is no relationship between target version of SDK updates in your example.

[spoo-bar]

@alexanderbez In the solution which involves passing a script.sh to cosmovisor to be executed before the upgrade, it is not recommended to pass any cmd line args to cosmovisor. As all args received by cosmovisor are passed to the application.

[spoo-bar]

Additionally, this would mean the script would also need to include all the migrations which are SDK specific which application dev need to be aware of and missing something could cause failure.

[alexanderbez]

In the solution which involves passing a script.sh to cosmovisor to be executed before the upgrade, it is not recommended to pass any cmd line args to cosmovisor. As all args received by cosmovisor are passed to the application.

Yes, correct. The proposal is we add a flag to cosmosvisor to do this.

Additionally, this would mean the script would also need to include all the migrations which are SDK specific which application dev need to be aware of and missing something could cause failure.

Well, the migrations are agonistic. They can application or Tendermint or something completely else. We can't make any assumptions about what migrations are happening.

[jgimeno]

This all started from @anilcse, they updated the SDK in their application and when they run it it panicked because some field was not updated on the app.toml related to the cosmos SDK.

In the script approach looks like the developer needs to be aware that he needs to add this field, what @spoo-bar tries to solve is that the developer does not care if there is something updated from the SDK, that happens automatic and he can add more things (whatever it is) on top of it.

If we just want to execute something arbitrary before the upgrade then the script is enough, but does not solve the problem @anilcse faced.

[alexanderbez]

@aaronc or @robert-zaremba do you have any thoughts here?

[alexanderbez]

So I spoke with @aaronc and I see where there is some miscommunication. @spoo-bar we're not doing any state migrations and we're not touching or dealing with modules of any kind. We're just doing off-chain pre-upgrade "general" logic.

To summarize, we will have convention where cosmovisor executes a $ <appd> pre-upgrade command (note, this command doesn't exist in the SDK -- the application defines it) prior to starting the app. We then evaluate the following exit status codes:

• 0: success; continue
• 1: assume command does not exist; continue
• 30: assume command exists but failed; fail
• 31: assume command exists but we should retry; keep retrying until 0 or 30 is returned

This work is small scope and belongs 100% in cosmovisor (cc @robert-zaremba)

[aaronc]

Agree with what @alexanderbez said.

I would also add that pre-upgrade handler should basically look like what we're doing now for "pre-upgrades" of the multi-store: https://github.com/cosmos/gaia/blob/main/app/app.go#L428-L439. It would check for an upgrade name and skip-heights. So the pre-upgrade command will need to take --unsafe-skip-upgrades just like start does.

Also, it's an open question whether pre-upgrade should read the upgrade info file off the disk or get that passed to it by cosmovisor.

I think we should document how to do this in the SDK and provide any known migrations needed for things like app.toml in the SDK, but I don't think we can really generalized this pre-upgrade command. Apps will need to implement it for the upgrades they have planned.

[jgimeno]

Thanks for the clarification!

[aaronc]

Broke the Cosmovisor piece out into its own issue: #9973

[alexanderbez]

Thanks @aaronc. We can probably close this issue then in favor of #9973

[alexanderbez]

Closing in favor of #9973