Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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 {
},
}
}

// makeKeyMigrateCmd 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 makeKeyMigrateCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "key-migrate",
Copy link
Copy Markdown
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
Copy Markdown
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 Tendermint database key migration",
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
Copy Markdown
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
Copy Markdown
Contributor Author

@amaury1093 amaury1093 May 27, 2022

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
Copy Markdown
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
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,
makeKeyMigrateCmd(),
)

startCmd := StartCmd(appCreator, defaultNodeHome)
Expand Down
7 changes: 4 additions & 3 deletions simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,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 @@ -393,6 +390,10 @@ func NewSimApp(
app.ModuleManager.RegisterInvariants(&app.CrisisKeeper)
app.ModuleManager.RegisterRoutes(app.Router(), app.QueryRouter(), encodingConfig.Amino)

// 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
Copy Markdown
Contributor

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
Copy Markdown
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
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.ModuleManager.RunMigrations(ctx, app.Configurator(), fromVM)
})

Expand Down
10 changes: 7 additions & 3 deletions x/staking/migrations/v046/store.go
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
Copy Markdown
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)
}
}