From fb9f7298b823ac2b86640f92b6577bd0ca876bc4 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 14:05:15 +0100 Subject: [PATCH] fix!: drop nil votes in misbehaviour handling (#1404) * update CHANGELOG for final release * save * update test to extract byzantine validators * improve testing * nits * nits * Update tests/integration/misbehaviour.go Co-authored-by: insumity * Update testutil/crypto/evidence.go Co-authored-by: insumity * update util func * doc * check misb client ID * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: insumity * nits --------- Co-authored-by: insumity --- tests/integration/misbehaviour.go | 162 +++++++++++++++----------- testutil/crypto/evidence.go | 39 +++++++ x/ccv/provider/keeper/misbehaviour.go | 20 +++- 3 files changed, 148 insertions(+), 73 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 766fa41e5b..306fb06ec4 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -5,6 +5,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" + testutil "github.com/cosmos/interchain-security/v2/testutil/crypto" "github.com/cosmos/interchain-security/v2/x/ccv/provider/types" tmtypes "github.com/tendermint/tendermint/types" ) @@ -211,6 +212,43 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { []*tmtypes.Validator{}, true, }, + { + "validators who did vote nil should not be returned", + func() *ibctmtypes.Misbehaviour { + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Minute), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // create conflicting header with 2/4 validators voting nil + clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Hour), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:2]) + + return &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeaderWithNilVotes, + } + }, + // Expect validators who did NOT vote nil + clientTMValset.Validators[2:], + true, + }, } for _, tc := range testCases { @@ -223,21 +261,17 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { s.NoError(err) s.Equal(len(tc.expByzantineValidators), len(byzantineValidators)) - // For both lunatic and equivocation attacks all the validators - // who signed the bad header (Header2) should be in returned in the evidence + // For both lunatic and equivocation attacks, all the validators + // who signed both headers and didn't vote nil should be returned if len(tc.expByzantineValidators) > 0 { - equivocatingVals := tc.getMisbehaviour().Header2.ValidatorSet - s.Equal(len(equivocatingVals.Validators), len(byzantineValidators)) - - vs, err := tmtypes.ValidatorSetFromProto(equivocatingVals) + expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators) s.NoError(err) for _, v := range tc.expByzantineValidators { - idx, _ := vs.GetByAddress(v.Address) + idx, _ := expValset.GetByAddress(v.Address) s.True(idx >= 0) } } - } else { s.Error(err) } @@ -268,27 +302,53 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { altSigners := make(map[string]tmtypes.PrivValidator, 1) altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()] + + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // create a conflicting client header with insufficient voting power + // by changing 3/4 of its validators votes to nil + clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + // use a different block time to change the header BlockID + headerTs.Add(time.Hour), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:4]) + testCases := []struct { name string misbehaviour *ibctmtypes.Misbehaviour expPass bool }{ { - "client state not found - shouldn't pass", + "identical headers - shouldn't pass", &ibctmtypes.Misbehaviour{ - ClientId: "clientID", + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeader, + }, + false, + }, + { + "misbehaviour isn't for a consumer chain - shouldn't pass", + &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, Header1: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ), - Header2: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, + "aChainID", int64(clientHeight.RevisionHeight+1), clientHeight, headerTs, @@ -297,13 +357,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { clientTMValset, altSigners, ), + Header2: clientHeader, }, false, }, { - "invalid misbehaviour with empty header1 - shouldn't pass", + "client ID doesn't correspond to the client ID of consumer chain - shouldn't pass", &ibctmtypes.Misbehaviour{ - Header1: &ibctmtypes.Header{}, + ClientId: "clientID", + Header1: clientHeader, Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), @@ -321,16 +383,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { "invalid misbehaviour with different header height - shouldn't pass", &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, - Header1: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ), + Header1: clientHeader, Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+2), @@ -345,49 +398,20 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { false, }, { - "valid misbehaviour - should pass", + "one header of the misbehaviour has insufficient voting power - shouldn't pass", &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, - Header1: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ), - // create header using a different validator set - Header2: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - altValset, - altValset, - clientTMValset, - altSigners, - ), + Header1: clientHeader, + Header2: clientHeaderWithNilVotes, }, - true, + false, }, { - "valid misbehaviour with already frozen client - should pass", + "valid misbehaviour - should pass", &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, - Header1: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ), - // the resulting Header2 will have a different BlockID - // than Header1 since doesn't share the same valset and signers + Header1: clientHeader, + // create header using a different validator set Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index 3f75010fa1..1a2c2cbc7e 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -1,8 +1,10 @@ package crypto import ( + "fmt" "time" + ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" "github.com/tendermint/tendermint/crypto/tmhash" tmtypes "github.com/tendermint/tendermint/types" ) @@ -89,3 +91,40 @@ func MakeAndSignVoteWithForgedValAddress( vote.Signature = v.Signature return vote } + +// UpdateHeaderCommitWithNilVotes updates the given light client header +// by changing the commit BlockIDFlag of the given validators to nil +// +// Note that this method is solely used for testing purposes +func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmtypes.Validator) { + if len(validators) > len(header.ValidatorSet.Validators) { + panic(fmt.Sprintf("cannot change more than %d validators votes: got %d", + len(header.ValidatorSet.Validators), len(header.ValidatorSet.Validators))) + } + + commit, err := tmtypes.CommitFromProto(header.Commit) + if err != nil { + panic(err) + } + + valset, err := tmtypes.ValidatorSetFromProto(header.ValidatorSet) + if err != nil { + panic(err) + } + + for _, v := range validators { + // get validator index in valset + idx, _ := valset.GetByAddress(v.Address) + if idx != -1 { + // get validator commit sig + s := commit.Signatures[idx] + // change BlockIDFlag to nil + s.BlockIDFlag = tmtypes.BlockIDFlagNil + // update the signatures + commit.Signatures[idx] = s + } + } + + // update the commit in client the header + header.SignedHeader.Commit = commit.ToProto() +} diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index c9749a1245..7307284655 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -103,7 +103,7 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes. // create a map with the validators' address that signed header1 header1Signers := map[string]struct{}{} for _, sign := range lightBlock1.Commit.Signatures { - if sign.Absent() { + if !sign.ForBlock() { continue } header1Signers[sign.ValidatorAddress.String()] = struct{}{} @@ -111,7 +111,7 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes. // iterate over the header2 signers and check if they signed header1 for _, sign := range lightBlock2.Commit.Signatures { - if sign.Absent() { + if !sign.ForBlock() { continue } if _, ok := header1Signers[sign.ValidatorAddress.String()]; ok { @@ -142,9 +142,21 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) { } // CheckMisbehaviour checks that headers in the given misbehaviour forms -// a valid light client attack and that the corresponding light client isn't expired +// a valid light client attack from an ICS consumer chain and that the light client isn't expired func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour) error { - clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID()) + // check that the misbehaviour is for an ICS consumer chain + clientId, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID) + if !found { + return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", misbehaviour.Header1.Header.ChainID) + } else if misbehaviour.ClientId != clientId { + return fmt.Errorf("incorrect misbehaviour: expected client ID for consumer chain %s is %s got %s", + misbehaviour.Header1.Header.ChainID, + clientId, + misbehaviour.ClientId, + ) + } + + clientState, found := k.clientKeeper.GetClientState(ctx, clientId) if !found { return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID()) }