Skip to content
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.

Sign up for GitHub

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

Merged
merged 13 commits into from
May 30, 2022
Merged

fix: Fix v0.45->v0.46 migration #12028

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f7f181b
fix: Fix v0.45->v0.46 migration
amaury1093 May 24, 2022
543fe08
Add comment
amaury1093 May 24, 2022
11a48e4
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/fix…
amaury1093 May 24, 2022
9884ac5
Make migration work
amaury1093 May 24, 2022
fbb5bab
changelog
amaury1093 May 24, 2022
3146574
Add comment
amaury1093 May 24, 2022
8a8b57b
private function
amaury1093 May 24, 2022
73dfe79
Update server/tm_cmds.go
amaury1093 May 24, 2022
55ae6b7
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/fix…
amaury1093 May 25, 2022
881179a
Merge branch 'main' into am/fix-045-046-mig
alexanderbez May 26, 2022
d8b5f0c
Merge branch 'main' into am/fix-045-046-mig
amaury1093 May 27, 2022
f9e46fb
Merge branch 'main' into am/fix-045-046-mig
amaury1093 May 27, 2022
26eebaf
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/fix…
amaury1093 May 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* (cli) [#12028](https://github.com/cosmos/cosmos-sdk/pull/12028) Add the `tendermint key-migrate` to perform Tendermint v0.35 DB key migration.

### Bug Fixes

* (migrations) [#12028](https://github.com/cosmos/cosmos-sdk/pull/12028) Fix v0.45->v0.46 in-place store migrations.

## [v0.46.0-rc1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.0-rc1) - 2022-05-23

### Features

* (types) [#11985](https://github.com/cosmos/cosmos-sdk/pull/11985) Add a `Priority` field on `sdk.Context`, which represents the CheckTx priority field. It is only used during CheckTx.
* (gRPC) [#11889](https://github.com/cosmos/cosmos-sdk/pull/11889) Support custom read and write gRPC options in `app.toml`. See `max-recv-msg-size` and `max-send-msg-size` respectively.
* (cli) [\#11738](https://github.com/cosmos/cosmos-sdk/pull/11738) Add `tx auth multi-sign` as alias of `tx auth multisign` for consistency with `multi-send`.
Expand Down
68 changes: 68 additions & 0 deletions server/tm_cmds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ package server
// DONTCOVER

import (
"context"
"fmt"

"github.com/spf13/cobra"
cfg "github.com/tendermint/tendermint/config"
pvm "github.com/tendermint/tendermint/privval"
"github.com/tendermint/tendermint/scripts/keymigrate"
"github.com/tendermint/tendermint/scripts/scmigrate"
tversion "github.com/tendermint/tendermint/version"
"sigs.k8s.io/yaml"

Expand Down Expand Up @@ -123,3 +127,67 @@ func VersionCmd() *cobra.Command {
},
}
}

// MakeKeyMigrateCommand is ported from tendermint's key-migrate command, but
// uses the SDK's own server.Context.
// ref: https://github.com/tendermint/tendermint/blob/master/UPGRADING.md#database-key-format-changes
func MakeKeyMigrateCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "key-migrate",
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Short: "Run TendermintDatabase key migration",
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
RunE: func(cmd *cobra.Command, args []string) error {
ctx, cancel := context.WithCancel(cmd.Context())
defer cancel()

serverCtx := GetServerContextFromCmd(cmd)
config := serverCtx.Config

contexts := []string{
Copy link
Contributor

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?

Copy link
Contributor Author

@amaury1093 amaury1093 May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TM exposes:

https://github.com/tendermint/tendermint/blob/f33722b4233159a31cdf6c33258fbc601cdbd1d4/cmd/tendermint/commands/key_migrate.go#L15

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.

Copy link
Contributor

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...

// this is ordered to put the
// (presumably) biggest/most important
// subsets first.
"blockstore",
"state",
"peerstore",
"tx_index",
"evidence",
"light",
}

for idx, dbctx := range contexts {
serverCtx.Logger.Info("beginning a key migration",
"dbctx", dbctx,
"num", idx+1,
"total", len(contexts),
)

db, err := cfg.DefaultDBProvider(&cfg.DBContext{
ID: dbctx,
Config: config,
})

if err != nil {
return fmt.Errorf("constructing database handle: %w", err)
}

if err = keymigrate.Migrate(ctx, db); err != nil {
return fmt.Errorf("running migration for context %q: %w",
dbctx, err)
}

if dbctx == "blockstore" {
if err := scmigrate.Migrate(ctx, db); err != nil {
return fmt.Errorf("running seen commit migration: %w", err)

}
}
}

serverCtx.Logger.Info("completed database migration successfully")

return nil
},
}

return cmd
}
1 change: 1 addition & 0 deletions server/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ func AddCommands(rootCmd *cobra.Command, defaultNodeHome string, appCreator type
tmcmd.ResetAllCmd,
tmcmd.ResetStateCmd,
tmcmd.InspectCmd,
MakeKeyMigrateCommand(),
)

startCmd := StartCmd(appCreator, defaultNodeHome)
Expand Down
7 changes: 4 additions & 3 deletions simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,6 @@ func NewSimApp(
// set the governance module account as the authority for conducting upgrades
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String())

// RegisterUpgradeHandlers is used for registering any on-chain upgrades
app.RegisterUpgradeHandlers()

app.NFTKeeper = nftkeeper.NewKeeper(keys[nftkeeper.StoreKey], appCodec, app.AccountKeeper, app.BankKeeper)

// create evidence keeper with router
Expand Down Expand Up @@ -408,6 +405,10 @@ func NewSimApp(
app.configurator = module.NewConfigurator(app.appCodec, app.MsgServiceRouter(), app.GRPCQueryRouter())
app.mm.RegisterServices(app.configurator)

// RegisterUpgradeHandlers is used for registering any on-chain upgrades.
// Make sure it's called after `app.mm` and `app.configurator` are set.
app.RegisterUpgradeHandlers()
Comment on lines +393 to +395
Copy link
Collaborator

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

Copy link
Contributor Author

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?


// add test gRPC service for testing gRPC queries in isolation
testdata_pulsar.RegisterQueryServer(app.GRPCQueryRouter(), testdata_pulsar.QueryImpl{})

Expand Down
30 changes: 1 addition & 29 deletions simapp/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,35 +17,7 @@ const UpgradeName = "v045-to-v046"

func (app SimApp) RegisterUpgradeHandlers() {
app.UpgradeKeeper.SetUpgradeHandler(UpgradeName,
func(ctx sdk.Context, plan upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) {
// We set fromVersion to 1 to avoid running InitGenesis for modules for
// in-store migrations.
//
// If you wish to skip any module migrations, i.e. they were already migrated
// in an older version, you can use `modulename.AppModule{}.ConsensusVersion()`
// instead of `1` below.
//
// For example:
// "auth": auth.AppModule{}.ConsensusVersion()
fromVM := map[string]uint64{
"auth": 1,
"authz": 1,
"bank": 1,
"capability": 1,
"crisis": 1,
"distribution": 1,
"evidence": 1,
"feegrant": 1,
"gov": 1,
"mint": 1,
"params": 1,
"slashing": 1,
"staking": 1,
"upgrade": 1,
"vesting": 1,
"genutil": 1,
}

func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
return app.mm.RunMigrations(ctx, app.configurator, fromVM)
})

Expand Down
10 changes: 7 additions & 3 deletions x/staking/migrations/v046/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/staking/types"
)

// MigrateStore performs in-place store migrations from v0.43/v0.44 to v0.45.
// MigrateStore performs in-place store migrations from v0.43/v0.44/v0.45 to v0.46.
// The migration includes:
//
// - Setting the MinCommissionRate param in the paramstore
Expand All @@ -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() {
Copy link
Contributor Author

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

paramstore.Set(ctx, types.KeyMinCommissionRate, types.DefaultMinCommissionRate)
} else {
paramstore.WithKeyTable(types.ParamKeyTable())
paramstore.Set(ctx, types.KeyMinCommissionRate, types.DefaultMinCommissionRate)
}
}