feat!: update x/upgrade to handle additional instructions

[robert-zaremba]

Description

Closes: #10047

This is an alternative to solution to the one drafted in #10006

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[robert-zaremba]

If you think about a better name for this attribute, please share your proposals. I was also thinking about Setup, Preparation ...
We should not confuse with the Info field which is already overloaded (both in meaning and usage).

[spoo-bar]

Considering it contains both pre and post upgrade info, UpgradeInstructions sounds good.

[orijbot]

Visit https://dashboard.github.orijtech.com?back=0&pr=10032&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details.

[jgimeno]

Do you know any use case for this? Also how would you deal with the errors on the post upgrade? The application is already running in theory but your post upgrade script fails. What would you do?

[robert-zaremba]

One possible use-case is reporting.
But maybe it's not needed. I'm happy to remove it if you think it may confuse people or if it will be abused.

[anilcse]

I believe this post-run script can be used for non-critical activities, so we don't need to handle errors. We can use it for removing backups, alerting users etc

[spoo-bar]

As we use go-getter to download the files in cosmovisor, it automatically checks for the checksum from the download url as ./foo.sh?checksum=b7d96c89d09d9e204f5fedc4d5d55b21. So a seperate checksum property might not be needed

[robert-zaremba]

My intention was to make it universal. Proto files should not assume that we are using the go implementation or cosmovisor. We could put the checksum in the URL, but then we would need to explain it in a comment and require it when registering a new proposal.

[tychoish]

I think we should be more clear as to the execution environment of these commands:

• which shell? Preferably we wouldn't allow people to write full on shell scripts, but if so we should be clear about this expectation.
• what's the working directory should this be configurable?
• should we provide some way to pass environment variables to commands?

[robert-zaremba]

Maybe we can drop "shell" and just say "command"?
It's up to the app developers to make a system requirements.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dwedul-figure]

With the addition of the pre_run and post_run fields, there will be a need to download those as well. By being able to provide a name for assets, users won't be constrained to only providing archives when they need to include a special pre_run script.

I feel like adding a string name field to the Asset is the most versatile solution. We'd state that entries in repeated Asset assets SHOULD be unique by name and platform (as opposed to just platform).

Alternatively, a hierarchy could be used, but there's a couple ways to do that, either name -> platforms or platform -> names. There's good arguments for either way though. But I feel like leaving it flat allows users to handle it the way that makes the most sense for them.

Cosmovisor Usage

Assets would be downloaded in the order provided but only if there is a Source entry that matches the system's platform (or there's a platform = any entry for it). Basically, we'd loop through the assets, if the platform matches the system, then it's downloaded. Or if the platform is any and the name is one that hasn't yet been downloaded this upgrade, then it'll be downloaded.

If the url returns an archive, then the name becomes a directory name. Otherwise it's a filename. Either way, the full path to the downloaded result would be {DAEMON_HOME}/{upgrade name}/{name}. Sub directories would be allowed in the name, but we'd need to discuss whether or not to allow it to contain ../.

I'd make a couple special cases for the name value:

1. If the name is empty, and the url returns an archive, it is unpacked and becomes {DAEMON_HOME}/{upgrade name} (similar to current behavior). If the name is empty and the url returns a non-archive file, it'll end up as {DAEMON_HOME}/{upgrade name}/bin/{DAEMON_NAME}.
2. If the name is . then the url will need to return an archive and it'll be unpacked into {DAEMON_HOME}/{upgrade name}. If the name is . and the url returns something other than an archive, an error will be given and the upgrade/download process will be interrupted.

We'd probably want to maintain the current functionality where, after doing the downloads, if {DAEMON_HOME}/{upgrade name}/bin/{DAEMON_NAME} doesn't exist, but {DAEMON_HOME}/{upgrade name}/{DAEMON_NAME} does, the latter is copied to the former.

[dwedul-figure]

It might actually be desirable to extract the platform field all the way out to the UpgradeInstructions message and make the UpgradeInstructions Upgrade a repeated field with a comment that it SHOULD have only one entry per platform. This would allow for different pre/post-run commands that can be tailored for specific environments.

Also, in order to help minimize on-chain data, should we throw this in a oneof and allow a url to be provided in place of the full structure here? The idea is that the url should return the same structure, but really, for chains that don't use Cosmovisor, it could return whatever they want. If the UpgradeInstructions are provided directly, they'll be serialized as json and added to the upgrade-info.json file in the upgrade field. If a url is provided, that url will be added to the upgrade-info.json file as upgrade_url.

Lastly, I feel like this field would be better named instructions. The proto file is named upgrade.proto and so is the module for it. The message the field is in is Plan but is sometimes called an "upgrade plan." The info field is sometimes called "upgrade info." Basically, the word "upgrade" is already used in a few ways and having a field with that name can easily cause more confusion.

[dwedul-figure]

Also, in order to help minimize on-chain data, should we throw this in a oneof and allow a url to be provided in place of the full structure here? The idea is that the url should return the same structure, but really, for chains that don't use Cosmovisor, it could return whatever they want. If the UpgradeInstructions are provided directly, they'll be serialized as json and added to the upgrade-info.json file in the upgrade field. If a url is provided, that url will be added to the upgrade-info.json file as upgrade_url.

I was thinking about this part a bit further with extra consideration for the first upgrade using new definitions.

If we allow the a url in the info field to return the newly defined UpgradeInstructions structure (e.g. as JSON), then as long as the nodes are running updated versions of Cosmovisor, an update can be done using these new proto fields without needing the chain software to already be using the new version. Otherwise, these new fields cannot be used for the next upgrade; they can only be used for upgrades after the one that adds these fields to the protos.

The downside is the further overloading of the info field.

[dwedul-figure]

I think we can remove this description field.

If someone provides UpgradeInstructions in the Plan then the info field can then be used for this. If UpgradeInstructions aren't included in the Plan then this field wouldn't be there to populate.

I guess that if the UpgradeInstructions are included, then the info field is ignored (as far as automated processing goes).

[dwedul-figure]

PR for related DRAFT ADR: #10602

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.