From 449f412c7573b28c99bb430b6edc1a6098fd48bd Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 18:56:05 +0200 Subject: [PATCH 1/8] fix cherry-pick --- 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, 215 insertions(+), 37 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 4ce4c791afb..a118894cf6b 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -208,7 +208,7 @@ func NewSimApp( authzkeeper.StoreKey, ) tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) - memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey) + memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey, "testing") app := &SimApp{ BaseApp: bApp, @@ -323,7 +323,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, ) 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 ead46dd1f4f..b354d5dc6f9 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,44 @@ 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() +<<<<<<< HEAD k.cdc.MustUnmarshal(iterator.Value(), &capOwners) k.InitializeCapability(ctx, index, capOwners) } +======= + for ; iterator.Valid(); iterator.Next() { + index := types.IndexFromKey(iterator.Key()) +>>>>>>> 8570d1bb6a... implement BeginBlock fix - 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 +364,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 2868fd1d9b8..dfc9c455f1f 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 9d298c26e0b..5d6e8ffcbb0 100644 --- a/x/capability/module.go +++ b/x/capability/module.go @@ -143,7 +143,9 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw func (AppModule) ConsensusVersion() uint64 { return 1 } // 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 e9e0d49209a20d15d87fd99c864eee431394f63b Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 19:48:15 +0200 Subject: [PATCH 2/8] Remove InitializeAndSeal logic and replace with Seal --- simapp/app.go | 10 ---------- x/capability/keeper/keeper.go | 15 ++++----------- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index a118894cf6b..ec37ce977c6 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -399,16 +399,6 @@ func NewSimApp( if err := app.LoadLatestVersion(); err != nil { tmos.Exit(err.Error()) } - - // Initialize and seal the capability keeper so all persistent capabilities - // are loaded in-memory and prevent any further modules from creating scoped - // sub-keepers. - // This must be done during creation of baseapp rather than in InitChain so - // that in-memory capabilities get regenerated on app restart. - // Note that since this reads from the store, we can only perform it when - // `loadLatest` is set to true. - ctx := app.BaseApp.NewUncachedContext(true, tmproto.Header{}) - app.CapabilityKeeper.InitializeAndSeal(ctx) } return app diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index b354d5dc6f9..53bef6588ba 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -89,11 +89,10 @@ 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. -func (k *Keeper) InitializeAndSeal(ctx sdk.Context) { +// Seal seals the keeper to prevent further modules from creating a scoped keeper. +// Seal may be called during app initialization for applications that do not wish to create scoped keepers dynamically. +// Seal also asserts the memory store type is correct, and will panic if it does not have store type of `StoreTypeMemory`. +func (k *Keeper) Seal(ctx sdk.Context) { if k.sealed { panic("cannot initialize and seal an already sealed capability keeper") } @@ -125,14 +124,8 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { // initialize the in-memory store for all persisted capabilities defer iterator.Close() -<<<<<<< HEAD - k.cdc.MustUnmarshal(iterator.Value(), &capOwners) - k.InitializeCapability(ctx, index, capOwners) - } -======= for ; iterator.Valid(); iterator.Next() { index := types.IndexFromKey(iterator.Key()) ->>>>>>> 8570d1bb6a... implement BeginBlock fix var capOwners types.CapabilityOwners From 079f9c1bfdb6f601d0c79e077144672190a014a0 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 19:55:28 +0200 Subject: [PATCH 3/8] document changes in simapp and changelog --- simapp/app.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/simapp/app.go b/simapp/app.go index ec37ce977c6..44702e412d9 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -227,6 +227,9 @@ func NewSimApp( bApp.SetParamStore(app.ParamsKeeper.Subspace(baseapp.Paramspace).WithKeyTable(paramstypes.ConsensusParamsKeyTable())) app.CapabilityKeeper = capabilitykeeper.NewKeeper(appCodec, keys[capabilitytypes.StoreKey], memKeys[capabilitytypes.MemStoreKey]) + // Applications that wish to enforce statically created ScopedKeepers should call `Seal` after creating + // their scoped modules in `NewApp` with `ScopeToModule` + app.CapabilityKeeper.Seal() // add keepers app.AccountKeeper = authkeeper.NewAccountKeeper( From 13d76a6d54e5d03b47efc7925fd29317b8707302 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 20:00:55 +0200 Subject: [PATCH 4/8] add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6579ea3c70a..b48578700ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#9432](https://github.com/cosmos/cosmos-sdk/pull/9432) `ConsensusParamsKeyTable` moved from `params/keeper` to `params/types` * [\#9576](https://github.com/cosmos/cosmos-sdk/pull/9576) Add debug error message to `sdkerrors.QueryResult` when enabled * [\#9650](https://github.com/cosmos/cosmos-sdk/pull/9650) Removed deprecated message handler implementation from the SDK modules. +* [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Removed `InitializeAndSeal(ctx sdk.Context)` and replaced with `Seal()`. ### Client Breaking Changes @@ -94,6 +95,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (server) [#9704](https://github.com/cosmos/cosmos-sdk/pull/9704) Start GRPCWebServer in goroutine, avoid blocking other services from starting. * [\#9762](https://github.com/cosmos/cosmos-sdk/pull/9762) The init command uses the chain-id from the client config if --chain-id is not provided * [\#9793](https://github.com/cosmos/cosmos-sdk/pull/9793) Fixed ECDSA/secp256r1 transaction malleability. +* [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Fixes capability initialization issue on tx revert by moving initialization logic to `InitChain` and `BeginBlock`. ### State Machine Breaking From e845eba9d133e992d759fc48182ce3c3590e6982 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 20:14:46 +0200 Subject: [PATCH 5/8] fix api and tests --- simapp/app.go | 1 - x/capability/capability_test.go | 4 ++-- x/capability/keeper/keeper.go | 21 ++++++++++----------- x/capability/keeper/keeper_test.go | 9 +++------ 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index 44702e412d9..c35c9fe9b1f 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -13,7 +13,6 @@ import ( abci "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/libs/log" tmos "github.com/tendermint/tendermint/libs/os" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" dbm "github.com/tendermint/tm-db" "github.com/cosmos/cosmos-sdk/baseapp" diff --git a/x/capability/capability_test.go b/x/capability/capability_test.go index ae6df05a373..fc195891ae1 100644 --- a/x/capability/capability_test.go +++ b/x/capability/capability_test.go @@ -20,7 +20,7 @@ import ( type CapabilityTestSuite struct { suite.Suite - cdc codec.Marshaler + cdc codec.Codec ctx sdk.Context app *simapp.SimApp keeper *keeper.Keeper @@ -57,7 +57,7 @@ func (suite *CapabilityTestSuite) TestInitializeMemStore() { // Mock App startup ctx := suite.app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - newKeeper.InitializeAndSeal(ctx) + newKeeper.Seal() // Mock app beginblock and ensure that no gas has been consumed ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewGasMeter(50)) diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go index 53bef6588ba..d1a3c26198a 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -91,31 +91,30 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper { // Seal seals the keeper to prevent further modules from creating a scoped keeper. // Seal may be called during app initialization for applications that do not wish to create scoped keepers dynamically. -// Seal also asserts the memory store type is correct, and will panic if it does not have store type of `StoreTypeMemory`. -func (k *Keeper) Seal(ctx sdk.Context) { +func (k *Keeper) Seal() { if k.sealed { 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 } // 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 also asserts the memory store type is correct, and will panic if it does not have store type of `StoreTypeMemory`. func (k *Keeper) InitMemStore(ctx sdk.Context) { + 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)) + } + // create context with no block gas meter to ensure we do not consume gas during local initialization logic. 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.MemInitializedKey()) if flag == nil { prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability) @@ -129,7 +128,7 @@ func (k *Keeper) InitMemStore(ctx sdk.Context) { var capOwners types.CapabilityOwners - k.cdc.MustUnmarshalBinaryBare(iterator.Value(), &capOwners) + k.cdc.MustUnmarshal(iterator.Value(), &capOwners) k.InitializeCapability(noGasCtx, index, capOwners) } diff --git a/x/capability/keeper/keeper_test.go b/x/capability/keeper/keeper_test.go index dfc9c455f1f..e7b9b2d4a32 100644 --- a/x/capability/keeper/keeper_test.go +++ b/x/capability/keeper/keeper_test.go @@ -7,7 +7,6 @@ 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" @@ -19,7 +18,6 @@ import ( type KeeperTestSuite struct { suite.Suite - cdc codec.Marshaler ctx sdk.Context app *simapp.SimApp keeper *keeper.Keeper @@ -36,10 +34,9 @@ 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() { +func (suite *KeeperTestSuite) TestSeal() { sk := suite.keeper.ScopeToModule(banktypes.ModuleName) suite.Require().Panics(func() { suite.keeper.ScopeToModule(" ") @@ -59,7 +56,7 @@ func (suite *KeeperTestSuite) TestInitializeAndSeal() { } suite.Require().NotPanics(func() { - suite.keeper.InitializeAndSeal(suite.ctx) + suite.keeper.Seal() }) for i, cap := range caps { @@ -70,7 +67,7 @@ func (suite *KeeperTestSuite) TestInitializeAndSeal() { } suite.Require().Panics(func() { - suite.keeper.InitializeAndSeal(suite.ctx) + suite.keeper.Seal() }) suite.Require().Panics(func() { From dd7bd05c35016b1c033abb5e9125e1c37aa6a083 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 2 Aug 2021 20:30:50 +0200 Subject: [PATCH 6/8] 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 d1a3c26198a..026dad96448 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -115,7 +115,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. - 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) @@ -133,7 +133,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 fe1dd5cf4b02efd221776874496d769221137584 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Tue, 3 Aug 2021 11:44:43 +0200 Subject: [PATCH 7/8] refactor based on suggestions --- x/capability/capability_test.go | 17 ++++++++++++++--- x/capability/keeper/keeper.go | 10 ++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/x/capability/capability_test.go b/x/capability/capability_test.go index fc195891ae1..8a4ad45b61f 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.Seal() + 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 026dad96448..48e0bdfe318 100644 --- a/x/capability/keeper/keeper.go +++ b/x/capability/keeper/keeper.go @@ -115,8 +115,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. - 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) @@ -133,10 +132,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 b6467b389146a9c4f251d1a6b1c050da6c404f5c Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Tue, 3 Aug 2021 13:32:23 +0200 Subject: [PATCH 8/8] address review --- CHANGELOG.md | 2 +- simapp/app.go | 5 ++++- x/capability/capability_test.go | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b48578700ef..a8f2738f449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,7 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#9432](https://github.com/cosmos/cosmos-sdk/pull/9432) `ConsensusParamsKeyTable` moved from `params/keeper` to `params/types` * [\#9576](https://github.com/cosmos/cosmos-sdk/pull/9576) Add debug error message to `sdkerrors.QueryResult` when enabled * [\#9650](https://github.com/cosmos/cosmos-sdk/pull/9650) Removed deprecated message handler implementation from the SDK modules. -* [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Removed `InitializeAndSeal(ctx sdk.Context)` and replaced with `Seal()`. +* (x/capability) [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Removed `InitializeAndSeal(ctx sdk.Context)` and replaced with `Seal()`. ### Client Breaking Changes diff --git a/simapp/app.go b/simapp/app.go index c35c9fe9b1f..c1cfce132cf 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -207,7 +207,9 @@ func NewSimApp( authzkeeper.StoreKey, ) tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) - memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey, "testing") + // NOTE: The testingkey is just mounted for testing purposes. Actual applications should + // not include this key. + memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey, "testingkey") app := &SimApp{ BaseApp: bApp, @@ -324,6 +326,7 @@ func NewSimApp( // there is nothing left over in the validator fee pool, so as to keep the // CanWithdrawInvariant invariant. // NOTE: staking module is required if HistoricalEntries param > 0 + // NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC) app.mm.SetOrderBeginBlockers( upgradetypes.ModuleName, capabilitytypes.ModuleName, minttypes.ModuleName, distrtypes.ModuleName, slashingtypes.ModuleName, evidencetypes.ModuleName, stakingtypes.ModuleName, diff --git a/x/capability/capability_test.go b/x/capability/capability_test.go index 8a4ad45b61f..45a5f6ea42a 100644 --- a/x/capability/capability_test.go +++ b/x/capability/capability_test.go @@ -52,7 +52,7 @@ func (suite *CapabilityTestSuite) TestInitializeMemStore() { 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")) + newKeeper := keeper.NewKeeper(suite.cdc, suite.app.GetKey(types.StoreKey), suite.app.GetMemKey("testingkey")) newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName) // Mock App startup