Always queue vsc matured packet data (#624)

* wip

* tests

* finish e2e test

* Update relay.go

* small

* fix after merge

Co-authored-by: Marius Poke <[email protected]>

tests/e2e/throttle.go

@@ -693,6 +693,76 @@ func (s CCVTestSuite) TestSlashAllValidators() {
 	// "applying the validator changes would result in empty set".
 }
 
+func (s *CCVTestSuite) TestLeadingVSCMaturedAreDequeued() {
+
+	s.SetupAllCCVChannels()
+	providerKeeper := s.providerApp.GetProviderKeeper()
+
+	// Queue up 50 vsc matured packets for each consumer
+	for _, bundle := range s.consumerBundles {
+		for i := 0; i < 50; i++ {
+			ibcSeqNum := uint64(i)
+			packet := s.constructVSCMaturedPacketFromConsumer(*bundle, ibcSeqNum)
+			packetData := ccvtypes.ConsumerPacketData{}
+			ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetData)
+			providerKeeper.OnRecvVSCMaturedPacket(s.providerCtx(),
+				packet, *packetData.GetVscMaturedPacketData())
+		}
+	}
+
+	// Queue up 50 slash packets for each consumer
+	for _, bundle := range s.consumerBundles {
+		for i := 0; i < 50; i++ {
+			ibcSeqNum := uint64(i)
+			packet := s.constructSlashPacketFromConsumer(*bundle,
+				*s.providerChain.Vals.Validators[0], stakingtypes.Downtime, ibcSeqNum)
+			packetData := ccvtypes.ConsumerPacketData{}
+			ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetData)
+			providerKeeper.OnRecvSlashPacket(s.providerCtx(),
+				packet, *packetData.GetSlashPacketData())
+		}
+	}
+
+	// Queue up another 50 vsc matured packets for each consumer
+	for _, bundle := range s.consumerBundles {
+		for i := 0; i < 50; i++ {
+			ibcSeqNum := uint64(i)
+			packet := s.constructVSCMaturedPacketFromConsumer(*bundle, ibcSeqNum)
+			packetData := ccvtypes.ConsumerPacketData{}
+			ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetData)
+			providerKeeper.OnRecvVSCMaturedPacket(s.providerCtx(),
+				packet, *packetData.GetVscMaturedPacketData())
+		}
+	}
+
+	// Confirm queue size is 150 for each consumer-specific queue.
+	for _, bundle := range s.consumerBundles {
+		s.Require().Equal(uint64(150),
+			providerKeeper.GetThrottledPacketDataSize(s.providerCtx(), bundle.Chain.ChainID))
+	}
+	// Confirm global queue size is 50 * 5 (50 slash packets for each of 5 consumers)
+	globalEntries := providerKeeper.GetAllGlobalSlashEntries(s.providerCtx())
+	s.Require().Equal(len(globalEntries), 50*5)
+
+	// Set slash meter to negative value to not allow any slash packets to be handled.
+	providerKeeper.SetSlashMeter(s.providerCtx(), sdktypes.NewInt(-1))
+
+	// Set last full time to block time, so no slash meter replenishment happens on end block.
+	providerKeeper.SetLastSlashMeterFullTime(s.providerCtx(), s.providerCtx().BlockTime())
+
+	// Execute end blocker to dequeue only the leading vsc matured packets.
+	s.providerChain.NextBlock()
+
+	// Confirm queue size is 100 for each consumer-specific queue (50 leading vsc matured are dequeued).
+	for _, bundle := range s.consumerBundles {
+		s.Require().Equal(uint64(100),
+			providerKeeper.GetThrottledPacketDataSize(s.providerCtx(), bundle.Chain.ChainID))
+	}
+
+	// No slash packets handled, global slash queue is same size as last block.
+	s.Require().Equal(len(globalEntries), 50*5)
+}
+
 func (s *CCVTestSuite) confirmValidatorJailed(tmVal tmtypes.Validator, checkPower bool) {
 	sdkVal, found := s.providerApp.GetE2eStakingKeeper().GetValidator(
 		s.providerCtx(), sdktypes.ValAddress(tmVal.Address))

testutil/e2e/debug_test.go

@@ -184,6 +184,10 @@ func TestSlashAllValidators(t *testing.T) {
 	runCCVTestByName(t, "TestSlashAllValidators")
 }
 
+func TestLeadingVSCMaturedAreDequeued(t *testing.T) {
+	runCCVTestByName(t, "TestLeadingVSCMaturedAreDequeued")
+}
+
 //
 // Unbonding tests
 //

x/ccv/provider/keeper/relay.go

@@ -17,16 +17,6 @@ import (
 	utils "github.com/cosmos/interchain-security/x/ccv/utils"
 )
 
-func removeStringFromSlice(slice []string, x string) (newSlice []string, numRemoved int) {
-	for _, y := range slice {
-		if x != y {
-			newSlice = append(newSlice, y)
-		}
-	}
-
-	return newSlice, len(slice) - len(newSlice)
-}
-
 // OnRecvVSCMaturedPacket handles a VSCMatured packet
 func (k Keeper) OnRecvVSCMaturedPacket(
 	ctx sdk.Context,
@@ -42,19 +32,31 @@ func (k Keeper) OnRecvVSCMaturedPacket(
 		panic(fmt.Errorf("VSCMaturedPacket received on unknown channel %s", packet.DestinationChannel))
 	}
 
-	// If no packets are in the chain specific throttled packet data queue,
-	// immediately handle the vsc matured packet data.
-	if k.GetThrottledPacketDataSize(ctx, chainID) == 0 {
-		k.HandleVSCMaturedPacket(ctx, chainID, data)
-	} else {
-		// Otherwise queue the packet data (behind one or more throttled slash packet data instances)
-		k.QueueThrottledVSCMaturedPacketData(ctx, chainID, packet.Sequence, data)
-	}
+	k.QueueThrottledVSCMaturedPacketData(ctx, chainID, packet.Sequence, data)
 
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 	return ack
 }
 
+// HandleLeadingVSCMaturedPackets handles all VSCMatured packet data that has been queued this block,
+// but does not need to be throttled. The handled data is then removed from the queue.
+//
+// Note: VSC matured packet data which is queued behind slash packet data CANNOT be
+// handled until the leading slash packet data has been handled. This is to maintain
+// the "VSC Maturity and Slashing Order" CCV property. If VSC matured packet data DOES NOT
+// trail slash packet data for that consumer, it will be handled in this method,
+// bypassing HandleThrottleQueues.
+func (k Keeper) HandleLeadingVSCMaturedPackets(ctx sdk.Context) {
+
+	for _, chain := range k.GetAllConsumerChains(ctx) {
+		leadingVscMatured, ibcSeqNums := k.GetLeadingVSCMaturedData(ctx, chain.ChainId)
+		for _, data := range leadingVscMatured {
+			k.HandleVSCMaturedPacket(ctx, chain.ChainId, data)
+		}
+		k.DeleteThrottledPacketData(ctx, chain.ChainId, ibcSeqNums...)
+	}
+}
+
 // HandleVSCMaturedPacket handles a VSCMatured packet.
 //
 // Note: This method should only panic for a system critical error like a
@@ -233,6 +235,8 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) {
 	// This ensures the meter value is replenished, and not greater than the allowance (max value)
 	// for the block, before the throttling logic is executed.
 	k.CheckForSlashMeterReplenishment(ctx)
+	// Handle leading vsc matured packets before throttling logic
+	k.HandleLeadingVSCMaturedPackets(ctx)
 	// Execute slash packet throttling logic
 	k.HandleThrottleQueues(ctx)
 }
@@ -427,6 +431,16 @@ func (k Keeper) EndBlockCCR(ctx sdk.Context) {
 	}
 }
 
+func removeStringFromSlice(slice []string, x string) (newSlice []string, numRemoved int) {
+	for _, y := range slice {
+		if x != y {
+			newSlice = append(newSlice, y)
+		}
+	}
+
+	return newSlice, len(slice) - len(newSlice)
+}
+
 // getMappedInfractionHeight gets the infraction height mapped from val set ID for the given chain ID
 func (k Keeper) getMappedInfractionHeight(ctx sdk.Context,
 	chainID string, valsetUpdateID uint64) (height uint64, found bool) {

x/ccv/provider/keeper/relay_test.go

@@ -109,10 +109,10 @@ func TestOnRecvVSCMaturedPacket(t *testing.T) {
 	ack := executeOnRecvVSCMaturedPacket(t, &providerKeeper, ctx, "channel-1", 1)
 	require.Equal(t, channeltypes.NewResultAcknowledgement([]byte{byte(1)}), ack)
 
-	// Assert that the packet data was handled immediately, not queued
-	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+	// Assert that the packet data was queued for chain-1
+	require.Equal(t, uint64(1), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
 
-	// Other queue still empty
+	// chain-2 queue empty
 	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
 
 	// Now queue a slash packet data instance for chain-2, then confirm the on recv method
@@ -122,26 +122,99 @@ func TestOnRecvVSCMaturedPacket(t *testing.T) {
 	require.Equal(t, channeltypes.NewResultAcknowledgement([]byte{byte(1)}), ack)
 	require.Equal(t, uint64(2), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
 
-	// Other queue still empty
-	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+	// Chain-1 still has 1 packet data queued
+	require.Equal(t, uint64(1), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
 
-	// Receive 5 more vsc matured packets for chain-2, then confirm chain-2 queue size is 7, chain-1 still 0
+	// Receive 5 more vsc matured packets for chain-2, then confirm chain-2 queue size is 7, chain-1 still size 1
 	for i := 0; i < 5; i++ {
 		ack = executeOnRecvVSCMaturedPacket(t, &providerKeeper, ctx, "channel-2", uint64(i+3))
 		require.Equal(t, channeltypes.NewResultAcknowledgement([]byte{byte(1)}), ack)
 	}
 	require.Equal(t, uint64(7), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
-	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+	require.Equal(t, uint64(1), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
 
 	// Delete chain-2's data from its queue, then confirm the queue size is 0
 	providerKeeper.DeleteThrottledPacketData(ctx, "chain-2", []uint64{1, 2, 3, 4, 5, 6, 7}...)
 	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
+}
 
-	// Finally, confirm vsc packet data will again be handled immediately
-	ack = executeOnRecvVSCMaturedPacket(t, &providerKeeper, ctx, "channel-2", 8)
-	require.Equal(t, channeltypes.NewResultAcknowledgement([]byte{byte(1)}), ack)
-	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
-	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+func TestHandleLeadingVSCMaturedPackets(t *testing.T) {
+
+	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+	providerKeeper.SetParams(ctx, providertypes.DefaultParams())
+
+	vscData := getTenSampleVSCMaturedPacketData()
+
+	// Set channel to chain, and chain to client mappings
+	// (faking multiple established consumer channels)
+	providerKeeper.SetChannelToChain(ctx, "channel-1", "chain-1")
+	providerKeeper.SetConsumerClientId(ctx, "chain-1", "client-1")
+	providerKeeper.SetChannelToChain(ctx, "channel-2", "chain-2")
+	providerKeeper.SetConsumerClientId(ctx, "chain-2", "client-2")
+
+	// Queue some leading vsc matured packet data for chain-1
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-1", 1, vscData[0])
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-1", 2, vscData[1])
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-1", 3, vscData[2])
+
+	// Queue some trailing slash packet data (and a couple more vsc matured)
+	providerKeeper.QueueThrottledSlashPacketData(ctx, "chain-1", 4, testkeeper.GetNewSlashPacketData())
+	providerKeeper.QueueThrottledSlashPacketData(ctx, "chain-1", 5, testkeeper.GetNewSlashPacketData())
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-1", 6, vscData[3])
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-1", 7, vscData[4])
+
+	// Queue some leading vsc matured packet data for chain-2
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-2", 1, vscData[5])
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-2", 2, vscData[6])
+
+	// And trailing slash packet data for chain-2
+	providerKeeper.QueueThrottledSlashPacketData(ctx, "chain-2", 3, testkeeper.GetNewSlashPacketData())
+	providerKeeper.QueueThrottledSlashPacketData(ctx, "chain-2", 4, testkeeper.GetNewSlashPacketData())
+
+	// And one more trailing vsc matured packet for chain-2
+	providerKeeper.QueueThrottledVSCMaturedPacketData(ctx, "chain-2", 5, vscData[7])
+
+	// Set VSC Send timestamps for each recv vsc matured packet
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-1", vscData[0].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-1", vscData[1].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-1", vscData[2].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-1", vscData[3].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-1", vscData[4].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-2", vscData[5].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-2", vscData[6].ValsetUpdateId, time.Now())
+	providerKeeper.SetVscSendTimestamp(ctx, "chain-2", vscData[7].ValsetUpdateId, time.Now())
+
+	// Confirm each chain-specific queue has the expected number of packet data instances
+	require.Equal(t, uint64(7), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+	require.Equal(t, uint64(5), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
+
+	// Handle leading vsc matured packets and confirm queue sizes change for both chains
+	providerKeeper.HandleLeadingVSCMaturedPackets(ctx)
+	require.Equal(t, uint64(4), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-1"))
+	require.Equal(t, uint64(3), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-2"))
+
+	// Confirm the leading vsc matured packet data was handled for both chains,
+	// but not the vsc matured packet data that trails slash data in the queue.
+	// This assertion is made by checking that VSC Send timestamps were deleted for
+	// handled vsc matured packet data.
+	_, found := providerKeeper.GetVscSendTimestamp(ctx, "chain-1", vscData[0].ValsetUpdateId)
+	require.False(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-1", vscData[1].ValsetUpdateId)
+	require.False(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-1", vscData[2].ValsetUpdateId)
+	require.False(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-1", vscData[3].ValsetUpdateId)
+	require.True(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-1", vscData[4].ValsetUpdateId)
+	require.True(t, found)
+
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-2", vscData[5].ValsetUpdateId)
+	require.False(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-2", vscData[6].ValsetUpdateId)
+	require.False(t, found)
+	_, found = providerKeeper.GetVscSendTimestamp(ctx, "chain-2", vscData[7].ValsetUpdateId)
+	require.True(t, found)
 }
 
 // TestOnRecvSlashPacket tests the OnRecvSlashPacket method, and how it interacts with the

x/ccv/provider/keeper/throttle.go

@@ -71,6 +71,7 @@ func (k Keeper) GetEffectiveValPower(ctx sdktypes.Context,
 
 // HandlePacketDataForChain handles only the first queued slash packet relevant to the passed consumer chainID,
 // and then handles any trailing vsc matured packets in that (consumer chain specific) throttled packet data queue.
+// The handled data is then deleted from the queue.
 //
 // Note: Any packet data which is handled in this method is also deleted from the (consumer chain specific) queue.
 func (k Keeper) HandlePacketDataForChain(ctx sdktypes.Context, consumerChainID string,
@@ -321,6 +322,43 @@ func (k Keeper) QueueThrottledPacketData(
 	k.IncrementThrottledPacketDataSize(ctx, consumerChainID)
 }
 
+// GetLeadingVSCMaturedData returns the leading vsc matured packet data instances
+// for a chain-specific throttled packet data queue. Ie the vsc matured packet data instances
+// that do not have any slash packet data instances preceding them in the queue for consumerChainID.
+func (k Keeper) GetLeadingVSCMaturedData(ctx sdktypes.Context, consumerChainID string) (
+	vscMaturedData []ccvtypes.VSCMaturedPacketData, ibcSeqNums []uint64) {
+
+	store := ctx.KVStore(k.storeKey)
+	iteratorPrefix := providertypes.ChainIdWithLenKey(providertypes.ThrottledPacketDataBytePrefix, consumerChainID)
+	iterator := sdktypes.KVStorePrefixIterator(store, iteratorPrefix)
+	defer iterator.Close()
+
+	// Iterate over the throttled packet data queue,
+	// and return vsc matured packet data instances until we encounter a slash packet data instance.
+	vscMaturedData = []ccvtypes.VSCMaturedPacketData{}
+	ibcSeqNums = []uint64{}
+	for ; iterator.Valid(); iterator.Next() {
+
+		bz := iterator.Value()
+		if bz[0] == slashPacketData {
+			break
+		} else if bz[0] != vscMaturedPacketData {
+			panic(fmt.Sprintf("unexpected packet data type: %d", bz[0]))
+		}
+
+		var data ccvtypes.VSCMaturedPacketData
+		err := data.Unmarshal(bz[1:])
+		if err != nil {
+			panic(fmt.Sprintf("failed to unmarshal vsc matured packet data: %v", err))
+		}
+
+		vscMaturedData = append(vscMaturedData, data)
+		_, ibcSeqNum := providertypes.MustParseThrottledPacketDataKey(iterator.Key())
+		ibcSeqNums = append(ibcSeqNums, ibcSeqNum)
+	}
+	return vscMaturedData, ibcSeqNums
+}
+
 // GetSlashAndTrailingData returns the first slash packet data instance and any
 // trailing vsc matured packet data instances in the chain-specific throttled packet data queue.
 //