v1 result and change tests

tests/integration/throttle_retry.go

@@ -8,9 +8,12 @@ import (
 	ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"
 )
 
-// Note we don't fully test IBC integration in favor of being able to test ack results better
+// TestSlashRetries tests the throttling v2 retry logic. Without provider changes,
+// the consumer will queue up a slash packet, the provider will return a v1 result,
+// and the consumer will never need to retry.
 //
-// See FSM explanation in throttle_retry.go
+// Once provider changes are made (slash packet queuing is removed), the consumer may retry packets
+// via new result acks from the provider.
 //
 // TODO: This test will need updating once provider changes are made.
 func (s *CCVTestSuite) TestSlashRetries() {
@@ -42,7 +45,7 @@ func (s *CCVTestSuite) TestSlashRetries() {
 	s.Require().False(found)
 
 	//
-	// Test section
+	// Test section: See FSM explanation in throttle_retry.go
 	//
 
 	// Construct a mock slash packet from consumer
@@ -58,10 +61,10 @@ func (s *CCVTestSuite) TestSlashRetries() {
 	consumerKeeper.UpdateSlashRecordOnSend(s.consumerCtx())
 	s.Require().Len(consumerKeeper.GetPendingPackets(s.consumerCtx()), 1)
 
-	// Recv packet on provider and assert ack. Provider should return no-op result since packet is handled.
+	// Recv packet on provider and assert ack. Provider should return v1 result.
 	ack := providerModule.OnRecvPacket(s.providerCtx(), packet1, nil)
-	expectedNoOpAck := channeltypes.NewResultAcknowledgement([]byte(ccvtypes.NoOpResult))
-	s.Require().Equal(expectedNoOpAck.Acknowledgement(), ack.Acknowledgement())
+	expectedv1Ack := channeltypes.NewResultAcknowledgement([]byte(ccvtypes.V1Result))
+	s.Require().Equal(expectedv1Ack.Acknowledgement(), ack.Acknowledgement())
 
 	// Couple blocks pass on provider for staking keeper to process jailing
 	s.providerChain.NextBlock()
@@ -72,20 +75,29 @@ func (s *CCVTestSuite) TestSlashRetries() {
 	s.Require().True(vals[1].IsJailed())
 	s.Require().Equal(int64(0),
 		s.providerApp.GetTestStakingKeeper().GetLastValidatorPower(s.providerCtx(), vals[1].GetOperator()))
+	s.Require().Equal(uint64(0), providerKeeper.GetThrottledPacketDataSize(s.providerCtx(),
+		s.getFirstBundle().Chain.ChainID))
 
 	// Now slash meter should be negative on provider
 	s.Require().True(s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx()).IsNegative())
 
 	// Apply ack back on consumer
-	ackForConsumer := expectedNoOpAck
+	ackForConsumer := expectedv1Ack
 	err := consumerKeeper.OnAcknowledgementPacket(s.consumerCtx(), packet1, ackForConsumer)
 	s.Require().NoError(err)
 
 	// Slash record should have been deleted, head of pending packets should have been popped
+	// Since provider has handled the packet
 	_, found = consumerKeeper.GetSlashRecord(s.consumerCtx())
 	s.Require().False(found)
 	s.Require().Empty(consumerKeeper.GetPendingPackets(s.consumerCtx()))
 
+	// pass two blocks on provider and consumer for good measure
+	s.providerChain.NextBlock()
+	s.providerChain.NextBlock()
+	s.consumerChain.NextBlock()
+	s.consumerChain.NextBlock()
+
 	// Construct and mock the sending of a second packet on consumer
 	tmval2 := s.providerChain.Vals.Validators[2]
 	packet2 := s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval2, stakingtypes.Infraction_INFRACTION_DOWNTIME, 1)
@@ -102,26 +114,32 @@ func (s *CCVTestSuite) TestSlashRetries() {
 	s.Require().Len(consumerKeeper.GetPendingPackets(s.consumerCtx()), 1)
 
 	// Recv 2nd slash packet on provider for different validator.
-	// Provider should return bounce result since packet is not handled.
+	// Provider should return the same v1 result ack even tho the packet was queued.
 	ack = providerModule.OnRecvPacket(s.providerCtx(), packet2, nil)
-	expectedBouncedAck := channeltypes.NewResultAcknowledgement([]byte(ccvtypes.SlashPacketBouncedResult))
-	s.Require().Equal(expectedBouncedAck.Acknowledgement(), ack.Acknowledgement())
+	expectedv1Ack = channeltypes.NewResultAcknowledgement([]byte(ccvtypes.V1Result))
+	s.Require().Equal(expectedv1Ack.Acknowledgement(), ack.Acknowledgement())
+
+	// Couple blocks pass on provider for staking keeper to process jailings
+	s.providerChain.NextBlock()
+	s.providerChain.NextBlock()
 
-	// Val shouldn't be jailed on provider
+	// Val shouldn't be jailed on provider. Slash packet was queued
 	s.Require().False(vals[2].IsJailed())
 	s.Require().Equal(int64(1000),
 		providerStakingKeeper.GetLastValidatorPower(s.providerCtx(), vals[2].GetOperator()))
+	s.Require().Equal(uint64(1), providerKeeper.GetThrottledPacketDataSize(s.providerCtx(),
+		s.getFirstBundle().Chain.ChainID))
 
 	// Apply ack on consumer
-	ackForConsumer = channeltypes.NewResultAcknowledgement(ack.Acknowledgement()) // Shim since provider uses a different type
+	ackForConsumer = expectedv1Ack
 	err = consumerKeeper.OnAcknowledgementPacket(s.consumerCtx(), packet2, ackForConsumer)
 	s.Require().NoError(err)
 
-	// slashRecord.WaitingOnReply should have been updated to false
-	slashRecord, found = consumerKeeper.GetSlashRecord(s.consumerCtx())
-	s.Require().True(found)
-	s.Require().False(slashRecord.WaitingOnReply)
+	// TODO: when provider changes are made, slashRecord.WaitingOnReply should have been updated to false. Packet still in queue
 
-	// Packet still in queue
-	s.Require().Len(consumerKeeper.GetPendingPackets(s.consumerCtx()), 1)
+	// Slash record should have been deleted, head of pending packets should have been popped
+	// Since provider has handled the packet
+	_, found = consumerKeeper.GetSlashRecord(s.consumerCtx())
+	s.Require().False(found)
+	s.Require().Empty(consumerKeeper.GetPendingPackets(s.consumerCtx()))
 }

x/ccv/consumer/keeper/relay.go

@@ -239,7 +239,16 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac
 			k.Logger(ctx).Error("recv invalid ack; expected length 1", "channel", packet.SourceChannel, "ack", res)
 		}
 		switch res[0] {
-		case ccv.NoOpResult[0]:
+		case ccv.V1Result[0]:
+			// If slash record is found, slash packet is at head of queue.
+			// But provider is running v1 throttling and has queued the slash packet itself.
+			// Therefore we can clear the slash record and delete the slash packet from the queue, unblocking packet sending.
+			// TODO: tests for this scenario with vsc matured.
+			_, found := k.GetSlashRecord(ctx)
+			if found {
+				k.ClearSlashRecord(ctx)
+				k.DeleteHeadOfPendingPackets(ctx)
+			}
 			k.Logger(ctx).Info("recv no-op ack", "channel", packet.SourceChannel, "ack", res)
 		case ccv.SlashPacketHandledResult[0]:
 			k.ClearSlashRecord(ctx)           // Clears slash record state, unblocks sending of pending packets.

x/ccv/consumer/keeper/relay_test.go

@@ -393,7 +393,7 @@ func TestOnAcknowledgementPacketResult(t *testing.T) {
 	require.Equal(t, types.SlashPacket, consumerKeeper.GetPendingPackets(ctx)[0].Type)
 
 	// No-op result shouldn't do anything
-	ack := channeltypes.NewResultAcknowledgement(types.NoOpResult)
+	ack := channeltypes.NewResultAcknowledgement(types.V1Result)
 	err := consumerKeeper.OnAcknowledgementPacket(ctx, packet, ack)
 	require.Nil(t, err)
 	_, found = consumerKeeper.GetSlashRecord(ctx)

x/ccv/consumer/keeper/throttle_retry.go

@@ -22,7 +22,12 @@ import (
 //
 // The slash packet remains at the head of the pending packets queue within the "Standby" state.
 //
-// - If the consumer receives a reply from the provider that the slash packet was successfully handled,
+// - If the consumer receives a V1Result ack from the provider, the consumer checks for a slash record,
+// and if found, the consumer transitions from "Standby" to "No Slash". The slash record is cleared upon this transition,
+// and the slash packet is popped from the pending packets queue.
+// TODO: Make note of above design for v1 throttling providers in the ADR, and explain that consumers must upgrade first in prod (where double queueing may exist for some time).
+//
+// - Else if the consumer receives a reply from the provider that the slash packet was successfully handled,
 // the consumer transitions from "Standby" to "No Slash". The slash record is cleared upon this transition,
 // and the slash packet is popped from the pending packets queue.
 //

x/ccv/provider/keeper/relay.go

@@ -42,7 +42,7 @@ func (k Keeper) OnRecvVSCMaturedPacket(
 		"vscID", data.ValsetUpdateId,
 	)
 
-	ack := channeltypes.NewResultAcknowledgement(ccv.NoOpResult)
+	ack := channeltypes.NewResultAcknowledgement(ccv.V1Result)
 	return ack
 }
 
@@ -353,7 +353,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d
 
 		// return successful ack, as an error would result
 		// in the consumer closing the CCV channel
-		return channeltypes.NewResultAcknowledgement(ccv.NoOpResult)
+		return channeltypes.NewResultAcknowledgement(ccv.V1Result)
 	}
 
 	// Queue a slash entry to the global queue, which will be seen by the throttling logic
@@ -377,7 +377,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d
 		"infractionType", data.Infraction,
 	)
 
-	return channeltypes.NewResultAcknowledgement(ccv.NoOpResult)
+	return channeltypes.NewResultAcknowledgement(ccv.V1Result)
 }
 
 // ValidateSlashPacket validates a recv slash packet before it is

x/ccv/types/ccv.go

@@ -167,12 +167,14 @@ type PacketAckResult []byte
 
 var ( // slice types can't be const
 
-	// No-op result ack. These are sent by the provider to indicate that the packet was received,
-	// and no actions are required by the consumer. Throttling v1 always sends this ack for slash and VSCMatured packets.
-	NoOpResult = PacketAckResult([]byte{byte(1)})
-	// Slash packet handled result ack, sent by the provider to indicate that a bouncing slash packet was handled.
+	// The result ack that has historically been sent from the provider.
+	// A provider with v1 throttling sends these acks for both slash and vsc matured packets.
+	// A provider with v2 throttling sends this ack for vsc matured packets only.
+	V1Result = PacketAckResult([]byte{byte(1)})
+	// Slash packet handled result ack, sent by a throttling v2 provider to indicate that a slash packet was handled.
 	SlashPacketHandledResult = PacketAckResult([]byte{byte(2)})
-	// Slash packet bounced result ack, sent by the provider to indicate that a bouncing slash packet was NOT handled.
+	// Slash packet bounced result ack, sent by a throttling v2 provider to indicate that a slash packet was NOT handled
+	// and should eventually be retried.
 	SlashPacketBouncedResult = PacketAckResult([]byte{byte(3)})
 )