From 05eb431cce6b4cfc8ca0871dc46f466acdd369e3 Mon Sep 17 00:00:00 2001 From: mpoke Date: Sat, 22 Apr 2023 13:38:55 +0200 Subject: [PATCH 1/7] provider: replace panic with StopConsumerChain --- x/ccv/provider/keeper/relay.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index eeaa0edfb3..fe60ecc595 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -197,9 +197,14 @@ func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string k.Logger(ctx).Debug("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 From 808b1a0cf8cb1db0ad713fe204eba6551920d5a5 Mon Sep 17 00:00:00 2001 From: mpoke Date: Sat, 22 Apr 2023 13:44:32 +0200 Subject: [PATCH 2/7] provider: replace panic with error message --- x/ccv/consumer/keeper/relay.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 072adf2438..9bb2e75d65 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).Debug("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 } } From 5fc52ae01f7bd589401978a48852997cc7aa7206 Mon Sep 17 00:00:00 2001 From: mpoke Date: Tue, 25 Apr 2023 10:25:54 +0200 Subject: [PATCH 3/7] Info logging on client expiration --- x/ccv/consumer/keeper/relay.go | 2 +- x/ccv/provider/keeper/relay.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 9bb2e75d65..bb284c3dd6 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -198,7 +198,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) { if clienttypes.ErrClientNotActive.Is(err) { // IBC client is expired! // leave the packet data stored to be sent once the client is upgraded - k.Logger(ctx).Debug("IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String()) + k.Logger(ctx).Info("IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String()) return } // Not able to send packet over IBC! diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index fe60ecc595..7722ac56a0 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -194,7 +194,7 @@ 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 } // Not able to send packet over IBC! From 8c644810ae477658fdedfcffe2f68feeb2631bf1 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 15 May 2023 13:53:13 -0700 Subject: [PATCH 4/7] add test for consumer --- x/ccv/consumer/keeper/relay_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index c23a553225..4f661ce6bb 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -280,3 +280,25 @@ 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 + 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)) +} From 71eccd7d841be611ece00489e8e6cea3d9bb4753 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 17 May 2023 10:45:46 -0700 Subject: [PATCH 5/7] add test for provider --- x/ccv/consumer/keeper/relay_test.go | 6 +++-- x/ccv/provider/keeper/relay_test.go | 39 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index 4f661ce6bb..fc09a6b250 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -295,8 +295,10 @@ func TestSendPacketsFailure(t *testing.T) { }}) // Mock the channel keeper to return an error - mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID, - "consumerCCVChannelID").Return(channeltypes.Channel{}, false).Times(1) + 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) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 0ad7d21548..fda8ab8eda 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -13,8 +13,10 @@ import ( ibcsimapp "github.com/cosmos/interchain-security/legacy_ibc_testing/simapp" cryptotestutil "github.com/cosmos/interchain-security/testutil/crypto" testkeeper "github.com/cosmos/interchain-security/testutil/keeper" + testutil "github.com/cosmos/interchain-security/testutil/keeper" "github.com/cosmos/interchain-security/x/ccv/provider/keeper" providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types" + "github.com/cosmos/interchain-security/x/ccv/types" ccv "github.com/cosmos/interchain-security/x/ccv/types" "github.com/golang/mock/gomock" abci "github.com/tendermint/tendermint/abci/types" @@ -676,3 +678,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, types.ProviderPortID, + "CCVChannelID").Return(channeltypes.Channel{}, false).Times(1), + ) + + // Append mocks for expected call to StopConsumerChain + mockCalls = append(mockCalls, testutil.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")) +} From daf2fa0399a95228634f9f3ae5f05384838c7537 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 17 May 2023 12:33:03 -0700 Subject: [PATCH 6/7] linter --- x/ccv/provider/keeper/relay_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index fda8ab8eda..250d241f5e 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -13,10 +13,8 @@ import ( ibcsimapp "github.com/cosmos/interchain-security/legacy_ibc_testing/simapp" cryptotestutil "github.com/cosmos/interchain-security/testutil/crypto" testkeeper "github.com/cosmos/interchain-security/testutil/keeper" - testutil "github.com/cosmos/interchain-security/testutil/keeper" "github.com/cosmos/interchain-security/x/ccv/provider/keeper" providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types" - "github.com/cosmos/interchain-security/x/ccv/types" ccv "github.com/cosmos/interchain-security/x/ccv/types" "github.com/golang/mock/gomock" abci "github.com/tendermint/tendermint/abci/types" @@ -694,12 +692,12 @@ func TestSendVSCPacketsToChainFailure(t *testing.T) { // append mocks for the channel keeper to return an error mockCalls = append(mockCalls, - mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ProviderPortID, + mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, ccv.ProviderPortID, "CCVChannelID").Return(channeltypes.Channel{}, false).Times(1), ) // Append mocks for expected call to StopConsumerChain - mockCalls = append(mockCalls, testutil.GetMocksForStopConsumerChain(ctx, &mocks)...) + mockCalls = append(mockCalls, testkeeper.GetMocksForStopConsumerChain(ctx, &mocks)...) // Assert mock calls hit gomock.InOrder(mockCalls...) From 53b8f4af5a78901c8fd3a5bb344280c926476bf9 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Wed, 17 May 2023 12:33:46 -0700 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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)