fix!: proper deletion of pending packets (#1146)

* Update relay.go

* expectation func

* reg test

* Update CHANGELOG.md

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

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),
+	}
+}

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

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)
+}