fix!: Capability Issue on Restart, Backport to v0.43 (backport #9836)

CHANGELOG.md

@@ -46,6 +46,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * [\#9750](https://github.com/cosmos/cosmos-sdk/pull/9750) Emit events for tx signature and sequence, so clients can now query txs by signature (`tx.signature='<base64_sig>'`) or by address and sequence combo (`tx.acc_seq='<addr>/<seq>'`).
 
+### API Breaking Changes
+
+* (x/capability) [\#9836](https://github.com/cosmos/cosmos-sdk/pull/9836) Removed `InitializeAndSeal(ctx sdk.Context)` and replaced with `Seal()`.
+
 ### Improvements
 
 * (cli) [\#9717](https://github.com/cosmos/cosmos-sdk/pull/9717) Added CLI flag `--output json/text` to `tx` cli commands.
@@ -56,7 +60,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Client-Breaking Changes
 
-* [\#9785](https://github.com/cosmos/cosmos-sdk/issues/9785) Missing coin denomination in logs 
+* [\#9785](https://github.com/cosmos/cosmos-sdk/issues/9785) Missing coin denomination in logs
 
 ### CLI Breaking Changes
 

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"
@@ -210,7 +209,9 @@ func NewSimApp(
 		authzkeeper.StoreKey,
 	)
 	tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey)
-	memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey)
+	// 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,
@@ -229,6 +230,9 @@ func NewSimApp(
 	bApp.SetParamStore(app.ParamsKeeper.Subspace(baseapp.Paramspace).WithKeyTable(paramskeeper.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(
@@ -324,8 +328,9 @@ 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, 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)
@@ -401,16 +406,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

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)
+}

x/capability/capability_test.go

@@ -0,0 +1,97 @@
+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.Codec
+	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("testingkey"))
+	newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName)
+
+	// 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 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")
+
+	// 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{})
+
+	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) {
+	suite.Run(t, new(CapabilityTestSuite))
+}

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.

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")
+}

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
@@ -97,38 +89,58 @@ 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.
+func (k *Keeper) Seal() {
 	if k.sealed {
 		panic("cannot initialize and seal an already sealed capability keeper")
 	}
 
+	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))
 	}
 
-	prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixIndexCapability)
-	iterator := sdk.KVStorePrefixIterator(prefixStore, nil)
+	// 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.
+	if !k.IsInitialized(noGasCtx) {
+		prefixStore := prefix.NewStore(noGasCtx.KVStore(k.storeKey), types.KeyPrefixIndexCapability)
+		iterator := sdk.KVStorePrefixIterator(prefixStore, nil)
+
+		// initialize the in-memory store for all persisted capabilities
+		defer iterator.Close()
 
-	// initialize the in-memory store for all persisted capabilities
-	defer iterator.Close()
+		for ; iterator.Valid(); iterator.Next() {
+			index := types.IndexFromKey(iterator.Key())
 
-	for ; iterator.Valid(); iterator.Next() {
-		index := types.IndexFromKey(iterator.Key())
+			var capOwners types.CapabilityOwners
 
-		var capOwners types.CapabilityOwners
+			k.cdc.MustUnmarshal(iterator.Value(), &capOwners)
+			k.InitializeCapability(noGasCtx, index, capOwners)
+		}
 
-		k.cdc.MustUnmarshal(iterator.Value(), &capOwners)
-		k.InitializeCapability(ctx, index, capOwners)
+		// set the initialized flag so we don't rerun initialization logic
+		memStore := noGasCtx.KVStore(k.memKey)
+		memStore.Set(types.KeyMemInitialized, []byte{1})
 	}
+}
 
-	k.sealed = true
+// 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
@@ -350,22 +362,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
 	}