docs: simply ADR-47 #16150
docs: simply ADR-47 #16150
Conversation
github-prbot
requested review from
a team,
samricotta and
testinginprod
and removed request for
a team
| @@ -3,28 +3,28 @@ | |||
| ## Changelog | |||
|
|
|||
| * Nov, 23, 2021: Initial Draft | |||
| * Feb 15, 2023: Remove `pre_run` and `post_run` fields for simplifying the ADR | |||
There was a problem hiding this comment.
This does not seem needed anymore, is that a correct assumption?
There was a problem hiding this comment.
This ADR has been in the back of my mind for the last year+. To me, the only thing really worthwhile in it, is the pre_run and post_run stuff. Cosmovisor currently has support for running a pre-upgrade command on the new executable. Support for running an externally defined command before and/or after an upgrade would still be handy, though. An example pre-run is updating config files to a new format (e.g. when all the -s change to _s). An example post-run is sending a notification somewhere that a node is back up.
That being said, the post_run concept might be better as something a node can configure rather than something all nodes on a chain would run. I can't remember all of the use-cases previously thought of for it, though.
As for the Artifacts defined in this PR, I actually now think it's something that I do not want. We (at Provenance Blockchain) put a URL in the info field that returns the UpgradeConfig JSON. We've had a few instances where an upgrade proposal is submitted and passed, but before the upgrade height, we discover a major issue. Because the actual executable download URLs aren't in the proposal, we're able to change them to a version without the issue. Without this ability, it'd often be impossible to prevent the chain from upgrading to the problematic version.
If the pre_run and post_run parts are being removed from this ADR, I'd suggest getting rid of the ADR altogether.
There was a problem hiding this comment.
Exactly, the fact cosmovisor can run a pre-upgrade command on the new binary if it exists, makes the pre_run field unnecessary now.
Agreeing as well with your second point that post_run is something that nodes can configure.
Bringing a new breakage client breakage just for a sake of improving that info field, is a nit, so we may indeed simply reject/retract the ADR.
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
| The upgrade instructions will contain a list of artifacts available for each platform. | ||
| It allows for the definition of a pre-run and post-run commands. | ||
| These commands are not consensus guaranteed; they will be executed by Cosmosvisor (or other) during its upgrade handling. | ||
| The `x/upgrade.Plan` message is updated for providing upgrade artifacts. The artifacts available define each platform. |
There was a problem hiding this comment.
Since this is discussing things not yet done, it should be future tense. Also, the second sentence is weird to me. I'm not quite sure what it's trying to say.
| The `x/upgrade.Plan` message is updated for providing upgrade artifacts. The artifacts available define each platform. | |
| The `x/upgrade.Plan` message will be updated to provide a list of artifacts, one for each available platform. |
| 1. The `{checksum}` from the query parameter MUST equal the `checksum` provided in the `Artifact`. | ||
| 1. The `{checksum_algo}` from the query parameter MUST equal the `checksum_algo` provided in the `Artifact`. | ||
| 1. The following validation is currently done using the `info` field. We will apply similar validation to the `UpgradeInstructions`. | ||
| 1. There MUST be at least one entry in `artifacts`. |
There was a problem hiding this comment.
This contradicts the claim that the new artifacts are optional.
| We will deprecate the use of the `info` field for anything other than human readable information. | ||
| A warning will be logged if the `info` field is used to define the assets (either by URL or JSON). | ||
| The `info` field should be deprecated for anything other than human readable information. | ||
| A warning is logged if the `info` field is used to define the assets (either by URL or JSON). |
There was a problem hiding this comment.
After thinking about this for the last year+, I don't think this deprecation is a good idea. The specific functionality being removed that is still needed, is the ability to provide a URL that returns the artifact download URLs. The new artifacts stuff requires the artifact download URLs to be directly in the upgrade plan.
| * [Current upgrade.proto](https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/upgrade/v1beta1/upgrade.proto) | ||
| * [Upgrade Module README](https://github.com/cosmos/cosmos-sdk/blob/main/x/upgrade/README.md) | ||
| * [Cosmovisor README](https://github.com/cosmos/cosmos-sdk/blob/main/tools/cosmovisor/README.md) | ||
| * [Pre-upgrade README](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/migrations/01-pre-upgrade.md) |
There was a problem hiding this comment.
The Pre-upgrade README link is incorrect. The old one still works, but I'm not sure where it was moved to now.
Also, these links need to be to specific version tags and not to /main/ (or any other branch). They're here to provide context which is lost if the content changes later, or worse, the file moves and the link no longer works.
|
|
||
| ## Context | ||
|
|
||
| The `upgrade` module in conjunction with Cosmovisor are designed to facilitate and automate a blockchain's transition from one version to another. | ||
|
|
||
| Users submit a software upgrade governance proposal containing an upgrade `Plan`. | ||
| The [Plan](https://github.com/cosmos/cosmos-sdk/blob/v0.44.5/proto/cosmos/upgrade/v1beta1/upgrade.proto#L12) currently contains the following fields: | ||
| The [Plan](https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/upgrade/v1beta1/upgrade.proto#L13-L45) currently contains the following fields: |
There was a problem hiding this comment.
This link needs to be to a specific version instead of a link to whatever main currently has, specially since it include line numbers.
My vote is to simply reject it. |
Let's just do that then 👍🏾 |
Description
Extracted from #14878.
Opening this PR with a simplification of ADR-47 to get a feeling on what to do with this ADR.
Related: #14878, #11875, #11543, #10718
If we go with it, I can continue #14878 and wrap up the 4 issues above.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change