fix!: some cleaning up (#2346)

* init commit

* Update testing documentation

* took into account comments

* readded slash acks

---------

Co-authored-by: github-actions <[email protected]>

Author: insumity

scripts/test_doc/test_documentation.md

@@ -87,9 +87,9 @@
  [TestSlashPacketAcknowledgement](../../tests/integration/slashing.go#L185) | TestSlashPacketAcknowledgement tests the handling of a slash packet acknowledgement.<details><summary>Details</summary>* Set up a provider and consumer chain, with channel initialization between them performed.<br>* Send a slash packet with randomized fields from the consumer to the provider.<br>* The provider processes the packet</details> |
  [TestHandleSlashPacketDowntime](../../tests/integration/slashing.go#L236) | TestHandleSlashPacketDowntime tests the handling of a downtime related slash packet, with integration tests.<details><summary>Details</summary>* Retrieve a validator from provider chain's validators and checks if it's bonded.<br>* Set tThe signing information for the validator.<br>* The provider processes the downtime slashing packet from the consumer.<br>* Check that the validator has been jailed as a result of the downtime slashing packet being processed.<br>* Verify that the validator’s signing information is updated and that the jailing duration is set correctly.<br><br>Note that only downtime slash packets are processed by HandleSlashPacket.</details> |
  [TestOnRecvSlashPacketErrors](../../tests/integration/slashing.go#L283) | TestOnRecvSlashPacketErrors tests errors for the OnRecvSlashPacket method in an integration testing setting.<details><summary>Details</summary>* Set up all CCV channels and expect panic if the channel is not established via dest channel of packet.<br>* After the correct channelID is added to the packet, a panic shouldn't occur anymore.<br>* Create an instance of SlashPacketData and then verify correct processing and error handling<br>for slashing packets received by the provider chain.<br>TODO: Move to unit tests.</details> |
- [TestValidatorDowntime](../../tests/integration/slashing.go#L401) | TestValidatorDowntime tests if a slash packet is sent and if the outstanding slashing flag is switched when a validator has downtime on the slashing module.<details><summary>Details</summary>* Set up all CCV channel and send an empty VSC packet, then retrieve the address of a validator.<br>* Validator signs blocks for the duration of the signedBlocksWindow and a slash packet is constructed to be sent and committed.<br>* Simulate the validator missing blocks and then verify that the validator is jailed and the jailed time is correctly updated.<br>* Ensure that the missed block counters are reset.<br>* Check that there is a pending slash packet in the queue, and then send the pending packets.<br>* Check if slash record is created and verify that the consumer queue still contains the packet since no<br>acknowledgment has been received from the provider.<br>* Verify that the slash packet was sent and check that the outstanding slashing flag prevents the jailed validator to keep missing block.</details> |
- [TestQueueAndSendSlashPacket](../../tests/integration/slashing.go#L522) | TestQueueAndSendSlashPacket tests the integration of QueueSlashPacket with SendPackets. In normal operation slash packets are queued in BeginBlock and sent in EndBlock.<details><summary>Details</summary>* Set up all CCV channels and then queue slash packets for both downtime and double-signing infractions.<br>* Check that the correct number of slash requests are stored in the queue, including duplicates for downtime infractions.<br>* Prepare the CCV channel for sending actual slash packets.<br>* Send the slash packets and check that the outstanding downtime flags are correctly set for validators that were slashed<br>for downtime infractions.<br>* Ensure that the pending data packets queue is empty.<br>TODO: Move to unit tests.</details> |
- [TestCISBeforeCCVEstablished](../../tests/integration/slashing.go#L607) | TestCISBeforeCCVEstablished tests that the consumer chain doesn't panic or have any undesired behavior when a slash packet is queued before the CCV channel is established. Then once the CCV channel is established, the slash packet should be sent soon after.<details><summary>Details</summary>* Check that no pending packets exist and that there's no slash record found.<br>* Triggers a slashing event which queues a slash packet.<br>* The slash packet should be queued but not sent, and it should stay like that until the CCV channel is established and the packet is sent.<br>*Verify that a slashing record now exists, indicating that the slashing packet has been successfully sent.</details> |
+ [TestValidatorDowntime](../../tests/integration/slashing.go#L412) | TestValidatorDowntime tests if a slash packet is sent and if the outstanding slashing flag is switched when a validator has downtime on the slashing module.<details><summary>Details</summary>* Set up all CCV channel and send an empty VSC packet, then retrieve the address of a validator.<br>* Validator signs blocks for the duration of the signedBlocksWindow and a slash packet is constructed to be sent and committed.<br>* Simulate the validator missing blocks and then verify that the validator is jailed and the jailed time is correctly updated.<br>* Ensure that the missed block counters are reset.<br>* Check that there is a pending slash packet in the queue, and then send the pending packets.<br>* Check if slash record is created and verify that the consumer queue still contains the packet since no<br>acknowledgment has been received from the provider.<br>* Verify that the slash packet was sent and check that the outstanding slashing flag prevents the jailed validator to keep missing block.</details> |
+ [TestQueueAndSendSlashPacket](../../tests/integration/slashing.go#L533) | TestQueueAndSendSlashPacket tests the integration of QueueSlashPacket with SendPackets. In normal operation slash packets are queued in BeginBlock and sent in EndBlock.<details><summary>Details</summary>* Set up all CCV channels and then queue slash packets for both downtime and double-signing infractions.<br>* Check that the correct number of slash requests are stored in the queue, including duplicates for downtime infractions.<br>* Prepare the CCV channel for sending actual slash packets.<br>* Send the slash packets and check that the outstanding downtime flags are correctly set for validators that were slashed<br>for downtime infractions.<br>* Ensure that the pending data packets queue is empty.<br>TODO: Move to unit tests.</details> |
+ [TestCISBeforeCCVEstablished](../../tests/integration/slashing.go#L618) | TestCISBeforeCCVEstablished tests that the consumer chain doesn't panic or have any undesired behavior when a slash packet is queued before the CCV channel is established. Then once the CCV channel is established, the slash packet should be sent soon after.<details><summary>Details</summary>* Check that no pending packets exist and that there's no slash record found.<br>* Triggers a slashing event which queues a slash packet.<br>* The slash packet should be queued but not sent, and it should stay like that until the CCV channel is established and the packet is sent.<br>*Verify that a slashing record now exists, indicating that the slashing packet has been successfully sent.</details> |
 </details>
 
 # [stop_consumer.go](../../tests/integration/stop_consumer.go) 

tests/integration/slashing.go

@@ -356,16 +356,27 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {
 
 	// Expect the packet to bounce if the slash meter is negative
 	providerKeeper.SetSlashMeter(ctx, math.NewInt(-1))
-	// Only reaches the bouncing code if it fails in the check that chain is launched and the validator is not a consumer validator,
-	// so we set the chain as stopped.
-	providerKeeper.SetConsumerPhase(suite.providerCtx(), firstBundle.ConsumerId, providertypes.CONSUMER_PHASE_STOPPED)
+	// Only reaches the bouncing code if it fails in the check that chain is not launched and in the check that
+	// the validator is not a consumer validator.
+	providerKeeper.SetConsumerPhase(suite.providerCtx(), firstBundle.ConsumerId, providertypes.CONSUMER_PHASE_LAUNCHED)
+	err = providerKeeper.SetConsumerValidator(ctx, firstBundle.ConsumerId, providertypes.ConsensusValidator{
+		ProviderConsAddr: validAddress,
+	})
 	ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
 	suite.Require().NoError(err)
 	suite.Require().Equal(ccv.SlashPacketBouncedResult, ackResult, "expected bounced result")
 
-	// Expect packet not to bounce if the chain is launched
+	// Expect packet not to bounce if the chain is stopped
+	providerKeeper.SetSlashMeter(ctx, math.NewInt(-1))
+	providerKeeper.SetConsumerPhase(suite.providerCtx(), firstBundle.ConsumerId, providertypes.CONSUMER_PHASE_STOPPED)
+	ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
+	suite.Require().NoError(err)
+	suite.Require().Equal(ccv.SlashPacketHandledResult, ackResult, "expected successful ack")
+
+	// Expect packet not to bounce if the chain is launched but the validator is not a consumer validator
 	providerKeeper.SetSlashMeter(ctx, math.NewInt(-1))
 	providerKeeper.SetConsumerPhase(suite.providerCtx(), firstBundle.ConsumerId, providertypes.CONSUMER_PHASE_LAUNCHED)
+	providerKeeper.DeleteConsumerValidator(ctx, firstBundle.ConsumerId, providertypes.NewProviderConsAddress(sdk.ConsAddress(validAddress)))
 	ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
 	suite.Require().Equal(ccv.SlashPacketHandledResult, ackResult, "expected successful ack")
 

x/ccv/provider/keeper/relay.go

@@ -343,10 +343,27 @@ func (k Keeper) OnRecvSlashPacket(
 		return ccv.V1Result, nil
 	}
 
-	// Check that chain is launched and the validator belongs to the consumer chain valset
-	if k.GetConsumerPhase(ctx, consumerId) == providertypes.CONSUMER_PHASE_LAUNCHED && !k.IsConsumerValidator(ctx, consumerId, providerConsAddr) {
-		k.Logger(ctx).Error("cannot jail validator %s that does not belong to consumer %s valset",
-			providerConsAddr.String(), consumerId)
+	// check that the chain is launched
+	if k.GetConsumerPhase(ctx, consumerId) != providertypes.CONSUMER_PHASE_LAUNCHED {
+		k.Logger(ctx).Info("cannot jail validator on a chain that is not currently launched",
+			"consumerId", consumerId,
+			"phase", k.GetConsumerPhase(ctx, consumerId),
+			"provider cons addr", providerConsAddr.String(),
+		)
+
+		// drop packet but return a slash ack
+		k.AppendSlashAck(ctx, consumerId, consumerConsAddr.String())
+
+		return ccv.SlashPacketHandledResult, nil
+	}
+
+	// check that the validator belongs to the consumer chain valset
+	if !k.IsConsumerValidator(ctx, consumerId, providerConsAddr) {
+		k.Logger(ctx).Error("cannot jail validator that does not belong on the consumer valset",
+			"consumerId", consumerId,
+			"provider cons addr", providerConsAddr.String(),
+		)
+
 		// drop packet but return a slash ack so that the consumer can send another slash packet
 		k.AppendSlashAck(ctx, consumerId, consumerConsAddr.String())
 

x/ccv/provider/keeper/relay_test.go

@@ -152,8 +152,14 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
 	providerKeeper.SetParams(ctx, providertypes.DefaultParams())
 
 	// Set channel to chain (faking multiple established channels)
-	providerKeeper.SetChannelToConsumerId(ctx, "channel-1", "chain-1")
-	providerKeeper.SetChannelToConsumerId(ctx, "channel-2", "chain-2")
+	consumerId0 := "0"
+	channelId0 := "channel-0"
+	consumerId1 := "1"
+	channelId1 := "channel-1"
+	providerKeeper.SetChannelToConsumerId(ctx, channelId0, consumerId0)
+	providerKeeper.SetChannelToConsumerId(ctx, channelId1, consumerId1)
+	providerKeeper.SetConsumerPhase(ctx, consumerId0, providertypes.CONSUMER_PHASE_LAUNCHED)
+	providerKeeper.SetConsumerPhase(ctx, consumerId1, providertypes.CONSUMER_PHASE_LAUNCHED)
 
 	// Generate a new slash packet data instance with double sign infraction type
 	packetData := testkeeper.GetNewSlashPacketData()
@@ -163,33 +169,33 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
 	providerKeeper.SetValsetUpdateBlockHeight(ctx, packetData.ValsetUpdateId, uint64(15))
 
 	// Set consumer validator
-	err := providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{
+	err := providerKeeper.SetConsumerValidator(ctx, consumerId0, providertypes.ConsensusValidator{
 		ProviderConsAddr: packetData.Validator.Address,
 	})
 	require.NoError(t, err)
 
 	// Set slash meter to negative value and assert a bounce ack is returned
 	providerKeeper.SetSlashMeter(ctx, math.NewInt(-5))
-	ackResult, err := executeOnRecvSlashPacket(t, &providerKeeper, ctx, "channel-1", 1, packetData)
+	ackResult, err := executeOnRecvSlashPacket(t, &providerKeeper, ctx, channelId0, 1, packetData)
 	require.Equal(t, ccv.SlashPacketBouncedResult, ackResult)
 	require.NoError(t, err)
 
 	// Set consumer validator
-	err = providerKeeper.SetConsumerValidator(ctx, "chain-2", providertypes.ConsensusValidator{
+	err = providerKeeper.SetConsumerValidator(ctx, consumerId1, providertypes.ConsensusValidator{
 		ProviderConsAddr: packetData.Validator.Address,
 	})
 	require.NoError(t, err)
 
 	// Also bounced for chain-2
-	ackResult, err = executeOnRecvSlashPacket(t, &providerKeeper, ctx, "channel-2", 2, packetData)
+	ackResult, err = executeOnRecvSlashPacket(t, &providerKeeper, ctx, channelId1, 2, packetData)
 	require.Equal(t, ccv.SlashPacketBouncedResult, ackResult)
 	require.NoError(t, err)
 
 	// Now set slash meter to positive value and assert slash packet handled result is returned
 	providerKeeper.SetSlashMeter(ctx, math.NewInt(5))
 
 	// Set the consumer validator
-	err = providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{ProviderConsAddr: packetData.Validator.Address})
+	err = providerKeeper.SetConsumerValidator(ctx, consumerId0, providertypes.ConsensusValidator{ProviderConsAddr: packetData.Validator.Address})
 	require.NoError(t, err)
 
 	// Mock call to GetEffectiveValPower, so that it returns 2.
@@ -213,7 +219,7 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
 	gomock.InOrder(calls...)
 
 	// Execute on recv and confirm slash packet handled result is returned
-	ackResult, err = executeOnRecvSlashPacket(t, &providerKeeper, ctx, "channel-1", 1, packetData)
+	ackResult, err = executeOnRecvSlashPacket(t, &providerKeeper, ctx, channelId0, 1, packetData)
 	require.Equal(t, ccv.SlashPacketHandledResult, ackResult)
 	require.NoError(t, err)