diff --git a/tests/integration/throttle_retry.go b/tests/integration/throttle_retry.go index c7222372e6..85117a0c6c 100644 --- a/tests/integration/throttle_retry.go +++ b/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())) } diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 518bbaf89e..a304271e7e 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/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. diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index 386dc1a917..fe6232bde3 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/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) diff --git a/x/ccv/consumer/keeper/throttle_retry.go b/x/ccv/consumer/keeper/throttle_retry.go index 930aaa0256..d51f943415 100644 --- a/x/ccv/consumer/keeper/throttle_retry.go +++ b/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. // diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 4a1af16f42..953816bd58 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/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 diff --git a/x/ccv/types/ccv.go b/x/ccv/types/ccv.go index 9f3b982e7e..357d2ed654 100644 --- a/x/ccv/types/ccv.go +++ b/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)}) )