From d77fbf4f49c99b102b072fdf95d35e00ebbf0825 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 17 Jul 2023 09:18:24 -0700 Subject: [PATCH] fix!: proper deletion of pending packets (#1146) * Update relay.go * expectation func * reg test * Update CHANGELOG.md --- CHANGELOG.md | 1 + testutil/keeper/expectations.go | 19 +++++++++++++++ x/ccv/consumer/keeper/relay.go | 10 ++++---- x/ccv/consumer/keeper/relay_test.go | 36 +++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c017fc5b1..7109f43bbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Add an entry to the unreleased section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a release. +* (fix!) proper deletion of pending packets [#1146](https://github.com/cosmos/interchain-security/pull/1146) * (feat!) optimize pending packets storage on consumer, with migration! [#1037](https://github.com/cosmos/interchain-security/pull/1037) ## v3.1.0 diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 1e59085d23..8a231f4760 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -14,6 +14,7 @@ import ( channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" providertypes "github.com/cosmos/interchain-security/v3/x/ccv/provider/types" + "github.com/cosmos/interchain-security/v3/x/ccv/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" ccv "github.com/cosmos/interchain-security/v3/x/ccv/types" @@ -139,3 +140,21 @@ func ExpectGetCapabilityMock(ctx sdk.Context, mocks MockedKeepers, times int) *g ctx, host.PortPath(ccv.ConsumerPortID), ).Return(nil, true).Times(times) } + +func GetMocksForSendIBCPacket(ctx sdk.Context, mocks MockedKeepers, channelID string, times int) []*gomock.Call { + return []*gomock.Call{ + mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID, + "consumerCCVChannelID").Return(channeltypes.Channel{}, true).Times(times), + mocks.MockScopedKeeper.EXPECT().GetCapability(ctx, + host.ChannelCapabilityPath(types.ConsumerPortID, "consumerCCVChannelID")).Return( + capabilitytypes.NewCapability(1), true).Times(times), + mocks.MockChannelKeeper.EXPECT().SendPacket(ctx, + capabilitytypes.NewCapability(1), + types.ConsumerPortID, + "consumerCCVChannelID", + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(uint64(888), nil).Times(times), + } +} diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 073a1d3996..120c0218c2 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -186,6 +186,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) { } pending := k.GetPendingPackets(ctx) + idxsForDeletion := []uint64{} for _, p := range pending { // send packet over IBC @@ -203,18 +204,19 @@ func (k Keeper) SendPackets(ctx sdk.Context) { // 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 + break } // 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 + break } + idxsForDeletion = append(idxsForDeletion, p.Idx) } - - k.DeleteAllPendingDataPackets(ctx) + // Delete pending packets that were successfully sent and did not return an error from SendIBCPacket + k.DeletePendingDataPackets(ctx, idxsForDeletion...) } // OnAcknowledgementPacket executes application logic for acknowledgments of sent VSCMatured and Slash packets diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index 48ad7f5ab9..6dbebe273f 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -307,3 +307,39 @@ func TestSendPacketsFailure(t *testing.T) { consumerKeeper.SendPackets(ctx) require.Equal(t, 3, len(consumerKeeper.GetPendingPackets(ctx))) } + +// Regression test for https://github.com/cosmos/interchain-security/issues/1145 +func TestSendPacketsDeletion(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()) + + // Queue two pending packets + consumerKeeper.AppendPendingPacket(ctx, types.SlashPacket, &types.ConsumerPacketData_SlashPacketData{ // Slash appears first + SlashPacketData: &types.SlashPacketData{ + Validator: abci.Validator{}, + ValsetUpdateId: 88, + Infraction: stakingtypes.Infraction_INFRACTION_DOWNTIME, + }, + }) + consumerKeeper.AppendPendingPacket(ctx, types.VscMaturedPacket, &types.ConsumerPacketData_VscMaturedPacketData{ + VscMaturedPacketData: &types.VSCMaturedPacketData{ + ValsetUpdateId: 90, + }, + }) + + // Get mocks for a successful SendPacket call that does NOT return an error + expectations := testkeeper.GetMocksForSendIBCPacket(ctx, mocks, "consumerCCVChannelID", 1) + // Append mocks for a failed SendPacket call, which returns an error + expectations = append(expectations, mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID, + "consumerCCVChannelID").Return(channeltypes.Channel{}, false).Times(1)) + gomock.InOrder(expectations...) + + consumerKeeper.SendPackets(ctx) + + // Expect the first successfully sent packet to be popped from queue + require.Equal(t, 1, len(consumerKeeper.GetPendingPackets(ctx))) + require.Equal(t, types.VscMaturedPacket, consumerKeeper.GetPendingPackets(ctx)[0].Type) +}