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: added consensus version tracking (part of ADR-041) #8743

Merged
merged 42 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
42 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
b640766
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
8c7b105
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
5c1ae59
Changed MigrationMap identifier to VersionMap
technicallyty Mar 4, 2021
4c48f35
updated docs
technicallyty Mar 4, 2021
6db21e3
forgot this
technicallyty Mar 4, 2021
c9b435a
added line to changelog for this PR
technicallyty Mar 4, 2021
1c5b132
Change set consensus version function to match adr 041 spec
technicallyty Mar 8, 2021
304abfa
add documentation
technicallyty Mar 8, 2021
d06bbbd
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 8, 2021
5c75ead
Merge branch 'ty-8514-module_tracking' of https://github.com/technica…
technicallyty Mar 8, 2021
e53423e
remove newline from changelog
technicallyty Mar 8, 2021
c36086f
updated example in simapp for RunMigrations, SetCurrentConsensusVersi…
technicallyty Mar 8, 2021
758c67d
switch TestMigrations to use Require instead of t.Fatal
technicallyty Mar 8, 2021
0dac9c2
Update CHANGELOG.md
technicallyty Mar 10, 2021
8f526ef
docs for SetVersionManager
technicallyty Mar 10, 2021
027740d
-init genesis method added -removed panics/fails from setting consens…
technicallyty Mar 11, 2021
18eaed2
merge master
technicallyty Mar 11, 2021
fc38e5a
update identifiers to be more go-like
technicallyty Mar 12, 2021
28d5c04
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 12, 2021
1fca959
update docs and UpgradeHandler fnc sig
technicallyty Mar 15, 2021
d912f4f
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 15, 2021
a99c538
Upgrade Keeper now takes a VersionMap instead of a VersionManager int…
technicallyty Mar 15, 2021
e6424ce
upgrade keeper transition to Version Map
technicallyty Mar 16, 2021
9b40fe7
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 16, 2021
799b646
cleanup, added versionmap return to RunMigrations
technicallyty Mar 16, 2021
0f9447c
quick fix
technicallyty Mar 16, 2021
a157c48
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 17, 2021
b839a59
Update docs/architecture/adr-041-in-place-store-migrations.md
technicallyty Mar 17, 2021
75f8ae5
remove support for versionmap field on upgrade keeper
technicallyty Mar 17, 2021
22ab55e
cleanup
technicallyty Mar 17, 2021
47ce533
merge adr change
technicallyty Mar 17, 2021
7f5b6aa
rename get/set version map keeper functions
technicallyty Mar 18, 2021
8547426
update adr doc to match name changes
technicallyty Mar 18, 2021
1504a2d
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 18, 2021
c3072ab
remove redudant line
technicallyty Mar 19, 2021
1f82fff
Merge branch 'ty-8514-module_tracking' of https://github.com/technica…
technicallyty Mar 19, 2021
cf1343f
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 19, 2021
3f6bea8
Merge branch 'master' into ty-8514-module_tracking
aaronc Mar 19, 2021
d987555
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 19, 2021
5d6e4ab
Merge branch 'master' into ty-8514-module_tracking
mergify[bot] Mar 19, 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
6 changes: 5 additions & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,10 @@ func NewSimApp(
authz.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
transferModule,
)
// give upgrade keeper the module manager
app.UpgradeKeeper.SetModuleManager(app.mm)
// pass the updated keeper to the module manager
app.mm.Modules[upgradetypes.ModuleName] = upgrade.NewAppModule(app.UpgradeKeeper)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// During begin block slashing happens after distr.BeginBlocker so that
// there is nothing left over in the validator fee pool, so as to keep the
Expand All @@ -391,7 +395,7 @@ func NewSimApp(
// so that other modules that want to create or claim capabilities afterwards in InitChain
// can do so safely.
app.mm.SetOrderInitGenesis(
capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName,
upgradetypes.ModuleName, capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName,
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
slashingtypes.ModuleName, govtypes.ModuleName, minttypes.ModuleName, crisistypes.ModuleName,
ibchost.ModuleName, genutiltypes.ModuleName, evidencetypes.ModuleName, authztypes.ModuleName, ibctransfertypes.ModuleName,
feegranttypes.ModuleName,
Expand Down
12 changes: 12 additions & 0 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,3 +395,15 @@ func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
Events: ctx.EventManager().ABCIEvents(),
}
}

// GetConsensusVersions gets consensus version from all modules
func (m *Manager) GetConsensusVersions() MigrationMap {
migmap := make(map[string]uint64)
for _, v := range m.Modules {
version := v.ConsensusVersion()
name := v.Name()
migmap[name] = version
}

return migmap
}
4 changes: 4 additions & 0 deletions x/upgrade/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
func BuildUpgradeNeededMsg(plan types.Plan) string {
return fmt.Sprintf("UPGRADE \"%s\" NEEDED at %s: %s", plan.Name, plan.DueAt(), plan.Info)
}

func InitChainer(k keeper.Keeper, ctx sdk.Context) {
k.SetConsensusVersions(ctx)
}
6 changes: 3 additions & 3 deletions x/upgrade/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func VerifyDoUpgrade(t *testing.T) {
})

t.Log("Verify that the upgrade can be successfully applied with a handler")
s.keeper.SetUpgradeHandler("test", func(ctx sdk.Context, plan types.Plan) {})
s.keeper.SetUpgradeHandler("test", func(ctx sdk.Context, plan types.Plan, migrationMap module.MigrationMap) error { return nil })
require.NotPanics(t, func() {
s.module.BeginBlock(newCtx, req)
})
Expand All @@ -144,7 +144,7 @@ func VerifyDoUpgradeWithCtx(t *testing.T, newCtx sdk.Context, proposalName strin
})

t.Log("Verify that the upgrade can be successfully applied with a handler")
s.keeper.SetUpgradeHandler(proposalName, func(ctx sdk.Context, plan types.Plan) {})
s.keeper.SetUpgradeHandler(proposalName, func(ctx sdk.Context, plan types.Plan, migrationMap module.MigrationMap) error { return nil })
require.NotPanics(t, func() {
s.module.BeginBlock(newCtx, req)
})
Expand All @@ -156,7 +156,7 @@ func TestHaltIfTooNew(t *testing.T) {
s := setupTest(10, map[int64]bool{})
t.Log("Verify that we don't panic with registered plan not in database at all")
var called int
s.keeper.SetUpgradeHandler("future", func(ctx sdk.Context, plan types.Plan) { called++ })
s.keeper.SetUpgradeHandler("future", func(ctx sdk.Context, plan types.Plan, migrationMap module.MigrationMap) error { called++; return nil })

newCtx := s.ctx.WithBlockHeight(s.ctx.BlockHeight() + 1).WithBlockTime(time.Now())
req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
Expand Down
3 changes: 2 additions & 1 deletion x/upgrade/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

Expand Down Expand Up @@ -110,7 +111,7 @@ func (suite *UpgradeTestSuite) TestAppliedCurrentPlan() {
suite.app.UpgradeKeeper.ScheduleUpgrade(suite.ctx, plan)

suite.ctx = suite.ctx.WithBlockHeight(expHeight)
suite.app.UpgradeKeeper.SetUpgradeHandler(planName, func(ctx sdk.Context, plan types.Plan) {})
suite.app.UpgradeKeeper.SetUpgradeHandler(planName, func(_ sdk.Context, _ types.Plan, _ module.MigrationMap) error { return nil })
suite.app.UpgradeKeeper.ApplyUpgrade(suite.ctx, plan)

req = &types.QueryAppliedPlanRequest{Name: planName}
Expand Down
48 changes: 47 additions & 1 deletion x/upgrade/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
store "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

Expand All @@ -28,6 +29,7 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryMarshaler
upgradeHandlers map[string]types.UpgradeHandler
ModuleManager *module.Manager
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
}

// NewKeeper constructs an upgrade Keeper
Expand All @@ -48,6 +50,50 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
k.upgradeHandlers[name] = upgradeHandler
}

func (k *Keeper) SetModuleManager(mm *module.Manager) *Keeper {
k.ModuleManager = mm
return k
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
}

// SetConsensusVersions saves the consensus versions retrieved from module.Manager
func (k Keeper) SetConsensusVersions(ctx sdk.Context) {
modules := k.ModuleManager.GetConsensusVersions()
for modName, ver := range modules {
k.setConsensusVersion(ctx, ver, modName)
}
}

// SetConsensusVersion sets the given module's consensus version to the given version
func (k Keeper) setConsensusVersion(ctx sdk.Context, version uint64, moduleName string) {
store := k.getConsensusVersionPrefixStore(ctx, moduleName)
versionBytes := make([]byte, 8)
binary.LittleEndian.PutUint64(versionBytes, version)
store.Set(types.MigrationMapKeyModule(moduleName), versionBytes)
}

// getConsensusVersionStore gets the store of the given module name
func (k Keeper) getConsensusVersionPrefixStore(ctx sdk.Context, moduleName string) prefix.Store {
store := ctx.KVStore(k.storeKey)
return prefix.NewStore(store, types.MigrationMapKeyModule(moduleName))
}

// GetConsensusVersions gets a MigrationMap from state
func (k Keeper) GetConsensusVersions(ctx sdk.Context) module.MigrationMap {
store := ctx.KVStore(k.storeKey)
it := sdk.KVStorePrefixIterator(store, []byte{types.MigrationMapByte})

migmap := make(map[string]uint64)

defer it.Close()
for ; it.Valid(); it.Next() {
moduleName := string(it.Key())
moduleVersion := binary.LittleEndian.Uint64(it.Value())
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
migmap[moduleName] = moduleVersion
}

return migmap
}

// ScheduleUpgrade schedules an upgrade based on the specified plan.
// If there is another Plan already scheduled, it will overwrite it
// (implicitly cancelling the current plan)
Expand Down Expand Up @@ -191,7 +237,7 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {
panic("ApplyUpgrade should never be called without first checking HasHandler")
}

handler(ctx, plan)
handler(ctx, plan, k.GetConsensusVersions(ctx))
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// 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.
Expand Down
3 changes: 2 additions & 1 deletion x/upgrade/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/simapp"
store "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)
Expand Down Expand Up @@ -135,7 +136,7 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
Height: 123450000,
},
setup: func() {
s.app.UpgradeKeeper.SetUpgradeHandler("all-good", func(_ sdk.Context, _ types.Plan) {})
s.app.UpgradeKeeper.SetUpgradeHandler("all-good", func(_ sdk.Context, _ types.Plan, _ module.MigrationMap) error { return nil })
s.app.UpgradeKeeper.ApplyUpgrade(s.ctx, types.Plan{
Name: "all-good",
Info: "some text here",
Expand Down
3 changes: 2 additions & 1 deletion x/upgrade/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
}

// InitGenesis is ignored, no sense in serializing future upgrades
func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate {
func (am AppModule) InitGenesis(ctx sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
InitChainer(am.keeper, ctx)
Copy link
Contributor

@amaury1093 amaury1093 Mar 2, 2021

Choose a reason for hiding this comment

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

@tyler I think i might have been mistaken as to when we should call keeper.SetConsensusVersions. We actually don't need to call it in InitChain, just leave it blank on a fresh chain, as per #8504 (comment), I believe it should be called AFTER the handler has run (so here), to say "ok I've ran the handler (e.g. migration scripts), now I'll update the store with the newest modules' ConsensusVersions".

I think the ultimate E2E test would be with 2 binaries, but since that's hard, let's try to simulate that in a test here:

  • create a mock module.VersionManager, say with bank set to 1, and everything else set to latest ConsensusVersion.
  • upgradeKeeper.SetModuleManager(mockVersionMgr). This simulates saving the "old" modules version to state.
  • add an upgrade handler to keeper.
  • call ApplyUpgrade on keeper
  • after ApplyUpgrade, call keeper.GetConsensusVersions, test that the return value's bank key should be 2, not 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added and initchainer removed

Copy link
Member

Choose a reason for hiding this comment

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

If it's not set in IntiChain then how do initial versions get set?? I don't see a good alternative and it's not a safe assumption that all chains start at module version 1

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to add this back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return []abci.ValidatorUpdate{}
}

Expand Down
26 changes: 26 additions & 0 deletions x/upgrade/module_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package upgrade_test

import (
"testing"

"github.com/stretchr/testify/require"
abcitypes "github.com/tendermint/tendermint/abci/types"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

"github.com/cosmos/cosmos-sdk/simapp"
)

func TestItSavesConsensusVersionToState(t *testing.T) {
app := simapp.Setup(false)
ctx := app.BaseApp.NewContext(false, tmproto.Header{})

app.InitChain(
abcitypes.RequestInitChain{
AppStateBytes: []byte("{}"),
ChainId: "test-chain-id",
},
)

migmap := app.UpgradeKeeper.GetConsensusVersions(ctx)
require.NotEmpty(t, migmap)
}
3 changes: 2 additions & 1 deletion x/upgrade/types/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ package types

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
)

// UpgradeHandler specifies the type of function that is called when an upgrade is applied
type UpgradeHandler func(ctx sdk.Context, plan Plan)
type UpgradeHandler func(ctx sdk.Context, plan Plan, migrationMap module.MigrationMap) error
13 changes: 13 additions & 0 deletions x/upgrade/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const (
// DoneByte is a prefix for to look up completed upgrade plan by name
DoneByte = 0x1

// MigrationMapByte is a prefix to look up module names (key) and versions (value)
MigrationMapByte = 0x2

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

Expand Down Expand Up @@ -51,3 +54,13 @@ func UpgradedClientKey(height int64) []byte {
func UpgradedConsStateKey(height int64) []byte {
return []byte(fmt.Sprintf("%s/%d/%s", KeyUpgradedIBCState, height, KeyUpgradedConsState))
}

func MigrationMapKey() []byte {
return []byte{MigrationMapByte}
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

func MigrationMapKeyModule(moduleName string) []byte {
nameBytes := []byte(moduleName)
key := append(MigrationMapKey(), nameBytes...)
return key
}