Correctly handle VSC packet with duplicate val updates on consumer (#846

)

* fix dup val update consumer

* gofumpt

* add small refactor to OnRecvVSCPacket

---------

Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com>

x/ccv/consumer/keeper/relay.go

@@ -48,13 +48,12 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
 		)
 	}
 	// Set pending changes by accumulating changes from this packet with all prior changes
-	var pendingChanges []abci.ValidatorUpdate
+	currentValUpdates := []abci.ValidatorUpdate{}
 	currentChanges, exists := k.GetPendingChanges(ctx)
-	if !exists {
-		pendingChanges = newChanges.ValidatorUpdates
-	} else {
-		pendingChanges = utils.AccumulateChanges(currentChanges.ValidatorUpdates, newChanges.ValidatorUpdates)
+	if exists {
+		currentValUpdates = currentChanges.ValidatorUpdates
 	}
+	pendingChanges := utils.AccumulateChanges(currentValUpdates, newChanges.ValidatorUpdates)
 
 	k.SetPendingChanges(ctx, ccv.ValidatorSetChangePacketData{
 		ValidatorUpdates: pendingChanges,

x/ccv/consumer/keeper/relay_test.go

@@ -13,6 +13,7 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v4/modules/core/24-host"
+	"github.com/cosmos/interchain-security/testutil/crypto"
 	testkeeper "github.com/cosmos/interchain-security/testutil/keeper"
 	consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types"
 	"github.com/cosmos/interchain-security/x/ccv/types"
@@ -152,6 +153,60 @@ func TestOnRecvVSCPacket(t *testing.T) {
 	}
 }
 
+// TestOnRecvVSCPacketDuplicateUpdates tests that the consumer can correctly handle a single VSC packet
+// with duplicate valUpdates for the same pub key.
+//
+// Note: This scenario shouldn't usually happen, ie. the provider shouldn't send duplicate val updates
+// for the same pub key. But it's useful to guard against.
+func TestOnRecvVSCPacketDuplicateUpdates(t *testing.T) {
+	// Arbitrary channel IDs
+	consumerCCVChannelID := "consumerCCVChannelID"
+	providerCCVChannelID := "providerCCVChannelID"
+
+	// Keeper setup
+	consumerKeeper, ctx, ctrl, _ := testkeeper.GetConsumerKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	consumerKeeper.SetProviderChannel(ctx, consumerCCVChannelID)
+	consumerKeeper.SetParams(ctx, consumertypes.DefaultParams())
+
+	// Construct packet/data with duplicate val updates for the same pub key
+	cId := crypto.NewCryptoIdentityFromIntSeed(43278947)
+	valUpdates := []abci.ValidatorUpdate{
+		{
+			PubKey: cId.TMProtoCryptoPublicKey(),
+			Power:  0,
+		},
+		{
+			PubKey: cId.TMProtoCryptoPublicKey(),
+			Power:  473289,
+		},
+	}
+	vscData := types.NewValidatorSetChangePacketData(
+		valUpdates,
+		1,
+		nil,
+	)
+	packet := channeltypes.NewPacket(vscData.GetBytes(), 2, types.ProviderPortID,
+		providerCCVChannelID, types.ConsumerPortID, consumerCCVChannelID, clienttypes.NewHeight(1, 0), 0)
+
+	// Confirm no pending changes exist before OnRecvVSCPacket
+	_, ok := consumerKeeper.GetPendingChanges(ctx)
+	require.False(t, ok)
+
+	// Execute OnRecvVSCPacket
+	ack := consumerKeeper.OnRecvVSCPacket(ctx, packet, vscData)
+	require.NotNil(t, ack)
+	require.True(t, ack.Success())
+
+	// Confirm pending changes are queued by OnRecvVSCPacket
+	gotPendingChanges, ok := consumerKeeper.GetPendingChanges(ctx)
+	require.True(t, ok)
+
+	// Confirm that only the latest update is kept, duplicate update is discarded
+	require.Equal(t, 1, len(gotPendingChanges.ValidatorUpdates))
+	require.Equal(t, valUpdates[1], gotPendingChanges.ValidatorUpdates[0]) // Only latest update should be kept
+}
+
 // TestOnAcknowledgementPacket tests application logic for acknowledgments of sent VSCMatured and Slash packets
 // in conjunction with the ibc module's execution of "acknowledgePacket",
 // according to https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#processing-acknowledgements