From 8570d1bb6a09d8fb0d87cfb966bf4e1992ad838c Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 18:56:05 +0200 Subject: [PATCH 01/10] implement BeginBlock fix --- simapp/app.go | 4 +- x/capability/abci.go | 21 ++++++++ x/capability/capability_test.go | 86 ++++++++++++++++++++++++++++++ x/capability/genesis.go | 5 +- x/capability/genesis_test.go | 59 ++++++++++++++++++++ x/capability/keeper/keeper.go | 62 ++++++++++----------- x/capability/keeper/keeper_test.go | 3 ++ x/capability/module.go | 4 +- x/capability/types/keys.go | 8 +++ 9 files changed, 212 insertions(+), 40 deletions(-) create mode 100644 x/capability/abci.go create mode 100644 x/capability/capability_test.go create mode 100644 x/capability/genesis_test.go diff --git a/simapp/app.go b/simapp/app.go index 5eebfebb0f4..18221250a7f 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -215,7 +215,7 @@ func NewSimApp( evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, ) tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) - memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey) + memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey, "testing") app := &SimApp{ BaseApp: bApp, @@ -350,7 +350,7 @@ func NewSimApp( // CanWithdrawInvariant invariant. // NOTE: staking module is required if HistoricalEntries param > 0 app.mm.SetOrderBeginBlockers( - upgradetypes.ModuleName, minttypes.ModuleName, distrtypes.ModuleName, slashingtypes.ModuleName, + upgradetypes.ModuleName, capabilitytypes.ModuleName, minttypes.ModuleName, distrtypes.ModuleName, slashingtypes.ModuleName, evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, ) app.mm.SetOrderEndBlockers(crisistypes.ModuleName, govtypes.ModuleName, stakingtypes.ModuleName) diff --git a/x/capability/abci.go b/x/capability/abci.go new file mode 100644 index 00000000000..9d1c94b5b90 --- /dev/null +++ b/x/capability/abci.go @@ -0,0 +1,21 @@ +package capability + +import ( + "time" + + "github.com/cosmos/cosmos-sdk/telemetry" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/capability/keeper" + "github.com/cosmos/cosmos-sdk/x/capability/types" +) + +// BeginBlocker will call InitMemStore to initialize the memory stores in the case +// that this is the first time the node is executing a block since restarting (wiping memory). +// In this case, the BeginBlocker method will reinitialize the memory stores locally, so that subsequent +// capability transactions will pass. +// Otherwise BeginBlocker performs a no-op. +func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { + defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) + + k.InitMemStore(ctx) +} diff --git a/x/capability/capability_test.go b/x/capability/capability_test.go new file mode 100644 index 00000000000..ae6df05a373 --- /dev/null +++ b/x/capability/capability_test.go @@ -0,0 +1,86 @@ +package capability_test + +import ( + "testing" + + "github.com/stretchr/testify/suite" + abci "github.com/tendermint/tendermint/abci/types" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/simapp" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/module" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/cosmos/cosmos-sdk/x/capability" + "github.com/cosmos/cosmos-sdk/x/capability/keeper" + "github.com/cosmos/cosmos-sdk/x/capability/types" +) + +type CapabilityTestSuite struct { + suite.Suite + + cdc codec.Marshaler + ctx sdk.Context + app *simapp.SimApp + keeper *keeper.Keeper + module module.AppModule +} + +func (suite *CapabilityTestSuite) SetupTest() { + checkTx := false + app := simapp.Setup(checkTx) + cdc := app.AppCodec() + + // create new keeper so we can define custom scoping before init and seal + keeper := keeper.NewKeeper(cdc, app.GetKey(types.StoreKey), app.GetMemKey(types.MemStoreKey)) + + suite.app = app + suite.ctx = app.BaseApp.NewContext(checkTx, tmproto.Header{Height: 1}) + suite.keeper = keeper + suite.cdc = cdc + suite.module = capability.NewAppModule(cdc, *keeper) +} + +// The following test case mocks a specific bug discovered in https://github.com/cosmos/cosmos-sdk/issues/9800 +// and ensures that the current code successfully fixes the issue. +func (suite *CapabilityTestSuite) TestInitializeMemStore() { + sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName) + + cap1, err := sk1.NewCapability(suite.ctx, "transfer") + suite.Require().NoError(err) + suite.Require().NotNil(cap1) + + // mock statesync by creating new keeper that shares persistent state but loses in-memory map + newKeeper := keeper.NewKeeper(suite.cdc, suite.app.GetKey(types.StoreKey), suite.app.GetMemKey("testing")) + newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName) + + // Mock App startup + ctx := suite.app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + newKeeper.InitializeAndSeal(ctx) + + // Mock app beginblock and ensure that no gas has been consumed + ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewGasMeter(50)) + prevGas := ctx.BlockGasMeter().GasConsumed() + restartedModule := capability.NewAppModule(suite.cdc, *newKeeper) + restartedModule.BeginBlock(ctx, abci.RequestBeginBlock{}) + gasUsed := ctx.BlockGasMeter().GasConsumed() + + suite.Require().Equal(prevGas, gasUsed, "beginblocker consumed gas during execution") + + // Mock the first transaction getting capability and subsequently failing + // by using a cached context and discarding all cached writes. + cacheCtx, _ := ctx.CacheContext() + _, ok := newSk1.GetCapability(cacheCtx, "transfer") + suite.Require().True((ok)) + + // Ensure that the second transaction can still receive capability even if first tx fails. + ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}) + + _, ok = newSk1.GetCapability(ctx, "transfer") + suite.Require().True(ok) +} + +func TestCapabilityTestSuite(t *testing.T) { + suite.Run(t, new(CapabilityTestSuite)) +} diff --git a/x/capability/genesis.go b/x/capability/genesis.go index ba8e09dcd37..2e9a11b994b 100644 --- a/x/capability/genesis.go +++ b/x/capability/genesis.go @@ -13,11 +13,12 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) panic(err) } - // set owners for each index and initialize capability + // set owners for each index for _, genOwner := range genState.Owners { k.SetOwners(ctx, genOwner.Index, genOwner.IndexOwners) - k.InitializeCapability(ctx, genOwner.Index, genOwner.IndexOwners) } + // initialize in-memory capabilities + k.InitMemStore(ctx) } // ExportGenesis returns the capability module's exported genesis. diff --git a/x/capability/genesis_test.go b/x/capability/genesis_test.go new file mode 100644 index 00000000000..34a09960e75 --- /dev/null +++ b/x/capability/genesis_test.go @@ -0,0 +1,59 @@ +package capability_test + +import ( + "github.com/tendermint/tendermint/libs/log" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + dbm "github.com/tendermint/tm-db" + + "github.com/cosmos/cosmos-sdk/simapp" + sdk "github.com/cosmos/cosmos-sdk/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/cosmos/cosmos-sdk/x/capability" + "github.com/cosmos/cosmos-sdk/x/capability/keeper" + "github.com/cosmos/cosmos-sdk/x/capability/types" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" +) + +func (suite *CapabilityTestSuite) TestGenesis() { + sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName) + sk2 := suite.keeper.ScopeToModule(stakingtypes.ModuleName) + + cap1, err := sk1.NewCapability(suite.ctx, "transfer") + suite.Require().NoError(err) + suite.Require().NotNil(cap1) + + err = sk2.ClaimCapability(suite.ctx, cap1, "transfer") + suite.Require().NoError(err) + + cap2, err := sk2.NewCapability(suite.ctx, "ica") + suite.Require().NoError(err) + suite.Require().NotNil(cap2) + + genState := capability.ExportGenesis(suite.ctx, *suite.keeper) + + // create new app that does not share persistent or in-memory state + // and initialize app from exported genesis state above. + db := dbm.NewMemDB() + encCdc := simapp.MakeTestEncodingConfig() + newApp := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, simapp.DefaultNodeHome, 5, encCdc, simapp.EmptyAppOptions{}) + + newKeeper := keeper.NewKeeper(suite.cdc, newApp.GetKey(types.StoreKey), newApp.GetMemKey(types.MemStoreKey)) + newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName) + newSk2 := newKeeper.ScopeToModule(stakingtypes.ModuleName) + deliverCtx, _ := newApp.BaseApp.NewUncachedContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewInfiniteGasMeter()).CacheContext() + + capability.InitGenesis(deliverCtx, *newKeeper, *genState) + + // check that all previous capabilities exist in new app after InitGenesis + sk1Cap1, ok := newSk1.GetCapability(deliverCtx, "transfer") + suite.Require().True(ok, "could not get first capability after genesis on first ScopedKeeper") + suite.Require().Equal(*cap1, *sk1Cap1, "capability values not equal on first ScopedKeeper") + + sk2Cap1, ok := newSk2.GetCapability(deliverCtx, "transfer") + suite.Require().True(ok, "could not get first capability after genesis on first ScopedKeeper") + suite.Require().Equal(*cap1, *sk2Cap1, "capability values not equal on first ScopedKeeper") + + sk2Cap2, ok := newSk2.GetCapability(deliverCtx, "ica") + suite.Require().True(ok, "could not get second capability after genesis on second ScopedKeeper") + suite.Require().Equal(*cap2, *sk2Cap2, "capability values not equal on second ScopedKeeper") +} diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index b92dfc94baf..5ae963790e3 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -13,14 +13,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/capability/types" ) -// initialized is a global variable used by GetCapability to ensure that the memory store -// and capability map are correctly populated. A state-synced node may copy over all the persistent -// state and start running the application without having the in-memory state required for x/capability. -// Thus, we must initialized the memory stores on-the-fly during tx execution once the first GetCapability -// is called. -// This is a temporary fix and should be replaced by a more robust solution in the next breaking release. -var initialized = false - type ( // Keeper defines the capability module's keeper. It is responsible for provisioning, // tracking, and authenticating capabilities at runtime. During application @@ -113,22 +105,38 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { panic(fmt.Sprintf("invalid memory store type; got %s, expected: %s", memStoreType, sdk.StoreTypeMemory)) } - prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) - iterator := sdk.KVStorePrefixIterator(prefixStore, nil) + k.sealed = true +} - // initialize the in-memory store for all persisted capabilities - defer iterator.Close() +// InitMemStore will initialize the memory store if the initialized flag is not set. +// InitMemStore logic should be run in first `BeginBlock` or `InitChain` (whichever is run on app start) +// so that the memory store is properly initialized before any transactions are run. +func (k *Keeper) InitMemStore(ctx sdk.Context) { + // create context with no block gas meter to ensure we do not consume gas during local initialization logic. + noGasCtx := ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) - for ; iterator.Valid(); iterator.Next() { - index := types.IndexFromKey(iterator.Key()) + // check if memory store has not been initialized yet by checking if initialized flag is nil. + memStore := noGasCtx.KVStore(k.memKey) + flag := memStore.Get(types.MemInitializedKey()) + if flag == nil { + prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) + iterator := sdk.KVStorePrefixIterator(prefixStore, nil) - var capOwners types.CapabilityOwners + // initialize the in-memory store for all persisted capabilities + defer iterator.Close() - k.cdc.MustUnmarshalBinaryBare(iterator.Value(), &capOwners) - k.InitializeCapability(ctx, index, capOwners) - } + for ; iterator.Valid(); iterator.Next() { + index := types.IndexFromKey(iterator.Key()) - k.sealed = true + var capOwners types.CapabilityOwners + + k.cdc.MustUnmarshalBinaryBare(iterator.Value(), &capOwners) + k.InitializeCapability(noGasCtx, index, capOwners) + } + + // set the initialized flag so we don't rerun initialization logic + memStore.Set(types.MemInitializedKey(), []byte{1}) + } } // InitializeIndex sets the index to one (or greater) in InitChain according @@ -350,22 +358,6 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) // by name. The module is not allowed to retrieve capabilities which it does not // own. func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capability, bool) { - // Create a keeper that will set all in-memory mappings correctly into memstore and capmap if scoped keeper is not initialized yet. - // This ensures that the in-memory mappings are correctly filled in, in case this is a state-synced node. - // This is a temporary non-breaking fix, a future PR should store the reverse mapping in the persistent store and reconstruct forward mapping and capmap on the fly. - if !initialized { - // create context with infinite gas meter to avoid app state mismatch. - initCtx := ctx.WithGasMeter(sdk.NewInfiniteGasMeter()) - k := Keeper{ - cdc: sk.cdc, - storeKey: sk.storeKey, - memKey: sk.memKey, - capMap: sk.capMap, - } - k.InitializeAndSeal(initCtx) - initialized = true - } - if strings.TrimSpace(name) == "" { return nil, false } diff --git a/x/capability/keeper/keeper_test.go b/x/capability/keeper/keeper_test.go index e62176a7246..094b1281139 100644 --- a/x/capability/keeper/keeper_test.go +++ b/x/capability/keeper/keeper_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/suite" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" @@ -18,6 +19,7 @@ import ( type KeeperTestSuite struct { suite.Suite + cdc codec.Marshaler ctx sdk.Context app *simapp.SimApp keeper *keeper.Keeper @@ -34,6 +36,7 @@ func (suite *KeeperTestSuite) SetupTest() { suite.app = app suite.ctx = app.BaseApp.NewContext(checkTx, tmproto.Header{Height: 1}) suite.keeper = keeper + suite.cdc = cdc } func (suite *KeeperTestSuite) TestInitializeAndSeal() { diff --git a/x/capability/module.go b/x/capability/module.go index 7957f57747d..388d8021074 100644 --- a/x/capability/module.go +++ b/x/capability/module.go @@ -137,7 +137,9 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONMarshaler) json } // BeginBlock executes all ABCI BeginBlock logic respective to the capability module. -func (am AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {} +func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) { + BeginBlocker(ctx, am.keeper) +} // EndBlock executes all ABCI EndBlock logic respective to the capability module. It // returns no validator updates. diff --git a/x/capability/types/keys.go b/x/capability/types/keys.go index 5f171e28a7f..5424167557d 100644 --- a/x/capability/types/keys.go +++ b/x/capability/types/keys.go @@ -25,6 +25,9 @@ var ( // KeyPrefixIndexCapability defines a key prefix that stores index to capability // name mappings. KeyPrefixIndexCapability = []byte("capability_index") + + // KeyMemInitialized defines the key that stores the initialized flag in the memory store + KeyMemInitialized = []byte("mem_initialized") ) // RevCapabilityKey returns a reverse lookup key for a given module and capability @@ -49,3 +52,8 @@ func IndexToKey(index uint64) []byte { func IndexFromKey(key []byte) uint64 { return sdk.BigEndianToUint64(key) } + +// MemInitializedKey returns the key to store the initialized flag in memory store +func MemInitializedKey() []byte { + return []byte(KeyMemInitialized) +} From 3d8755f71eb19c1a371ba3f7311d215e0799c17f Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 19:24:07 +0200 Subject: [PATCH 02/10] add changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c05bc1a621..982850d5a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## Unreleased +### Bug Fixes + +* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminsim issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). + ## [v0.42.8](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.8) - 2021-07-30 ### Features From aee978e378ed99f2630d8de3918ad079cd32bd1e Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 20:40:08 +0200 Subject: [PATCH 03/10] fix lint --- x/capability/keeper/keeper.go | 4 ++-- x/capability/types/keys.go | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index 5ae963790e3..2ad0550c749 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -117,7 +117,7 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { // check if memory store has not been initialized yet by checking if initialized flag is nil. memStore := noGasCtx.KVStore(k.memKey) - flag := memStore.Get(types.MemInitializedKey()) + flag := memStore.Get(types.KeyMemInitialized) if flag == nil { prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) iterator := sdk.KVStorePrefixIterator(prefixStore, nil) @@ -135,7 +135,7 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { } // set the initialized flag so we don't rerun initialization logic - memStore.Set(types.MemInitializedKey(), []byte{1}) + memStore.Set(types.KeyMemInitialized, []byte{1}) } } diff --git a/x/capability/types/keys.go b/x/capability/types/keys.go index 5424167557d..aefd13ba228 100644 --- a/x/capability/types/keys.go +++ b/x/capability/types/keys.go @@ -52,8 +52,3 @@ func IndexToKey(index uint64) []byte { func IndexFromKey(key []byte) uint64 { return sdk.BigEndianToUint64(key) } - -// MemInitializedKey returns the key to store the initialized flag in memory store -func MemInitializedKey() []byte { - return []byte(KeyMemInitialized) -} From 5cd6ebad654b0bc79306b1fdc05f28b6ccac1f98 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Tue, 3 Aug 2021 11:41:48 +0200 Subject: [PATCH 04/10] address reviews --- x/capability/capability_test.go | 17 ++++++++++++++--- x/capability/keeper/keeper.go | 17 ++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/x/capability/capability_test.go b/x/capability/capability_test.go index ae6df05a373..a197f00dd83 100644 --- a/x/capability/capability_test.go +++ b/x/capability/capability_test.go @@ -58,12 +58,14 @@ func (suite *CapabilityTestSuite) TestInitializeMemStore() { // Mock App startup ctx := suite.app.BaseApp.NewUncachedContext(false, tmproto.Header{}) newKeeper.InitializeAndSeal(ctx) + suite.Require().False(newKeeper.IsInitialized(ctx), "memstore initialized flag set before BeginBlock") - // Mock app beginblock and ensure that no gas has been consumed + // Mock app beginblock and ensure that no gas has been consumed and memstore is initialized ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewGasMeter(50)) prevGas := ctx.BlockGasMeter().GasConsumed() restartedModule := capability.NewAppModule(suite.cdc, *newKeeper) restartedModule.BeginBlock(ctx, abci.RequestBeginBlock{}) + suite.Require().True(newKeeper.IsInitialized(ctx), "memstore initialized flag not set") gasUsed := ctx.BlockGasMeter().GasConsumed() suite.Require().Equal(prevGas, gasUsed, "beginblocker consumed gas during execution") @@ -72,13 +74,22 @@ func (suite *CapabilityTestSuite) TestInitializeMemStore() { // by using a cached context and discarding all cached writes. cacheCtx, _ := ctx.CacheContext() _, ok := newSk1.GetCapability(cacheCtx, "transfer") - suite.Require().True((ok)) + suite.Require().True(ok) // Ensure that the second transaction can still receive capability even if first tx fails. ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}) - _, ok = newSk1.GetCapability(ctx, "transfer") + cap1, ok = newSk1.GetCapability(ctx, "transfer") + suite.Require().True(ok) + + // Ensure the capabilities don't get reinitialized on next BeginBlock + // by testing to see if capability returns same pointer + // also check that initialized flag is still set + restartedModule.BeginBlock(ctx, abci.RequestBeginBlock{}) + recap, ok := newSk1.GetCapability(ctx, "transfer") suite.Require().True(ok) + suite.Require().Equal(cap1, recap, "capabilities got reinitialized after second BeginBlock") + suite.Require().True(newKeeper.IsInitialized(ctx), "memstore initialized flag not set") } func TestCapabilityTestSuite(t *testing.T) { diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index 2ad0550c749..d7cf36420ac 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -89,10 +89,8 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper { } } -// InitializeAndSeal loads all capabilities from the persistent KVStore into the -// in-memory store and seals the keeper to prevent further modules from creating -// a scoped keeper. InitializeAndSeal must be called once after the application -// state is loaded. +// InitializeAndSeal seals the keeper to prevent further modules from creating +// a scoped keeper. It also panics if the memory store is not of storetype `StoreTypeMemory`. func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { if k.sealed { panic("cannot initialize and seal an already sealed capability keeper") @@ -116,9 +114,7 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { noGasCtx := ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) // check if memory store has not been initialized yet by checking if initialized flag is nil. - memStore := noGasCtx.KVStore(k.memKey) - flag := memStore.Get(types.KeyMemInitialized) - if flag == nil { + if !k.IsInitialized(noGasCtx) { prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) iterator := sdk.KVStorePrefixIterator(prefixStore, nil) @@ -135,10 +131,17 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { } // set the initialized flag so we don't rerun initialization logic + memStore := noGasCtx.KVStore(k.memKey) memStore.Set(types.KeyMemInitialized, []byte{1}) } } +// IsInitialized returns true if the initialized flag is set, and false otherwise +func (k *Keeper) IsInitialized(ctx sdk.Context) bool { + memStore := ctx.KVStore(k.memKey) + return memStore.Get(types.KeyMemInitialized) != nil +} + // InitializeIndex sets the index to one (or greater) in InitChain according // to the GenesisState. It must only be called once. // It will panic if the provided index is 0, or if the index is already set. From acc659ddcc728f7a8674024a7701b40a0d61d165 Mon Sep 17 00:00:00 2001 From: Aditya Date: Tue, 3 Aug 2021 17:13:58 +0200 Subject: [PATCH 05/10] Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f875191364..5f3c398d8a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminsim issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). +* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminism issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). ### Client Breaking Changes From acdbca4cf6267348a5a91a87eaad8994cb708cba Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 4 Aug 2021 11:35:37 +0200 Subject: [PATCH 06/10] address reviews --- CHANGELOG.md | 2 +- x/capability/abci.go | 21 --------------------- x/capability/module.go | 12 ++++++++++-- 3 files changed, 11 insertions(+), 24 deletions(-) delete mode 100644 x/capability/abci.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f875191364..64f869abf0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminsim issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). +* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminsim issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). Applications must now include the capability module in their BeginBlocker order before any module that uses capabilities gets run. ### Client Breaking Changes diff --git a/x/capability/abci.go b/x/capability/abci.go deleted file mode 100644 index 9d1c94b5b90..00000000000 --- a/x/capability/abci.go +++ /dev/null @@ -1,21 +0,0 @@ -package capability - -import ( - "time" - - "github.com/cosmos/cosmos-sdk/telemetry" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/capability/keeper" - "github.com/cosmos/cosmos-sdk/x/capability/types" -) - -// BeginBlocker will call InitMemStore to initialize the memory stores in the case -// that this is the first time the node is executing a block since restarting (wiping memory). -// In this case, the BeginBlocker method will reinitialize the memory stores locally, so that subsequent -// capability transactions will pass. -// Otherwise BeginBlocker performs a no-op. -func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { - defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - - k.InitMemStore(ctx) -} diff --git a/x/capability/module.go b/x/capability/module.go index 388d8021074..124712647e5 100644 --- a/x/capability/module.go +++ b/x/capability/module.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "math/rand" + "time" "github.com/gorilla/mux" "github.com/grpc-ecosystem/grpc-gateway/runtime" @@ -14,6 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" cdctypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/module" simtypes "github.com/cosmos/cosmos-sdk/types/simulation" @@ -136,9 +138,15 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONMarshaler) json return cdc.MustMarshalJSON(genState) } -// BeginBlock executes all ABCI BeginBlock logic respective to the capability module. +// BeginBlocker will call InitMemStore to initialize the memory stores in the case +// that this is the first time the node is executing a block since restarting (wiping memory). +// In this case, the BeginBlocker method will reinitialize the memory stores locally, so that subsequent +// capability transactions will pass. +// Otherwise BeginBlocker performs a no-op. func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) { - BeginBlocker(ctx, am.keeper) + defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) + + am.keeper.InitMemStore(ctx) } // EndBlock executes all ABCI EndBlock logic respective to the capability module. It From 346eb0c00969363b3fd78aae0d8da77ab0cdcda7 Mon Sep 17 00:00:00 2001 From: Aditya Date: Wed, 4 Aug 2021 11:38:29 +0200 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Robert Zaremba --- x/capability/keeper/keeper.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index d7cf36420ac..58e651a0af1 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -106,9 +106,11 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { k.sealed = true } -// InitMemStore will initialize the memory store if the initialized flag is not set. -// InitMemStore logic should be run in first `BeginBlock` or `InitChain` (whichever is run on app start) -// so that the memory store is properly initialized before any transactions are run. +// InitMemStore will initialize the memory store if it hasn't been initialized yet. +// The function is safe to be called multiple times. +// InitMemStore must be called every time the app starts before the keeper is used (so +// `BeginBlock` or `InitChain` - whichever is first). We need access to the store so we +// can't initialize it in a constructor. func (k *Keeper) InitMemStore(ctx sdk.Context) { // create context with no block gas meter to ensure we do not consume gas during local initialization logic. noGasCtx := ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) @@ -136,7 +138,7 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { } } -// IsInitialized returns true if the initialized flag is set, and false otherwise +// IsInitialized returns true if the keeper is properly initialized, and false otherwise func (k *Keeper) IsInitialized(ctx sdk.Context) bool { memStore := ctx.KVStore(k.memKey) return memStore.Get(types.KeyMemInitialized) != nil From cd92c301b2e90645373a490f932e82fb515fe258 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 4 Aug 2021 11:40:11 +0200 Subject: [PATCH 08/10] move store check --- x/capability/keeper/keeper.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index d7cf36420ac..134f4e72ddd 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -96,13 +96,6 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { panic("cannot initialize and seal an already sealed capability keeper") } - memStore := ctx.KVStore(k.memKey) - memStoreType := memStore.GetStoreType() - - if memStoreType != sdk.StoreTypeMemory { - panic(fmt.Sprintf("invalid memory store type; got %s, expected: %s", memStoreType, sdk.StoreTypeMemory)) - } - k.sealed = true } @@ -113,6 +106,13 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { // create context with no block gas meter to ensure we do not consume gas during local initialization logic. noGasCtx := ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) + memStore := noGasCtx.KVStore(k.memKey) + memStoreType := memStore.GetStoreType() + + if memStoreType != sdk.StoreTypeMemory { + panic(fmt.Sprintf("invalid memory store type; got %s, expected: %s", memStoreType, sdk.StoreTypeMemory)) + } + // check if memory store has not been initialized yet by checking if initialized flag is nil. if !k.IsInitialized(noGasCtx) { prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) @@ -131,7 +131,6 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { } // set the initialized flag so we don't rerun initialization logic - memStore := noGasCtx.KVStore(k.memKey) memStore.Set(types.KeyMemInitialized, []byte{1}) } } From 1dbb174e3c6434af5343b3f506aa78d20da0f4dd Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 4 Aug 2021 11:52:54 +0200 Subject: [PATCH 09/10] add api breaking changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64f869abf0f..f9cb1d8e16f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) Moved capability initialization logic to BeginBlocker to fix nondeterminsim issue mentioned in [\#9800](https://github.com/cosmos/cosmos-sdk/issues/9800). Applications must now include the capability module in their BeginBlocker order before any module that uses capabilities gets run. +### API Breaking Changes + +* [\#9835](https://github.com/cosmos/cosmos-sdk/pull/9835) The `InitializeAndSeal` API has not changed, however it no longer initializes the in-memory state. `InitMemStore` has been introduced to serve this function, which will be called either in `InitChain` or `BeginBlock` (whichever is first after app start). Nodes may run this version on a network running 0.42.x, however, they must update their app.go files to include the capability module in their begin blockers. + ### Client Breaking Changes * [\#9781](https://github.com/cosmos/cosmos-sdk/pull/9781) Improve`withdraw-all-rewards` UX when broadcast mode `async` or `async` is used. From e8f1885ff24bda394c4c5a37256565009738b050 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 4 Aug 2021 12:04:10 +0200 Subject: [PATCH 10/10] fix lint --- x/capability/keeper/keeper.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index b7df98b5ed7..4002c199a4f 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -101,8 +101,8 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { // InitMemStore will initialize the memory store if it hasn't been initialized yet. // The function is safe to be called multiple times. -// InitMemStore must be called every time the app starts before the keeper is used (so -// `BeginBlock` or `InitChain` - whichever is first). We need access to the store so we +// InitMemStore must be called every time the app starts before the keeper is used (so +// `BeginBlock` or `InitChain` - whichever is first). We need access to the store so we // can't initialize it in a constructor. func (k *Keeper) InitMemStore(ctx sdk.Context) { // create context with no block gas meter to ensure we do not consume gas during local initialization logic.