fix!: Remove panics on failure to send IBC packets (#876)

* provider: replace panic with StopConsumerChain

* provider: replace panic with error message

* Info logging on client expiration

* add test for consumer

* add test for provider

* linter

* Update CHANGELOG.md

---------

Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>

CHANGELOG.md

@@ -21,6 +21,7 @@ Some PRs from v1.3.0 may reappear from other releases below. This is due to the
 
 ## PRs included in v1.3.0
 
+* (fix) Remove panics on failure to send IBC packets [#876](https://github.com/cosmos/interchain-security/pull/876)
 * (fix) consumer key prefix order to avoid complex migrations [#963](https://github.com/cosmos/interchain-security/pull/963)
 * (fix) Prevent denom DOS [#931](https://github.com/cosmos/interchain-security/pull/931)
 * (fix) multisig for assigning consumer key, use json [#916](https://github.com/cosmos/interchain-security/pull/916)

x/ccv/consumer/keeper/relay.go

@@ -196,13 +196,17 @@ func (k Keeper) SendPackets(ctx sdk.Context) {
 		)
 		if err != nil {
 			if clienttypes.ErrClientNotActive.Is(err) {
-				k.Logger(ctx).Debug("IBC client is inactive, packet remains in queue", "type", p.Type.String())
+				// IBC client is expired!
 				// leave the packet data stored to be sent once the client is upgraded
+				k.Logger(ctx).Info("IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String())
 				return
 			}
-			// something went wrong when sending the packet
-			// TODO do not panic if the send fails
-			panic(fmt.Errorf("packet could not be sent over IBC: %w", err))
+			// Not able to send packet over IBC!
+			// Leave the packet data stored for the sent to be retried in the next block.
+			// Note that if VSCMaturedPackets are not sent for long enough, the provider
+			// will remove the consumer anyway.
+			k.Logger(ctx).Error("cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), "err", err.Error())
+			return
 		}
 	}
 

x/ccv/consumer/keeper/relay_test.go

@@ -280,3 +280,27 @@ func TestOnAcknowledgementPacket(t *testing.T) {
 	err = consumerKeeper.OnAcknowledgementPacket(ctx, packet, ack)
 	require.Nil(t, err)
 }
+
+// TestSendPackets tests the SendPackets method failing
+func TestSendPacketsFailure(t *testing.T) {
+	// Keeper setup
+	consumerKeeper, ctx, ctrl, mocks := testkeeper.GetConsumerKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	consumerKeeper.SetProviderChannel(ctx, "consumerCCVChannelID")
+	consumerKeeper.SetParams(ctx, consumertypes.DefaultParams())
+
+	// Set some pending packets
+	consumerKeeper.SetPendingPackets(ctx, types.ConsumerPacketDataList{List: []types.ConsumerPacketData{
+		{}, {}, {},
+	}})
+
+	// Mock the channel keeper to return an error
+	gomock.InOrder(
+		mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID,
+			"consumerCCVChannelID").Return(channeltypes.Channel{}, false).Times(1),
+	)
+
+	// No panic should occur, pending packets should not be cleared
+	consumerKeeper.SendPackets(ctx)
+	require.Equal(t, 3, len(consumerKeeper.GetPendingPackets(ctx).List))
+}

x/ccv/provider/keeper/relay.go

@@ -194,12 +194,17 @@ func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string
 				// IBC client is expired!
 				// leave the packet data stored to be sent once the client is upgraded
 				// the client cannot expire during iteration (in the middle of a block)
-				k.Logger(ctx).Debug("IBC client is expired, cannot send VSC, leaving packet data stored:", "chainID", chainID, "vscid", data.ValsetUpdateId)
+				k.Logger(ctx).Info("IBC client is expired, cannot send VSC, leaving packet data stored:", "chainID", chainID, "vscid", data.ValsetUpdateId)
 				return
 			}
-			// TODO do not panic if the send fails
-			// https://github.com/cosmos/interchain-security/issues/649
-			panic(fmt.Errorf("packet could not be sent over IBC: %w", err))
+			// Not able to send packet over IBC!
+			k.Logger(ctx).Error("cannot send VSC, removing consumer:", "chainID", chainID, "vscid", data.ValsetUpdateId, "err", err.Error())
+			// If this happens, most likely the consumer is malicious; remove it
+			err := k.StopConsumerChain(ctx, chainID, true)
+			if err != nil {
+				panic(fmt.Errorf("consumer chain failed to stop: %w", err))
+			}
+			return
 		}
 		// set the VSC send timestamp for this packet;
 		// note that the VSC send timestamp are set when the packets

x/ccv/provider/keeper/relay_test.go

@@ -676,3 +676,40 @@ func TestHandleVSCMaturedPacket(t *testing.T) {
 	_, found = pk.GetUnbondingOpIndex(ctx, "chain-1", 3)
 	require.False(t, found)
 }
+
+// TestSendVSCPacketsToChainFailure tests the SendVSCPacketsToChain method failing
+func TestSendVSCPacketsToChainFailure(t *testing.T) {
+	// Keeper setup
+	providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	providerKeeper.SetParams(ctx, providertypes.DefaultParams())
+
+	// Append mocks for full consumer setup
+	mockCalls := testkeeper.GetMocksForSetConsumerChain(ctx, &mocks, "consumerChainID")
+
+	// Set 3 pending vsc packets
+	providerKeeper.AppendPendingVSCPackets(ctx, "consumerChainID", []ccv.ValidatorSetChangePacketData{{}, {}, {}}...)
+
+	// append mocks for the channel keeper to return an error
+	mockCalls = append(mockCalls,
+		mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, ccv.ProviderPortID,
+			"CCVChannelID").Return(channeltypes.Channel{}, false).Times(1),
+	)
+
+	// Append mocks for expected call to StopConsumerChain
+	mockCalls = append(mockCalls, testkeeper.GetMocksForStopConsumerChain(ctx, &mocks)...)
+
+	// Assert mock calls hit
+	gomock.InOrder(mockCalls...)
+
+	// Execute setup
+	err := providerKeeper.SetConsumerChain(ctx, "channelID")
+	require.NoError(t, err)
+	providerKeeper.SetConsumerClientId(ctx, "consumerChainID", "clientID")
+
+	// No panic should occur, StopConsumerChain should be called
+	providerKeeper.SendVSCPacketsToChain(ctx, "consumerChainID", "CCVChannelID")
+
+	// Pending VSC packets should be deleted in StopConsumerChain
+	require.Empty(t, providerKeeper.GetPendingVSCPackets(ctx, "consumerChainID"))
+}