fix: verify consumer proposals execution to prevent provider halts (#602

)

* feat: ensure consumer proposals execution can't panic during BeginBlock

    - Verify proposals execution using a cached context in BeginBlock and consumer proposal handlers
    - Drop invalid consumer props in BeginBlock instead of panicing

* fix integration test submit prop to have enough gas

* fix linter

tests/e2e/unbonding.go

@@ -304,9 +304,10 @@ func (s *CCVTestSuite) TestUnbondingNoConsumer() {
 	providerKeeper := s.providerApp.GetProviderKeeper()
 	providerStakingKeeper := s.providerApp.GetE2eStakingKeeper()
 
-	// remove all consumer chains, which were already registered during setup
+	// remove all consumer chains, which were already started during setup
 	for chainID := range s.consumerBundles {
-		providerKeeper.DeleteConsumerClientId(s.providerCtx(), chainID)
+		err := providerKeeper.StopConsumerChain(s.providerCtx(), chainID, true)
+		s.Require().NoError(err)
 	}
 
 	// delegate bondAmt and undelegate 1/2 of it
@@ -341,8 +342,9 @@ func (s *CCVTestSuite) TestRedelegationNoConsumer() {
 	providerKeeper := s.providerApp.GetProviderKeeper()
 	stakingKeeper := s.providerApp.GetE2eStakingKeeper()
 
-	// remove the consumer chain, which was already registered during setup
-	providerKeeper.DeleteConsumerClientId(s.providerCtx(), s.consumerChain.ChainID)
+	// stop the consumer chain, which was already started during setup
+	err := providerKeeper.StopConsumerChain(s.providerCtx(), s.consumerChain.ChainID, true)
+	s.Require().NoError(err)
 
 	// Setup delegator, bond amount, and src/dst validators
 	bondAmt := sdk.NewInt(10000000)

tests/integration/actions.go

@@ -255,6 +255,7 @@ func (tr TestRun) submitConsumerAdditionProposal(
 		`--from`, `validator`+fmt.Sprint(action.from),
 		`--chain-id`, string(tr.chainConfigs[action.chain].chainId),
 		`--home`, tr.getValidatorHome(action.chain, action.from),
+		`--gas`, `900000`,
 		`--node`, tr.getValidatorNode(action.chain, action.from),
 		`--keyring-backend`, `test`,
 		`-b`, `block`,

testutil/keeper/expectations.go

@@ -31,7 +31,7 @@ func GetMocksForCreateConsumerClient(ctx sdk.Context, mocks *MockedKeepers,
 	// append MakeConsumerGenesis and CreateClient expectations
 	expectations := GetMocksForMakeConsumerGenesis(ctx, mocks, time.Hour)
 	createClientExp := mocks.MockClientKeeper.EXPECT().CreateClient(
-		ctx,
+		gomock.Any(),
 		// Allows us to expect a match by field. These are the only two client state values
 		// that are dependant on parameters passed to CreateConsumerClient.
 		extra.StructMatcher().Field(
@@ -48,13 +48,14 @@ func GetMocksForCreateConsumerClient(ctx sdk.Context, mocks *MockedKeepers,
 // GetMocksForMakeConsumerGenesis returns mock expectations needed to call MakeConsumerGenesis().
 func GetMocksForMakeConsumerGenesis(ctx sdk.Context, mocks *MockedKeepers,
 	unbondingTimeToInject time.Duration) []*gomock.Call {
+
 	return []*gomock.Call{
-		mocks.MockStakingKeeper.EXPECT().UnbondingTime(ctx).Return(unbondingTimeToInject).Times(1),
+		mocks.MockStakingKeeper.EXPECT().UnbondingTime(gomock.Any()).Return(unbondingTimeToInject).Times(1),
 
-		mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(ctx,
+		mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(),
 			clienttypes.GetSelfHeight(ctx)).Return(&ibctmtypes.ConsensusState{}, nil).Times(1),
 
-		mocks.MockStakingKeeper.EXPECT().IterateLastValidatorPowers(ctx, gomock.Any()).Times(1),
+		mocks.MockStakingKeeper.EXPECT().IterateLastValidatorPowers(gomock.Any(), gomock.Any()).Times(1),
 	}
 }
 
@@ -82,11 +83,11 @@ func GetMocksForSetConsumerChain(ctx sdk.Context, mocks *MockedKeepers,
 func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomock.Call {
 	dummyCap := &capabilitytypes.Capability{}
 	return []*gomock.Call{
-		mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, ccv.ProviderPortID, "channelID").Return(
+		mocks.MockChannelKeeper.EXPECT().GetChannel(gomock.Any(), ccv.ProviderPortID, "channelID").Return(
 			channeltypes.Channel{State: channeltypes.OPEN}, true,
 		).Times(1),
-		mocks.MockScopedKeeper.EXPECT().GetCapability(ctx, gomock.Any()).Return(dummyCap, true).Times(1),
-		mocks.MockChannelKeeper.EXPECT().ChanCloseInit(ctx, ccv.ProviderPortID, "channelID", dummyCap).Times(1),
+		mocks.MockScopedKeeper.EXPECT().GetCapability(gomock.Any(), gomock.Any()).Return(dummyCap, true).Times(1),
+		mocks.MockChannelKeeper.EXPECT().ChanCloseInit(gomock.Any(), ccv.ProviderPortID, "channelID", dummyCap).Times(1),
 	}
 }
 

x/ccv/provider/keeper/proposal.go

@@ -22,15 +22,17 @@ import (
 )
 
 // HandleConsumerAdditionProposal will receive the consumer chain's client state from the proposal.
-// If the spawn time has already passed, then set the consumer chain. Otherwise store the client
-// as a pending client, and set once spawn time has passed.
+// If the client can be successfully created in a cached context, it stores the proposal as a pending proposal.
 //
 // Note: This method implements SpawnConsumerChainProposalHandler in spec.
 // See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-hcaprop1
 // Spec tag: [CCV-PCF-HCAPROP.1]
 func (k Keeper) HandleConsumerAdditionProposal(ctx sdk.Context, p *types.ConsumerAdditionProposal) error {
-	if !ctx.BlockTime().Before(p.SpawnTime) {
-		return k.CreateConsumerClient(ctx, p)
+
+	// verify the consumer addition proposal execution
+	// in cached context and discard the cached writes
+	if _, _, err := k.CreateConsumerClientInCachedCtx(ctx, *p); err != nil {
+		return err
 	}
 
 	err := k.SetPendingConsumerAdditionProp(ctx, p)
@@ -50,17 +52,17 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi
 
 	chainID := prop.ChainId
 	// check that a client for this chain does not exist
-	if _, found := k.GetConsumerClientId(ctx, prop.ChainId); found {
-		// drop the proposal
-		return nil
+	if _, found := k.GetConsumerClientId(ctx, chainID); found {
+		return sdkerrors.Wrap(ccv.ErrDuplicateConsumerChain,
+			fmt.Sprintf("cannot create client for existent consumer chain: %s", chainID))
 	}
 
 	// Consumers always start out with the default unbonding period
 	consumerUnbondingPeriod := prop.UnbondingPeriod
 
 	// Create client state by getting template client from parameters and filling in zeroed fields from proposal.
 	clientState := k.GetTemplateClient(ctx)
-	clientState.ChainId = prop.ChainId
+	clientState.ChainId = chainID
 	clientState.LatestHeight = prop.InitialHeight
 	clientState.TrustingPeriod = consumerUnbondingPeriod / time.Duration(k.GetTrustingPeriodFraction(ctx))
 	clientState.UnbondingPeriod = consumerUnbondingPeriod
@@ -108,15 +110,16 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi
 }
 
 // HandleConsumerRemovalProposal stops a consumer chain and released the outstanding unbonding operations.
-// If the stop time hasn't already passed, it stores the proposal as a pending proposal.
+// If the consumer can be successfully stopped in a cached context, it stores the proposal as a pending proposal.
 //
 // This method implements StopConsumerChainProposalHandler from spec.
 // See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-hcrprop1
 // Spec tag: [CCV-PCF-HCRPROP.1]
 func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.ConsumerRemovalProposal) error {
-
-	if !ctx.BlockTime().Before(p.StopTime) {
-		return k.StopConsumerChain(ctx, p.ChainId, true)
+	// verify the consumer removal proposal execution
+	// in cached context and discard the cached writes
+	if _, _, err := k.StopConsumerChainInCachedCtx(ctx, *p); err != nil {
+		return err
 	}
 
 	k.SetPendingConsumerRemovalProp(ctx, p.ChainId, p.StopTime)
@@ -132,8 +135,8 @@ func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.Consumer
 func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan bool) (err error) {
 	// check that a client for chainID exists
 	if _, found := k.GetConsumerClientId(ctx, chainID); !found {
-		// drop the proposal
-		return nil
+		return sdkerrors.Wrap(ccv.ErrConsumerChainNotFound,
+			fmt.Sprintf("cannot stop non-existent consumer chain: %s", chainID))
 	}
 
 	// clean up states
@@ -332,11 +335,18 @@ func (k Keeper) BeginBlockInit(ctx sdk.Context) {
 	propsToExecute := k.ConsumerAdditionPropsToExecute(ctx)
 
 	for _, prop := range propsToExecute {
-		p := prop
-		err := k.CreateConsumerClient(ctx, &p)
+		// create consumer client in a cached context to handle errors
+		cachedCtx, writeFn, err := k.CreateConsumerClientInCachedCtx(ctx, prop)
 		if err != nil {
-			panic(fmt.Errorf("consumer client could not be created: %w", err))
+			// drop the proposal
+			ctx.Logger().Info("consumer client could not be created: %w", err)
+			continue
 		}
+		// The cached context is created with a new EventManager so we merge the event
+		// into the original context
+		ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events())
+		// write cache
+		writeFn()
 	}
 	// delete the executed proposals
 	k.DeletePendingConsumerAdditionProps(ctx, propsToExecute...)
@@ -457,10 +467,18 @@ func (k Keeper) BeginBlockCCR(ctx sdk.Context) {
 	propsToExecute := k.ConsumerRemovalPropsToExecute(ctx)
 
 	for _, prop := range propsToExecute {
-		err := k.StopConsumerChain(ctx, prop.ChainId, true)
+		// stop consumer chain in a cached context to handle errors
+		cachedCtx, writeFn, err := k.StopConsumerChainInCachedCtx(ctx, prop)
 		if err != nil {
-			panic(fmt.Errorf("consumer chain failed to stop: %w", err))
+			// drop the proposal
+			ctx.Logger().Info("consumer chain could not be stopped: %w", err)
+			continue
 		}
+		// The cached context is created with a new EventManager so we merge the event
+		// into the original context
+		ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events())
+		// write cache
+		writeFn()
 	}
 	// delete the executed proposals
 	k.DeletePendingConsumerRemovalProps(ctx, propsToExecute...)
@@ -544,3 +562,19 @@ func (k Keeper) CloseChannel(ctx sdk.Context, channelID string) {
 		}
 	}
 }
+
+// CreateConsumerClientInCachedCtx creates a consumer client
+// from a given consumer addition proposal in a cached context
+func (k Keeper) CreateConsumerClientInCachedCtx(ctx sdk.Context, p types.ConsumerAdditionProposal) (cc sdk.Context, writeCache func(), err error) {
+	cc, writeCache = ctx.CacheContext()
+	err = k.CreateConsumerClient(cc, &p)
+	return
+}
+
+// StopConsumerChainInCachedCtx stop a consumer chain
+// from a given consumer removal proposal in a cached context
+func (k Keeper) StopConsumerChainInCachedCtx(ctx sdk.Context, p types.ConsumerRemovalProposal) (cc sdk.Context, writeCache func(), err error) {
+	cc, writeCache = ctx.CacheContext()
+	err = k.StopConsumerChain(ctx, p.ChainId, true)
+	return
+}