From 89d6a7d85cbb27de314310e7c45e57a63251c885 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Mon, 22 May 2023 16:52:46 +0200 Subject: [PATCH] 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 | 1 + x/ccv/consumer/keeper/relay.go | 12 ++++++---- x/ccv/consumer/keeper/relay_test.go | 24 +++++++++++++++++++ x/ccv/provider/keeper/relay.go | 13 ++++++---- x/ccv/provider/keeper/relay_test.go | 37 +++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cc75efe60..2c4e0937dd 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index c690ef3701..fe8455563e 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/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 } } diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index c23a553225..fc09a6b250 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/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)) +} diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index a669d47697..55fff4e039 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/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 diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 0ad7d21548..250d241f5e 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/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")) +}