fix!: revert recent consumer packet data changes (#1150)

* revert changes

* lint

* Update CHANGELOG.md

* wrapper type

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!) revert consumer packet data changes from #1037 [#1150](https://github.com/cosmos/interchain-security/pull/1150)
 * (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)
 

proto/interchain_security/ccv/v1/ccv.proto

@@ -64,7 +64,6 @@ message ConsumerPacketData {
     SlashPacketData slashPacketData = 2;
     VSCMaturedPacketData vscMaturedPacketData = 3;
   }
-  uint64 idx = 4;
 }
 
 

x/ccv/consumer/keeper/genesis_test.go

@@ -262,16 +262,12 @@ func TestExportGenesis(t *testing.T) {
 				Data: &ccv.ConsumerPacketData_SlashPacketData{
 					SlashPacketData: ccv.NewSlashPacketData(abciValidator, vscID, stakingtypes.Infraction_INFRACTION_DOWNTIME),
 				},
-				Idx: 0,
 			},
 			{
 				Type: ccv.VscMaturedPacket,
 				Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
 					VscMaturedPacketData: ccv.NewVSCMaturedPacketData(vscID),
 				},
-				// This idx is a part of the expected genesis state.
-				// If the keeper is correctly storing consumer packet data, indexes should be populated.
-				Idx: 1,
 			},
 		},
 	}

x/ccv/consumer/keeper/keeper.go

@@ -606,9 +606,29 @@ func (k Keeper) getAndIncrementPendingPacketsIdx(ctx sdk.Context) (toReturn uint
 	return toReturn
 }
 
-// GetPendingPackets returns ALL the pending CCV packets from the store
+// GetPendingPackets returns ALL the pending CCV packets from the store without indexes.
 func (k Keeper) GetPendingPackets(ctx sdk.Context) []ccv.ConsumerPacketData {
-	var packets []ccv.ConsumerPacketData
+	ppWithIndexes := k.GetAllPendingPacketsWithIdx(ctx)
+	var ppList []ccv.ConsumerPacketData
+	for _, ppWithIndex := range ppWithIndexes {
+		ppList = append(ppList, ppWithIndex.ConsumerPacketData)
+	}
+	return ppList
+}
+
+// ConsumerPacketDataWithIdx is a wrapper around ConsumerPacketData
+// that also stores the index of the packet in the pending packets queue.
+//
+// Note this type is a shim to avoid changing the schema of ConsumerPacketData and breaking the wire.
+type ConsumerPacketDataWithIdx struct {
+	ccv.ConsumerPacketData // Struct embedding
+	Idx                    uint64
+}
+
+// GetAllPendingPacketsWithIdx returns ALL pending consumer packet data from the store
+// with indexes relevant to the pending packets queue.
+func (k Keeper) GetAllPendingPacketsWithIdx(ctx sdk.Context) []ConsumerPacketDataWithIdx {
+	packets := []ConsumerPacketDataWithIdx{}
 	store := ctx.KVStore(k.storeKey)
 	// Note: PendingDataPacketsBytePrefix is the correct prefix, NOT PendingDataPacketsByteKey.
 	// See consistency with PendingDataPacketsKey().
@@ -622,7 +642,12 @@ func (k Keeper) GetPendingPackets(ctx sdk.Context) []ccv.ConsumerPacketData {
 			// An error here would indicate something is very wrong,
 			panic(fmt.Errorf("failed to unmarshal pending data packet: %w", err))
 		}
-		packets = append(packets, packet)
+		packetWithIdx := ConsumerPacketDataWithIdx{
+			ConsumerPacketData: packet,
+			// index stored in key after prefix, see PendingDataPacketsKey()
+			Idx: sdk.BigEndianToUint64(iterator.Key()[1:]),
+		}
+		packets = append(packets, packetWithIdx)
 	}
 	return packets
 }
@@ -652,13 +677,10 @@ func (k Keeper) DeleteAllPendingDataPackets(ctx sdk.Context) {
 
 // AppendPendingPacket enqueues the given data packet to the end of the pending data packets queue
 func (k Keeper) AppendPendingPacket(ctx sdk.Context, packetType ccv.ConsumerPacketDataType, data ccv.ExportedIsConsumerPacketData_Data) {
-	cpd := ccv.NewConsumerPacketData(
-		packetType,
-		data,
-		k.getAndIncrementPendingPacketsIdx(ctx),
-	)
-	key := types.PendingDataPacketsKey(cpd.Idx)
+	idx := k.getAndIncrementPendingPacketsIdx(ctx) // for FIFO queue
+	key := types.PendingDataPacketsKey(idx)
 	store := ctx.KVStore(k.storeKey)
+	cpd := ccv.NewConsumerPacketData(packetType, data)
 	bz, err := cpd.Marshal()
 	if err != nil {
 		// This should never happen

x/ccv/consumer/keeper/keeper_test.go

@@ -325,14 +325,12 @@ func TestPendingPackets(t *testing.T) {
 			Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
 				VscMaturedPacketData: ccv.NewVSCMaturedPacketData(1),
 			},
-			Idx: 0, // Note these are expected idxs, we don't pass this data to the keeper
 		},
 		{
 			Type: ccv.VscMaturedPacket,
 			Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
 				VscMaturedPacketData: ccv.NewVSCMaturedPacketData(2),
 			},
-			Idx: 1,
 		},
 		{
 			Type: ccv.SlashPacket,
@@ -343,14 +341,12 @@ func TestPendingPackets(t *testing.T) {
 					stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN,
 				),
 			},
-			Idx: 2,
 		},
 		{
 			Type: ccv.VscMaturedPacket,
 			Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
 				VscMaturedPacketData: ccv.NewVSCMaturedPacketData(3),
 			},
-			Idx: 3,
 		},
 	}
 
@@ -376,7 +372,6 @@ func TestPendingPackets(t *testing.T) {
 		Data: &ccv.ConsumerPacketData_SlashPacketData{
 			SlashPacketData: slashPacket,
 		},
-		Idx: 4,
 	})
 
 	toAppend := packetData[len(packetData)-1]
@@ -391,7 +386,6 @@ func TestPendingPackets(t *testing.T) {
 		Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
 			VscMaturedPacketData: vscMaturedPacket,
 		},
-		Idx: 5,
 	})
 	toAppend = packetData[len(packetData)-1]
 	consumerKeeper.AppendPendingPacket(ctx, toAppend.Type, toAppend.Data)
@@ -404,16 +398,21 @@ func TestPendingPackets(t *testing.T) {
 	consumerKeeper.DeletePendingDataPackets(ctx, 5)
 	storedPacketData = consumerKeeper.GetPendingPackets(ctx)
 	require.Equal(t, packetData[:len(packetData)-1], storedPacketData)
+	pendingPacketsWithIdx := consumerKeeper.GetAllPendingPacketsWithIdx(ctx)
+	require.Equal(t, uint64(4), pendingPacketsWithIdx[len(pendingPacketsWithIdx)-1].Idx) // final element should have idx 4
 
 	// Delete packet with idx 0 (first index)
 	consumerKeeper.DeletePendingDataPackets(ctx, 0)
 	storedPacketData = consumerKeeper.GetPendingPackets(ctx)
 	require.Equal(t, packetData[1:len(packetData)-1], storedPacketData)
+	pendingPacketsWithIdx = consumerKeeper.GetAllPendingPacketsWithIdx(ctx)
+	require.Equal(t, uint64(1), pendingPacketsWithIdx[0].Idx) // first element should have idx 1
 
 	// Delete all packets
 	consumerKeeper.DeleteAllPendingDataPackets(ctx)
 	storedPacketData = consumerKeeper.GetPendingPackets(ctx)
 	require.Empty(t, storedPacketData)
+	require.Empty(t, consumerKeeper.GetAllPendingPacketsWithIdx(ctx))
 }
 
 // TestVerifyProviderChain tests the VerifyProviderChain method for the consumer keeper

x/ccv/consumer/keeper/migration_test.go

@@ -60,9 +60,4 @@ func TestMigrateConsumerPacketData(t *testing.T) {
 	require.Equal(t, uint64(77), obtainedPackets[0].GetSlashPacketData().ValsetUpdateId)
 	require.Equal(t, uint64(88), obtainedPackets[1].GetVscMaturedPacketData().ValsetUpdateId)
 	require.Equal(t, uint64(99), obtainedPackets[2].GetVscMaturedPacketData().ValsetUpdateId)
-
-	// Check that indexes are populated
-	require.Equal(t, uint64(0), obtainedPackets[0].Idx)
-	require.Equal(t, uint64(1), obtainedPackets[1].Idx)
-	require.Equal(t, uint64(2), obtainedPackets[2].Idx)
 }

x/ccv/consumer/keeper/relay.go

@@ -185,7 +185,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) {
 		return
 	}
 
-	pending := k.GetPendingPackets(ctx)
+	pending := k.GetAllPendingPacketsWithIdx(ctx)
 	idxsForDeletion := []uint64{}
 	for _, p := range pending {
 

x/ccv/types/ccv.go

@@ -171,10 +171,9 @@ type ExportedIsConsumerPacketData_Data interface {
 	isConsumerPacketData_Data
 }
 
-func NewConsumerPacketData(cpdType ConsumerPacketDataType, data isConsumerPacketData_Data, idx uint64) ConsumerPacketData {
+func NewConsumerPacketData(cpdType ConsumerPacketDataType, data isConsumerPacketData_Data) ConsumerPacketData {
 	return ConsumerPacketData{
 		Type: cpdType,
 		Data: data,
-		Idx:  idx,
 	}
 }