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.

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

x/upgrade: protocol version tracking #8897

Merged
merged 41 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
e900fb1
-added consensus version tracking to x/upgrade
technicallyty Mar 2, 2021
036db4f
-added interface to module manager -added e2e test for migrations usi…
technicallyty Mar 3, 2021
8c7b105
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
efe2817
protocol version p1
technicallyty Mar 4, 2021
32316ef
add protocol version to baseapp
technicallyty Mar 10, 2021
1a51e9e
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 11, 2021
0fe4051
rebase against master
technicallyty Mar 11, 2021
a6eae2a
add test
technicallyty Mar 11, 2021
fb696bc
added test case
technicallyty Mar 11, 2021
dc8e0d0
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 16, 2021
128926d
cleanup
technicallyty Mar 16, 2021
35a424e
docs and change to bigendian
technicallyty Mar 16, 2021
16b92fb
changelog
technicallyty Mar 16, 2021
d9c29c4
Merge branch 'master' into ty-7487-protocol_version
Mar 18, 2021
07ab2cc
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 19, 2021
4039b34
cleanup keeper
technicallyty Mar 19, 2021
61576a1
Merge branch 'ty-7487-protocol_version' of https://github.com/cosmos/…
technicallyty Mar 19, 2021
cb4c0f4
swap appVersion and version
technicallyty Mar 20, 2021
ec21cf5
rebase
technicallyty Mar 20, 2021
21af48c
cleanup
technicallyty Mar 20, 2021
f6e16a2
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 22, 2021
ad4fceb
change interface id
technicallyty Mar 22, 2021
d7af939
merge master
technicallyty Mar 23, 2021
6e3da1c
update keeper field name to versionSetter
technicallyty Mar 23, 2021
26ac281
reorder keys and docs
technicallyty Mar 23, 2021
e7309fd
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 24, 2021
008bd20
Merge branch 'master' into ty-7487-protocol_version2
technicallyty Mar 25, 2021
71a4dfa
-move interface into exported folder
technicallyty Mar 25, 2021
31a1f12
typo
technicallyty Mar 25, 2021
d20bfc1
typo2
technicallyty Mar 25, 2021
97ab692
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 27, 2021
4bf3eaa
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
1df5a51
docs on keeper fields
technicallyty Mar 29, 2021
b61ab5a
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
b0f7765
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
31a6a1a
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 30, 2021
a43f7e6
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 30, 2021
7760ab1
docs for upgrade NewKeeper
technicallyty Mar 30, 2021
bebb4fe
cleanup
technicallyty Mar 30, 2021
b2003dd
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 31, 2021
d86db6f
NewKeeper docs
technicallyty Apr 1, 2021
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) `SupplyI` interface and `Supply` are removed and uses `sdk.Coins` for supply tracking
* (x/upgrade) [\#8743](https://github.com/cosmos/cosmos-sdk/pull/8743) `UpgradeHandler` includes a new argument `VersionMap` which helps facilitate in-place migrations.
* (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error.
* (x/upgrade) [\7487](https://github.com/cosmos/cosmos-sdk/pull/8897) Upgrade `Keeper` takes new argument `ProtocolVersionSetter` which implements setting a protocol version on baseapp.
* (baseapp) [\7487](https://github.com/cosmos/cosmos-sdk/pull/8897) BaseApp's fields appVersion and version were swapped to match Tendermint's fields.
* [\#8682](https://github.com/cosmos/cosmos-sdk/pull/8682) `ante.NewAnteHandler` updated to receive all positional params as `ante.HandlerOptions` struct. If required fields aren't set, throws error accordingly.


Expand Down
4 changes: 3 additions & 1 deletion baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {

return abci.ResponseInfo{
Data: app.name,
Version: app.version,
AppVersion: app.appVersion,
LastBlockHeight: lastCommitID.Version,
LastBlockAppHash: lastCommitID.Hash,
}
Expand Down Expand Up @@ -755,7 +757,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) abci.Res
return abci.ResponseQuery{
Codespace: sdkerrors.RootCodespace,
Height: req.Height,
Value: []byte(app.appVersion),
Value: []byte(app.version),
}

default:
Expand Down
15 changes: 12 additions & 3 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ type BaseApp struct { // nolint: maligned
minRetainBlocks uint64

// application's version string
appVersion string
version string

// application's protocol version that increments on every upgrade
// if BaseApp is passed to the upgrade keeper's NewKeeper method.
appVersion uint64

// recovery handler for app.runTx method
runTxRecoveryMiddleware recoveryMiddleware
Expand Down Expand Up @@ -170,11 +174,16 @@ func (app *BaseApp) Name() string {
return app.name
}

// AppVersion returns the application's version string.
func (app *BaseApp) AppVersion() string {
// AppVersion returns the application's protocol version.
func (app *BaseApp) AppVersion() uint64 {
return app.appVersion
}

// Version returns the application's version string.
func (app *BaseApp) Version() string {
return app.version
}

// Logger returns the logger of the BaseApp.
func (app *BaseApp) Logger() log.Logger {
return app.logger
Expand Down
12 changes: 6 additions & 6 deletions baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,21 +340,21 @@ func TestSetLoader(t *testing.T) {
}
}

func TestAppVersionSetterGetter(t *testing.T) {
func TestVersionSetterGetter(t *testing.T) {
logger := defaultLogger()
pruningOpt := SetPruning(store.PruneDefault)
db := dbm.NewMemDB()
name := t.Name()
app := NewBaseApp(name, logger, db, nil, pruningOpt)

require.Equal(t, "", app.AppVersion())
require.Equal(t, "", app.Version())
res := app.Query(abci.RequestQuery{Path: "app/version"})
require.True(t, res.IsOK())
require.Equal(t, "", string(res.Value))

versionString := "1.0.0"
app.SetAppVersion(versionString)
require.Equal(t, versionString, app.AppVersion())
app.SetVersion(versionString)
require.Equal(t, versionString, app.Version())
res = app.Query(abci.RequestQuery{Path: "app/version"})
require.True(t, res.IsOK())
require.Equal(t, versionString, string(res.Value))
Expand Down Expand Up @@ -498,7 +498,7 @@ func TestInfo(t *testing.T) {
assert.Equal(t, t.Name(), res.GetData())
assert.Equal(t, int64(0), res.LastBlockHeight)
require.Equal(t, []uint8(nil), res.LastBlockAppHash)

require.Equal(t, app.AppVersion(), res.AppVersion)
// ----- test a proper response -------
// TODO
}
Expand All @@ -510,7 +510,7 @@ func TestBaseAppOptionSeal(t *testing.T) {
app.SetName("")
})
require.Panics(t, func() {
app.SetAppVersion("")
app.SetVersion("")
})
require.Panics(t, func() {
app.SetDB(nil)
Expand Down
16 changes: 13 additions & 3 deletions baseapp/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,22 @@ func (app *BaseApp) SetParamStore(ps ParamStore) {
app.paramStore = ps
}

// SetAppVersion sets the application's version string.
func (app *BaseApp) SetAppVersion(v string) {
// SetVersion sets the application's version string.
func (app *BaseApp) SetVersion(v string) {
if app.sealed {
panic("SetAppVersion() on sealed BaseApp")
panic("SetVersion() on sealed BaseApp")
}
app.version = v
}

// ProtocolVersionSetter exposes functionality to set
// BaseApp's protocol version.
type ProtocolVersionSetter interface {
SetProtocolVersion(uint64)
}
blushi marked this conversation as resolved.
Show resolved Hide resolved

// SetProtocolVersion sets the application's protocol version
func (app *BaseApp) SetProtocolVersion(v uint64) {
app.appVersion = v
}

Expand Down
4 changes: 2 additions & 2 deletions simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func NewSimApp(

bApp := baseapp.NewBaseApp(appName, logger, db, encodingConfig.TxConfig.TxDecoder(), baseAppOptions...)
bApp.SetCommitMultiStoreTracer(traceStore)
bApp.SetAppVersion(version.Version)
bApp.SetVersion(version.Version)
bApp.SetInterfaceRegistry(interfaceRegistry)

keys := sdk.NewKVStoreKeys(
Expand Down Expand Up @@ -257,7 +257,7 @@ func NewSimApp(
)

app.FeeGrantKeeper = feegrantkeeper.NewKeeper(appCodec, keys[feegranttypes.StoreKey], app.AccountKeeper)
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath)
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath, app.BaseApp)

// register the staking hooks
// NOTE: stakingKeeper above is passed by reference, so that it will contain these hooks
Expand Down
40 changes: 39 additions & 1 deletion x/upgrade/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,18 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryMarshaler
upgradeHandlers map[string]types.UpgradeHandler
versionSetter ProtocolVersionSetter // Implements setting the protocol version field on BaseApp
}

// NewKeeper constructs an upgrade Keeper
Copy link
Member

Choose a reason for hiding this comment

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

Can we document all of the parameters here? I'd also like this to explicitly allow ProtocolManager to be nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like this has been addressed @technicallyty, although I guess documenting vs ProtocolVersionSetter would be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does documentation on just the versionSetter suffice @aaronc? I can add comments to all of them, or maybe can do that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I would like comments on all of them. It can be a separate PR if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comments for all the fields @aaronc

Copy link
Member

@aaronc aaronc Mar 30, 2021

Choose a reason for hiding this comment

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

Awesome that's great. We also do want docs for all arguments to NewKeeper. That is what users of the SDK will see in godocs when they're using NewKeeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added doc comments. let me know if the format/content needs to change @aaronc

func NewKeeper(skipUpgradeHeights map[int64]bool, storeKey sdk.StoreKey, cdc codec.BinaryMarshaler, homePath string) Keeper {
func NewKeeper(skipUpgradeHeights map[int64]bool, storeKey sdk.StoreKey, cdc codec.BinaryMarshaler, homePath string, vs ProtocolVersionSetter) Keeper {
return Keeper{
homePath: homePath,
skipUpgradeHeights: skipUpgradeHeights,
storeKey: storeKey,
cdc: cdc,
upgradeHandlers: map[string]types.UpgradeHandler{},
versionSetter: vs,
}
}

Expand All @@ -49,6 +51,34 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
k.upgradeHandlers[name] = upgradeHandler
}

// ProtocolVersionSetter defines the interface fulfilled by BaseApp
// which allows setting it's appVersion field.
type ProtocolVersionSetter interface {
SetProtocolVersion(uint64)
}
blushi marked this conversation as resolved.
Show resolved Hide resolved

// setProtocolVersion sets the protocol version to state
func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) {
store := ctx.KVStore(k.storeKey)
versionBytes := make([]byte, 8)
binary.BigEndian.PutUint64(versionBytes, v)
store.Set([]byte{types.ProtocolVersionByte}, versionBytes)
}

// GetAppVersion gets the protocol version from state
func (k Keeper) getProtocolVersion(ctx sdk.Context) uint64 {
store := ctx.KVStore(k.storeKey)
ok := store.Has([]byte{types.ProtocolVersionByte})
if ok {
pvBytes := store.Get([]byte{types.ProtocolVersionByte})
protocolVersion := binary.BigEndian.Uint64(pvBytes)

return protocolVersion
}
// default value
return 0
}

// SetModuleVersionMap saves a given version map to state
func (k Keeper) SetModuleVersionMap(ctx sdk.Context, vm module.VersionMap) {
if len(vm) > 0 {
Expand Down Expand Up @@ -228,6 +258,14 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {

k.SetModuleVersionMap(ctx, updatedVM)

// incremement the protocol version and set it in state and baseapp
nextProtocolVersion := k.getProtocolVersion(ctx) + 1
k.setProtocolVersion(ctx, nextProtocolVersion)
if k.versionSetter != nil {
// set protocol version on BaseApp
k.versionSetter.SetProtocolVersion(nextProtocolVersion)
}

// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.
// This will prevent resubmission of upgrade msg after upgrade is already completed.
k.ClearIBCState(ctx, plan.Height)
Expand Down
18 changes: 17 additions & 1 deletion x/upgrade/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (s *KeeperTestSuite) SetupTest() {
app := simapp.Setup(false)
homeDir := filepath.Join(s.T().TempDir(), "x_upgrade_keeper_test")
app.UpgradeKeeper = keeper.NewKeeper( // recreate keeper in order to use a custom home path
make(map[int64]bool), app.GetKey(types.StoreKey), app.AppCodec(), homeDir,
make(map[int64]bool), app.GetKey(types.StoreKey), app.AppCodec(), homeDir, app.BaseApp,
)
s.T().Log("home dir:", homeDir)
s.homeDir = homeDir
Expand Down Expand Up @@ -194,6 +194,22 @@ func (s *KeeperTestSuite) TestSetUpgradedClient() {

}

// Test that the protocol version successfully increments after an
// upgrade and is succesfully set on BaseApp's appVersion.
func (s *KeeperTestSuite) TestIncrementProtocolVersion() {
oldProtocolVersion := s.app.BaseApp.AppVersion()
s.app.UpgradeKeeper.SetUpgradeHandler("dummy", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { return vm, nil })
dummyPlan := types.Plan{
Name: "dummy",
Info: "some text here",
Height: 100,
}
s.app.UpgradeKeeper.ApplyUpgrade(s.ctx, dummyPlan)
upgradedProtocolVersion := s.app.BaseApp.AppVersion()

s.Require().Equal(oldProtocolVersion+1, upgradedProtocolVersion)
}

// Tests that the underlying state of x/upgrade is set correctly after
// an upgrade.
func (s *KeeperTestSuite) TestMigrations() {
Expand Down
7 changes: 4 additions & 3 deletions x/upgrade/spec/02_state.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ order: 2

The internal state of the `x/upgrade` module is relatively minimal and simple. The
state contains the currently active upgrade `Plan` (if one exists) by key
`0x0` and if a `Plan` is marked as "done" by key `0x1`. Additionally, the state
`0x0` and if a `Plan` is marked as "done" by key `0x1`. The state
contains the consensus versions of all app modules in the application. The versions
are stored as big endian `uint64`, and can be accessed with prefix `0x2` appended
by the corresponding module name of type `string`.
by the corresponding module name of type `string`. The state maintains a
`Protocol Version` which can be accessed by key `0x3`.

- Plan: `0x0 -> Plan`
- Done: `0x1 | byte(plan name) -> BigEndian(Block Height)`
- ConsensusVersion: `0x2 | byte(module name) -> BigEndian(Module Consensus Version)`

- ProtocolVersion: `0x3 -> BigEndian(Protocol Version)`

The `x/upgrade` module contains no genesis state.
3 changes: 3 additions & 0 deletions x/upgrade/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ const (
// VersionMapByte is a prefix to look up module names (key) and versions (value)
VersionMapByte = 0x2

// ProtocolVersionByte is a prefix to look up Protocol Version
ProtocolVersionByte = 0x3
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// KeyUpgradedIBCState is the key under which upgraded ibc state is stored in the upgrade store
KeyUpgradedIBCState = "upgradedIBCState"

Expand Down