From 366e3b128d39e4f478c33dd069702381c7b883f2 Mon Sep 17 00:00:00 2001
From: Karolos Antoniadis <karolos@informal.systems>
Date: Wed, 27 Sep 2023 12:00:25 +0200
Subject: [PATCH] fix error returns and fix flaky test in a better way

---
 tests/integration/double_vote.go      | 45 +++++----------------------
 x/ccv/provider/keeper/misbehaviour.go |  9 +++++-
 2 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/tests/integration/double_vote.go b/tests/integration/double_vote.go
index 9320162318..d2a00e583b 100644
--- a/tests/integration/double_vote.go
+++ b/tests/integration/double_vote.go
@@ -1,8 +1,6 @@
 package integration
 
 import (
-	"bytes"
-
 	cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	testutil "github.com/cosmos/interchain-security/v2/testutil/crypto"
@@ -31,35 +29,7 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 	provValSet, err := tmtypes.ValidatorSetFromProto(s.providerChain.LastHeader.ValidatorSet)
 	s.Require().NoError(err)
 
-	// In what follows, we have the "valid double voting evidence 1 - should pass," and
-	// "valid double voting evidence 2 - should pass" test cases. The first test case has valid double
-	// voting evidence for the consumer chain (using a consumer key), while the second test case has valid double
-	// voting evidence (using a provider key). When the first "valid double voting evidence 1 - should pass" test runs
-	// it would slash the validator that corresponds to the consumer validator. If the second test
-	// "valid double voting evidence 2 - should pass" attempts to slash the same validator, the test would fail
-	// because the validator was already slashed in the first test case.
-	// Depending on what `consuSigner` and the `provSigner` are, that are arbitrarily chosen, when we run the test, we
-	// might have a successful or a failed test. To prevent a flaky test like this, in what follows we check that we find the
-	// validator that corresponds to `consuVal` and then find a `provVal` validator that is not the corresponding
-	// validator of `consuVal` on the provider chain. This way we guarantee that the test is not flaky.
-	allValidators := s.providerApp.GetProviderKeeper().GetAllValidatorsByConsumerAddr(s.providerCtx(), &s.consumerChain.ChainID)
-	consuValIndex := 0
-	for i := 0; i < len(allValidators); i++ {
-		if bytes.Equal(allValidators[i].ConsumerAddr, consuVal.Address.Bytes()) {
-			consuValIndex = i
-			break
-		}
-	}
-
-	provValIndex := 0
-	for i := 0; i < len(provValSet.Validators); i++ {
-		if !bytes.Equal(allValidators[consuValIndex].ProviderAddr, provValSet.Validators[i].Address.Bytes()) {
-			provValIndex = i
-			break
-		}
-	}
-
-	provVal := provValSet.Validators[provValIndex]
+	provVal := provValSet.Validators[0]
 	provSigner := s.providerChain.Signers[provVal.Address.String()]
 
 	blockID1 := testutil.MakeBlockID([]byte("blockhash"), 1000, []byte("partshash"))
@@ -186,15 +156,16 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 		},
 	}
 
-	consuAddr := types.NewConsumerConsAddress(sdk.ConsAddress(consuVal.Address.Bytes()))
-	provAddr := s.providerApp.GetProviderKeeper().GetProviderAddrFromConsumerAddr(s.providerCtx(), s.consumerChain.ChainID, consuAddr)
-
-	validator, _ := s.providerApp.GetTestStakingKeeper().GetValidator(s.providerCtx(), provAddr.ToSdkConsAddr().Bytes())
-	initialTokens := validator.GetTokens().ToDec()
 	for _, tc := range testCases {
 		s.Run(tc.name, func() {
+			consuAddr := types.NewConsumerConsAddress(sdk.ConsAddress(tc.ev.VoteA.ValidatorAddress.Bytes()))
+			provAddr := s.providerApp.GetProviderKeeper().GetProviderAddrFromConsumerAddr(s.providerCtx(), s.consumerChain.ChainID, consuAddr)
+
+			validator, _ := s.providerApp.GetTestStakingKeeper().GetValidator(s.providerCtx(), provAddr.ToSdkConsAddr().Bytes())
+			initialTokens := validator.GetTokens().ToDec()
+
 			// reset context for each run
-			provCtx := s.providerCtx()
+			provCtx, _ := s.providerCtx().CacheContext()
 
 			// if the evidence was built using the validator provider address and key,
 			// we remove the consumer key assigned to the validator otherwise
diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go
index 497767f84b..c53e11b450 100644
--- a/x/ccv/provider/keeper/misbehaviour.go
+++ b/x/ccv/provider/keeper/misbehaviour.go
@@ -60,9 +60,16 @@ func (k Keeper) HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour ibctmty
 		"byzantine validators", provAddrs,
 	)
 
+	// If we fail to slash all validators we return an error. However, if we only fail to slash some validators
+	// we just log an error to avoid having the whole `MsgSubmitMisbehaviour` failing and reverting the partial slashing.
+	if len(errors) == len(byzantineValidators) {
+		return fmt.Errorf("failed to slash, jail, or tombstone all validators: %v", errors)
+	}
+
 	if len(errors) > 0 {
-		return fmt.Errorf("failed to slash, jail, or tombstone validators: %v", errors)
+		logger.Error("failed to slash, jail, or tombstone validators: %v", errors)
 	}
+
 	return nil
 }