Merge branch 'shawn/fix-send-packets' into shawn/throttle-with-retrie…

…s-consumer-changes

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

x/ccv/consumer/keeper/relay.go

@@ -186,7 +186,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) {
 	}
 
 	pending := k.GetPendingPackets(ctx)
-	toDelete := []uint64{}
+	idxsForDeletion := []uint64{}
 	for _, p := range pending {
 		if !k.PacketSendingPermitted(ctx) {
 			return
@@ -207,14 +207,14 @@ 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
 		}
 		// If the packet that was just sent was a Slash packet, set the waiting on slash reply flag.
 		// This flag will be toggled false again when consumer hears back from provider. See OnAcknowledgementPacket below.
@@ -224,12 +224,11 @@ func (k Keeper) SendPackets(ctx sdk.Context) {
 			break
 		} else {
 			// Otherwise the vsc matured will be deleted
-			toDelete = append(toDelete, p.Idx)
+			idxsForDeletion = append(idxsForDeletion, p.Idx)
 		}
 	}
-	for _, idx := range toDelete {
-		k.DeletePendingDataPackets(ctx, idx)
-	}
+	// 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

@@ -450,3 +450,39 @@ func setupSlashBeforeVscMatured(ctx sdk.Context, k *consumerkeeper.Keeper) {
 		},
 	})
 }
+
+// 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)
+}