Fix: Iteration through PacketMaturityTimes assumes maturity time order (

#622) * WIP convert iterators to array getters. Still need to rename functions, and some compile errors in tests. * WIP - compiles, fixing tests * Unit and e2e tests work * add notes about stopping iteration * WIP - rename and add some notes * Add types to proto * delete unused code * resolve naming conflict * implement another type as proto * fixing more stuff * delete TODOJEHAN.md * adds many of Marius's iteration order comments from 599, and does some small refactors for clarity * fix nil pointer deref * call GetAllConsumerChains once * expand TestGetAllChannelToChains * expand TestGetAllUnbondingOps * GetAllUnbondingOpIndexes; cleanup proto files * fix GetAllValsetUpdateBlockHeights and UTs * remove GetAllSlashAck * add tests for GetFirstVscSendTimestamp * key assignment iterators * reviewed proposals * add TestGetAllValsetUpdateBlockHeights * add TestGetAllOutstandingDowntimes * add GetElapsedPacketMaturityTimes * fix linter * fix linter * prevent implicit memory aliasing * add UTC to TestPacketMaturityTime * fix TestPacketMaturityTime * avoid local variable name shadowing * Update x/ccv/consumer/keeper/keeper.go Co-authored-by: Shawn <[email protected]> * replace cases with packets in TestPacketMaturityTime * add expected order to TestPacketMaturityTime * add expected order to TestGetAllHeightToValsetUpdateIDs * add expected order to TestGetAllOutstandingDowntimes * add TestGetAllCCValidator * add expected order to TestGetAllConsumerChains * add expected order to TestGetAllChannelToChains * add expected order to TestGetAllUnbondingOps * add expected order to TestGetAllUnbondingOpIndexes * add expected order to TestGetAllValsetUpdateBlockHeights * add expected order to TestInitTimeoutTimestamp * add expected order to TestVscSendTimestamp * add expected order to TestGetAllValidatorConsumerPubKey * add expected order to TestGetAllValidatorsByConsumerAddr * add expected order to TestGetAllKeyAssignmentReplacements * add expected order to TestGetAllConsumerAddrsToPrune * iterate over packet maturities in order of time * fix linter * move AppendMany to utils * review suggestions * refactor TestPacketMaturityTime UT * nits Co-authored-by: Jehan Tremback <[email protected]> Co-authored-by: Shawn <[email protected]>
cosmos · Dec 23, 2022 · 0c62722 · 0c62722
1 parent b96b103
commit 0c62722
Show file tree

Hide file tree

Showing 15 changed files with 456 additions and 417 deletions.
diff --git a/proto/interchain_security/ccv/consumer/v1/consumer.proto b/proto/interchain_security/ccv/consumer/v1/consumer.proto
@@ -9,6 +9,7 @@ import "google/protobuf/any.proto";
 import "gogoproto/gogo.proto";
 import "cosmos_proto/cosmos.proto";
 import "google/protobuf/duration.proto";
+import "google/protobuf/timestamp.proto";
 
 // Params defines the parameters for CCV consumer module
 message Params {
@@ -68,3 +69,8 @@ message CrossChainValidator {
   ];
 }
 
+// MaturingVSCPacket contains the maturing time of a received VSCPacket
+message MaturingVSCPacket {
+  uint64 vscId = 1;
+  google.protobuf.Timestamp maturity_time = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
+}
diff --git a/proto/interchain_security/ccv/consumer/v1/genesis.proto b/proto/interchain_security/ccv/consumer/v1/genesis.proto
@@ -23,7 +23,7 @@ message GenesisState {
   // ProviderConsensusState filled in on new chain, nil on restart.
   ibc.lightclients.tendermint.v1.ConsensusState provider_consensus_state = 6;
   // MaturingPackets nil on new chain, filled in on restart.
-  repeated MaturingVSCPacket maturing_packets = 7
+  repeated interchain_security.ccv.consumer.v1.MaturingVSCPacket maturing_packets = 7
       [ (gogoproto.nullable) = false ];
   // InitialValset filled in on new chain and on restart.
   repeated .tendermint.abci.ValidatorUpdate initial_val_set = 8
@@ -42,13 +42,6 @@ message GenesisState {
   [ (gogoproto.nullable) = false ];
 }
 
-// MaturingVSCPacket defines the genesis information for the
-// unbonding VSC packet
-message MaturingVSCPacket {
-  uint64 vscId = 1;
-  uint64 maturity_time = 2;
-}
-
 // HeightValsetUpdateID defines the genesis information for the mapping 
 // of each block height to a valset update id
 message HeightToValsetUpdateID {

diff --git a/tests/e2e/valset_update.go b/tests/e2e/valset_update.go
@@ -99,21 +99,40 @@ func (suite *CCVTestSuite) TestQueueAndSendVSCMaturedPackets() {
 	suite.Require().NotNil(ack, "OnRecvVSCPacket did not return ack")
 	suite.Require().True(ack.Success(), "OnRecvVSCPacket did not return a Success Acknowledgment")
 
+	packetMaturities := consumerKeeper.GetAllPacketMaturityTimes(suite.consumerChain.GetContext())
+
 	// increase time such that first two packets are unbonded but third is not.
 	unbondingPeriod := consumerKeeper.GetUnbondingPeriod(suite.consumerChain.GetContext())
 	// increase time
 	incrementTime(suite, unbondingPeriod-time.Hour)
 
 	// ensure first two packets are unbonded and VSCMatured packets are queued
 	// unbonded time is deleted
-	time1 := consumerKeeper.GetPacketMaturityTime(suite.consumerChain.GetContext(), 1)
-	time2 := consumerKeeper.GetPacketMaturityTime(suite.consumerChain.GetContext(), 2)
-	suite.Require().Equal(uint64(0), time1, "maturity time not deleted for mature packet 1")
-	suite.Require().Equal(uint64(0), time2, "maturity time not deleted for mature packet 2")
-
+	suite.Require().False(
+		consumerKeeper.PacketMaturityTimeExists(
+			suite.consumerChain.GetContext(),
+			packetMaturities[0].VscId,
+			packetMaturities[0].MaturityTime,
+		),
+		"maturity time not deleted for mature packet 1",
+	)
+	suite.Require().False(
+		consumerKeeper.PacketMaturityTimeExists(
+			suite.consumerChain.GetContext(),
+			packetMaturities[1].VscId,
+			packetMaturities[1].MaturityTime,
+		),
+		"maturity time not deleted for mature packet 2",
+	)
 	// ensure that third packet did not get unbonded and is still in store
-	time3 := consumerKeeper.GetPacketMaturityTime(suite.consumerChain.GetContext(), 3)
-	suite.Require().True(time3 > uint64(suite.consumerChain.GetContext().BlockTime().UnixNano()), "maturity time for packet 3 is not after current time")
+	suite.Require().True(
+		consumerKeeper.PacketMaturityTimeExists(
+			suite.consumerChain.GetContext(),
+			packetMaturities[2].VscId,
+			packetMaturities[2].MaturityTime,
+		),
+		"maturity time for packet 3 is not after current time",
+	)
 
 	// check that the packets are committed in state
 	commitments := suite.consumerApp.GetIBCKeeper().ChannelKeeper.GetAllPacketCommitmentsAtChannel(

diff --git a/x/ccv/consumer/keeper/genesis_test.go b/x/ccv/consumer/keeper/genesis_test.go
@@ -64,7 +64,7 @@ func TestInitGenesis(t *testing.T) {
 	matPackets := []consumertypes.MaturingVSCPacket{
 		{
 			VscId:        1,
-			MaturityTime: uint64(time.Now().UnixNano()),
+			MaturityTime: time.Now().UTC(),
 		},
 	}
 	pendingDataPackets := ccv.ConsumerPacketDataList{
@@ -185,7 +185,7 @@ func TestInitGenesis(t *testing.T) {
 				require.True(t, ok)
 				require.Equal(t, provChannelID, gotChannelID)
 
-				require.Equal(t, vscID, ck.GetPacketMaturityTime(ctx, vscID))
+				require.True(t, ck.PacketMaturityTimeExists(ctx, matPackets[0].VscId, matPackets[0].MaturityTime))
 				require.Equal(t, pendingDataPackets, ck.GetPendingPackets(ctx))
 
 				require.Equal(t, gs.OutstandingDowntimeSlashing, ck.GetAllOutstandingDowntimes(ctx))
@@ -236,7 +236,7 @@ func TestExportGenesis(t *testing.T) {
 	matPackets := []consumertypes.MaturingVSCPacket{
 		{
 			VscId:        1,
-			MaturityTime: uint64(time.Now().UnixNano()),
+			MaturityTime: time.Now().UTC(),
 		},
 	}
 

diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go
@@ -3,6 +3,7 @@ package keeper
 import (
 	"encoding/binary"
 	"fmt"
+	"time"
 
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -206,76 +207,81 @@ func (k Keeper) DeletePendingChanges(ctx sdk.Context) {
 	store.Delete(types.PendingChangesKey())
 }
 
-// GetElapsedPacketMaturityTimes returns a slice of already elapsed PacketMaturityTimes, sorted by vscIDs,
-// i.e., the slice contains the IDs of the matured VSCPackets
-func (k Keeper) GetElapsedPacketMaturityTimes(ctx sdk.Context) (maturingVSCPacket []consumertypes.MaturingVSCPacket) {
-	currentTime := uint64(ctx.BlockTime().UnixNano())
+// GetElapsedPacketMaturityTimes returns a slice of already elapsed PacketMaturityTimes, sorted by maturity times,
+// i.e., the slice contains the IDs of the matured VSCPackets.
+func (k Keeper) GetElapsedPacketMaturityTimes(ctx sdk.Context) (maturingVSCPackets []consumertypes.MaturingVSCPacket) {
 	store := ctx.KVStore(k.storeKey)
 	iterator := sdk.KVStorePrefixIterator(store, []byte{types.PacketMaturityTimeBytePrefix})
 
 	defer iterator.Close()
 
 	for ; iterator.Valid(); iterator.Next() {
-		vscId := binary.BigEndian.Uint64(iterator.Key()[1:])
-		maturityTime := binary.BigEndian.Uint64(iterator.Value())
+		var maturingVSCPacket consumertypes.MaturingVSCPacket
+		if err := maturingVSCPacket.Unmarshal(iterator.Value()); err != nil {
+			panic(fmt.Errorf("failed to unmarshal MaturingVSCPacket: %w", err))
+		}
 
-		// If the maturity time is after the current time, then stop the iteration;
-		// TODO: the iteration over PacketMaturityTimes should be over maturity times,
-		// see https://github.com/cosmos/interchain-security/issues/598
-		if currentTime < maturityTime {
+		// If the current block time is before maturity time then stop the iteration.
+		// This is possible since the iteration over PacketMaturityTimes is in order
+		// of maturity times
+		if ctx.BlockTime().Before(maturingVSCPacket.MaturityTime) {
 			break
 		}
 
-		maturingVSCPacket = append(maturingVSCPacket, consumertypes.MaturingVSCPacket{
-			VscId:        vscId,
-			MaturityTime: maturityTime,
-		})
+		maturingVSCPackets = append(maturingVSCPackets, maturingVSCPacket)
 	}
-	return maturingVSCPacket
+	return maturingVSCPackets
 }
 
-// GetAllPacketMaturityTimes returns a slice of all PacketMaturityTimes, sorted by vscIDs.
-func (k Keeper) GetAllPacketMaturityTimes(ctx sdk.Context) (maturingVSCPacket []consumertypes.MaturingVSCPacket) {
+// GetAllPacketMaturityTimes returns a slice of all PacketMaturityTimes, sorted by maturity times.
+//
+// Note that PacketMaturityTimes are stored under keys with the following format:
+// PacketMaturityTimeBytePrefix | maturityTime.UnixNano() | vscID
+// Thus, the returned array is in ascending order of maturityTimes.
+// If two entries have the same maturityTime, then they are ordered by vscID.
+func (k Keeper) GetAllPacketMaturityTimes(ctx sdk.Context) (maturingVSCPackets []consumertypes.MaturingVSCPacket) {
 	store := ctx.KVStore(k.storeKey)
 	iterator := sdk.KVStorePrefixIterator(store, []byte{types.PacketMaturityTimeBytePrefix})
 
 	defer iterator.Close()
 	for ; iterator.Valid(); iterator.Next() {
-		vscId := binary.BigEndian.Uint64(iterator.Key()[1:])
-		maturityTime := binary.BigEndian.Uint64(iterator.Value())
+		var maturingVSCPacket consumertypes.MaturingVSCPacket
+		if err := maturingVSCPacket.Unmarshal(iterator.Value()); err != nil {
+			panic(fmt.Errorf("failed to unmarshal MaturingVSCPacket: %w", err))
+		}
 
-		maturingVSCPacket = append(maturingVSCPacket, consumertypes.MaturingVSCPacket{
-			VscId:        vscId,
-			MaturityTime: maturityTime,
-		})
+		maturingVSCPackets = append(maturingVSCPackets, maturingVSCPacket)
 	}
-	return maturingVSCPacket
+	return maturingVSCPackets
 }
 
 // SetPacketMaturityTime sets the maturity time for a given received VSC packet id
-func (k Keeper) SetPacketMaturityTime(ctx sdk.Context, vscId, maturityTime uint64) {
+func (k Keeper) SetPacketMaturityTime(ctx sdk.Context, vscId uint64, maturityTime time.Time) {
 	store := ctx.KVStore(k.storeKey)
-	timeBytes := make([]byte, 8)
-	binary.BigEndian.PutUint64(timeBytes, maturityTime)
-	store.Set(types.PacketMaturityTimeKey(vscId), timeBytes)
+	maturingVSCPacket := consumertypes.MaturingVSCPacket{
+		VscId:        vscId,
+		MaturityTime: maturityTime,
+	}
+	bz, err := maturingVSCPacket.Marshal()
+	if err != nil {
+		panic(fmt.Errorf("failed to marshal MaturingVSCPacket: %w", err))
+	}
+	store.Set(types.PacketMaturityTimeKey(vscId, maturityTime), bz)
 }
 
-// GetPacketMaturityTime gets the maturity time for a given received VSC packet id
-func (k Keeper) GetPacketMaturityTime(ctx sdk.Context, vscId uint64) uint64 {
+// PacketMaturityExists checks whether the packet maturity time for a given vscId and maturityTime exists.
+//
+// Note: this method is only used in testing.
+func (k Keeper) PacketMaturityTimeExists(ctx sdk.Context, vscId uint64, maturityTime time.Time) bool {
 	store := ctx.KVStore(k.storeKey)
-	bz := store.Get(types.PacketMaturityTimeKey(vscId))
-	if bz == nil {
-		return 0
-	}
-	return binary.BigEndian.Uint64(bz)
+	bz := store.Get(types.PacketMaturityTimeKey(vscId, maturityTime))
+	return bz != nil
 }
 
-// DeletePacketMaturityTimes deletes the packet maturity time for given received VSC packet ids
-func (k Keeper) DeletePacketMaturityTimes(ctx sdk.Context, vscIds ...uint64) {
+// DeletePacketMaturityTimes deletes the packet maturity time for a given vscId and maturityTime
+func (k Keeper) DeletePacketMaturityTimes(ctx sdk.Context, vscId uint64, maturityTime time.Time) {
 	store := ctx.KVStore(k.storeKey)
-	for _, vscId := range vscIds {
-		store.Delete(types.PacketMaturityTimeKey(vscId))
-	}
+	store.Delete(types.PacketMaturityTimeKey(vscId, maturityTime))
 }
 
 // VerifyProviderChain verifies that the chain trying to connect on the channel handshake

diff --git a/x/ccv/consumer/keeper/keeper_test.go b/x/ccv/consumer/keeper/keeper_test.go
@@ -90,60 +90,51 @@ func TestPacketMaturityTime(t *testing.T) {
 	defer ctrl.Finish()
 
 	now := time.Now().UTC()
-	nsNow := uint64(now.UnixNano())
 	packets := []types.MaturingVSCPacket{
 		{
 			VscId:        2,
-			MaturityTime: nsNow,
+			MaturityTime: now,
 		},
 		{
 			VscId:        1,
-			MaturityTime: nsNow - 15,
+			MaturityTime: now.Add(-time.Hour),
 		},
 		{
 			VscId:        5,
-			MaturityTime: nsNow - 30,
+			MaturityTime: now.Add(-2 * time.Hour),
 		},
 		{
 			VscId:        6,
-			MaturityTime: nsNow + 10,
+			MaturityTime: now.Add(time.Hour),
 		},
 	}
-	expectedGetAllOrder := packets
-	// sorting by VscId
-	sort.Slice(expectedGetAllOrder, func(i, j int) bool {
-		return expectedGetAllOrder[i].VscId < expectedGetAllOrder[j].VscId
-	})
-	expectedGetElapsedOrder := []types.MaturingVSCPacket{}
-	for _, packet := range packets {
-		// only packets with MaturityTime <= nsNow
-		if packet.MaturityTime <= nsNow {
-			expectedGetElapsedOrder = append(expectedGetElapsedOrder, packet)
-		}
-	}
-	// sorting by VscId
-	sort.Slice(expectedGetElapsedOrder, func(i, j int) bool {
-		return expectedGetElapsedOrder[i].VscId < expectedGetElapsedOrder[j].VscId
-	})
+	// sort by MaturityTime and not by VscId
+	expectedGetAllOrder := []types.MaturingVSCPacket{packets[2], packets[1], packets[0], packets[3]}
+	// only packets with MaturityTime before or equal to now
+	expectedGetElapsedOrder := []types.MaturingVSCPacket{packets[2], packets[1], packets[0]}
 
+	// test SetPacketMaturityTime
 	for _, packet := range packets {
 		ck.SetPacketMaturityTime(ctx, packet.VscId, packet.MaturityTime)
 	}
 
+	// test PacketMaturityTimeExists
 	for _, packet := range packets {
-		require.Equal(t, packet.MaturityTime, ck.GetPacketMaturityTime(ctx, packet.VscId))
+		require.True(t, ck.PacketMaturityTimeExists(ctx, packet.VscId, packet.MaturityTime))
 	}
 
+	// test GetAllPacketMaturityTimes
 	maturingVSCPackets := ck.GetAllPacketMaturityTimes(ctx)
 	require.Len(t, maturingVSCPackets, len(packets))
 	require.Equal(t, expectedGetAllOrder, maturingVSCPackets)
 
+	// test GetElapsedPacketMaturityTimes
 	elapsedMaturingVSCPackets := ck.GetElapsedPacketMaturityTimes(ctx.WithBlockTime(now))
 	require.Equal(t, expectedGetElapsedOrder, elapsedMaturingVSCPackets)
 
-	ck.DeletePacketMaturityTimes(ctx, 6)
-	require.Equal(t, uint64(0), ck.GetPacketMaturityTime(ctx, 3))
-	require.Equal(t, uint64(0), ck.GetPacketMaturityTime(ctx, 6))
+	// test DeletePacketMaturityTimes
+	ck.DeletePacketMaturityTimes(ctx, packets[0].VscId, packets[0].MaturityTime)
+	require.False(t, ck.PacketMaturityTimeExists(ctx, packets[0].VscId, packets[0].MaturityTime))
 }
 
 // TestCrossChainValidator tests the getter, setter, and deletion method for cross chain validator records

diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go
@@ -64,7 +64,7 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
 
 	// Save maturity time and packet
 	maturityTime := ctx.BlockTime().Add(k.GetUnbondingPeriod(ctx))
-	k.SetPacketMaturityTime(ctx, newChanges.ValsetUpdateId, uint64(maturityTime.UnixNano()))
+	k.SetPacketMaturityTime(ctx, newChanges.ValsetUpdateId, maturityTime)
 
 	// set height to VSC id mapping
 	k.SetHeightValsetUpdateID(ctx, uint64(ctx.BlockHeight())+1, newChanges.ValsetUpdateId)
@@ -85,24 +85,22 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
 // operations that resulted in validator updates included in that VSC have matured on
 // the consumer chain.
 func (k Keeper) QueueVSCMaturedPackets(ctx sdk.Context) {
-	currentTime := uint64(ctx.BlockTime().UnixNano())
-
 	for _, maturityTime := range k.GetElapsedPacketMaturityTimes(ctx) {
-		if currentTime < maturityTime.MaturityTime {
-			panic(fmt.Errorf("maturity time %d is greater than current time %d", maturityTime.MaturityTime, currentTime))
+		if ctx.BlockTime().Before(maturityTime.MaturityTime) {
+			panic(fmt.Errorf("maturity time %s is after than current time %s", maturityTime.MaturityTime, ctx.BlockTime()))
 		}
 		// construct validator set change packet data
 		vscPacket := ccv.NewVSCMaturedPacketData(maturityTime.VscId)
 
-		// append VSCMatured packet to pending packets
-		// sending packets is attempted each EndBlock
-		// unsent packets remain in the queue until sent
+		// Append VSCMatured packet to pending packets.
+		// Sending packets is attempted each EndBlock.
+		// Unsent packets remain in the queue until sent.
 		k.AppendPendingPacket(ctx, ccv.ConsumerPacketData{
 			Type: ccv.VscMaturedPacket,
 			Data: &ccv.ConsumerPacketData_VscMaturedPacketData{VscMaturedPacketData: vscPacket},
 		})
 
-		k.DeletePacketMaturityTimes(ctx, uint64(maturityTime.VscId))
+		k.DeletePacketMaturityTimes(ctx, maturityTime.VscId, maturityTime.MaturityTime)
 
 		ctx.EventManager().EmitEvent(
 			sdk.NewEvent(
@@ -111,7 +109,7 @@ func (k Keeper) QueueVSCMaturedPackets(ctx sdk.Context) {
 				sdk.NewAttribute(ccv.AttributeChainID, ctx.ChainID()),
 				sdk.NewAttribute(ccv.AttributeConsumerHeight, strconv.Itoa(int(ctx.BlockHeight()))),
 				sdk.NewAttribute(ccv.AttributeValSetUpdateID, strconv.Itoa(int(maturityTime.VscId))),
-				sdk.NewAttribute(ccv.AttributeTimestamp, strconv.Itoa(int(currentTime))),
+				sdk.NewAttribute(ccv.AttributeTimestamp, ctx.BlockTime().String()),
 			),
 		)
 	}

diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go
@@ -143,9 +143,12 @@ func TestOnRecvVSCPacket(t *testing.T) {
 		})
 		require.Equal(t, tc.expectedPendingChanges, *actualPendingChanges, "pending changes not equal to expected changes after successful packet receive. case: %s", tc.name)
 
-		expectedTime := uint64(ctx.BlockTime().Add(consumerKeeper.GetUnbondingPeriod(ctx)).UnixNano())
-		maturityTime := consumerKeeper.GetPacketMaturityTime(ctx, tc.newChanges.ValsetUpdateId)
-		require.Equal(t, expectedTime, maturityTime, "packet maturity time has unexpected value for case: %s", tc.name)
+		expectedTime := ctx.BlockTime().Add(consumerKeeper.GetUnbondingPeriod(ctx))
+		require.True(
+			t,
+			consumerKeeper.PacketMaturityTimeExists(ctx, tc.newChanges.ValsetUpdateId, expectedTime),
+			"no packet maturity time for case: %s", tc.name,
+		)
 	}
 }