-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Fix v0.45->v0.46 migration #12028
Conversation
@@ -19,6 +19,10 @@ func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.Binar | |||
} | |||
|
|||
func migrateParamsStore(ctx sdk.Context, paramstore paramtypes.Subspace) { | |||
paramstore.WithKeyTable(types.ParamKeyTable()) | |||
paramstore.Set(ctx, types.KeyMinCommissionRate, types.DefaultMinCommissionRate) | |||
if paramstore.HasKeyTable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encountered the same bug as in #9484, so applied the same fix
Is there a way of adding this inside the upgrade handler somehow? Has this happened before (the fact that we need to run another separate command for an upgrade)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
// RegisterUpgradeHandlers is used for registering any on-chain upgrades. | ||
// Make sure it's called after `app.mm` and `app.configurator` are set. | ||
app.RegisterUpgradeHandlers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be communicated in the release notes too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release notes imo are brief & high-level (see proposed defs in #11587).
How about int he UPGRADING.md document?
// ref: https://github.com/tendermint/tendermint/blob/master/UPGRADING.md#database-key-format-changes | ||
func makeKeyMigrateCmd() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "key-migrate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of adding this inside the upgrade handler somehow? Has this happened before (the fact that we need to run another separate command for an upgrade)?
@facundomedica In the upgrade handler, no. See the original issue, we need to run the key migration before starting tendermint, so way before the upgrade handler is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative solution, the SDK's start
command is a combination of TM's key-migrate
+ start
.
key-migrate
is idempotent, so it can be run multiple times.
However, if we decide to show logs, as we do in this PR and as it's done in tendermint, then those logs will be shown on each startup:
11:55AM INF beginning a key migration dbctx=blockstore num=1 total=6
11:55AM INF beginning a key migration dbctx=state num=2 total=6
11:55AM INF beginning a key migration dbctx=peerstore num=3 total=6
11:55AM INF beginning a key migration dbctx=tx_index num=4 total=6
11:55AM INF beginning a key migration dbctx=evidence num=5 total=6
11:55AM INF beginning a key migration dbctx=light num=6 total=6
Any opinions on separate key-migrate
and start
commands VS one start
command which runs key-migrate
under the hood?
serverCtx := GetServerContextFromCmd(cmd) | ||
config := serverCtx.Config | ||
|
||
contexts := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm shocked we have to actually write any substantial code here. I would've presumed Tendermint would've exposed a command that'll do all of this for us?
In any case, does Tendermint not expose the contexts we want to migrate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TM exposes:
But it needs additional flags to get the DB dir right. In this PR, we use the same code, but with serverCtx
.
I couldn't think of less code re-use, best I could do was to mention this was copy-pasted from tendermint - like other commands in that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Such an easy problem to solve if the TM command just had a function that the command called instead. Sigh...
I don't like that we have to implement the command in such a manner, but if there's no other choice, then oh well. |
tendermint should of made it automatic, imo. having a command like this will cause sooooooooo many problems. |
Is this a better UX for sdk users? #12028 (comment) |
I'll automerge this, so that we can fix the network issues more easily, e.g. have #12067 also incorporate these changes here. |
## Description closes #12027 - Add `tendermint key-migrate` subcommand - Fix in-place store migrations Test: - on v0.45 node, make a proposal to update software, make it pass - wait for node to halt - on v0.46 binary, run `simd tendermint key-migrate` - on v0.46 binary, run `simd start --mode validator` - make sure the v0.46 node runs correctly --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit ca0b8f9) # Conflicts: # simapp/upgrades.go
* fix: Fix v0.45->v0.46 migration (#12028) ## Description closes #12027 - Add `tendermint key-migrate` subcommand - Fix in-place store migrations Test: - on v0.45 node, make a proposal to update software, make it pass - wait for node to halt - on v0.46 binary, run `simd tendermint key-migrate` - on v0.46 binary, run `simd start --mode validator` - make sure the v0.46 node runs correctly --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit ca0b8f9) # Conflicts: # simapp/upgrades.go * fix conflicts * Update changelog Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
* fix: Fix v0.45->v0.46 migration (cosmos#12028) closes cosmos#12027 - Add `tendermint key-migrate` subcommand - Fix in-place store migrations Test: - on v0.45 node, make a proposal to update software, make it pass - wait for node to halt - on v0.46 binary, run `simd tendermint key-migrate` - on v0.46 binary, run `simd start --mode validator` - make sure the v0.46 node runs correctly --- *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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed *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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit ca0b8f9) * fix conflicts * Update changelog Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
## Description closes cosmos#12027 - Add `tendermint key-migrate` subcommand - Fix in-place store migrations Test: - on v0.45 node, make a proposal to update software, make it pass - wait for node to halt - on v0.46 binary, run `simd tendermint key-migrate` - on v0.46 binary, run `simd start --mode validator` - make sure the v0.46 node runs correctly --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
* fix: Fix v0.45->v0.46 migration (cosmos#12028) ## Description closes cosmos#12027 - Add `tendermint key-migrate` subcommand - Fix in-place store migrations Test: - on v0.45 node, make a proposal to update software, make it pass - wait for node to halt - on v0.46 binary, run `simd tendermint key-migrate` - on v0.46 binary, run `simd start --mode validator` - make sure the v0.46 node runs correctly --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit ca0b8f9) # Conflicts: # simapp/upgrades.go * fix conflicts * Update changelog Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Description
closes #12027
tendermint key-migrate
subcommandTest:
simd tendermint key-migrate
simd start --mode validator
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.md
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...
!
in the type prefix if API or client breaking change