From f4d44e157f4bdd1fc9fe5018625749fa0b0532f5 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 7 Nov 2023 09:04:51 +0100 Subject: [PATCH 01/13] update CHANGELOG for final release --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5c82babcb..21ae06d5de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Add an entry to the unreleased section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a release. +## v2.2.0-provider-lsm + ### Cryptographic verification of equivocation * New feature enabling the provider chain to verify equivocation evidence on its own instead of trusting consumer chains, see [EPIC](https://github.com/cosmos/interchain-security/issues/732). From eca31c6ea42096198255a89884d58a105a055a38 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Thu, 9 Nov 2023 16:28:04 +0100 Subject: [PATCH 02/13] save --- testutil/crypto/evidence.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index 3f75010fa1..4bebd4b95c 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -89,3 +89,6 @@ func MakeAndSignVoteWithForgedValAddress( vote.Signature = v.Signature return vote } + + +func NilifyCommit(valAddr []) \ No newline at end of file From f4920c544cab65a14df6789a9c5dec1e4fc8dd31 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 08:47:50 +0100 Subject: [PATCH 03/13] update test to extract byzantine validators --- tests/integration/misbehaviour.go | 88 +++++++++++++++++++-------- testutil/crypto/evidence.go | 35 ++++++++++- x/ccv/provider/keeper/misbehaviour.go | 4 +- 3 files changed, 97 insertions(+), 30 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 766fa41e5b..83444d463c 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" ) @@ -26,18 +27,20 @@ func (s *CCVTestSuite) TestHandleConsumerMisbehaviour() { clientTMValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators) clientSigners := s.consumerChain.Signers + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + misb := &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, - Header1: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - altTime, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ), + Header1: clientHeader, // create a different header by changing the header timestamp only // in order to create an equivocation, i.e. both headers have the same deterministic states Header2: s.consumerChain.CreateTMClientHeader( @@ -211,6 +214,35 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { []*tmtypes.Validator{}, true, }, + { + "validators who did vote nil should not be returned", + func() *ibctmtypes.Misbehaviour { + // create a valid header with a different hash + // and commit round + clientHeader := s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime.Add(time.Minute), + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ) + + // modify header commits in order to have 2/4 validators voting nil + testutil.UpdateHeaderCommitWithNilVotes(clientHeader, clientTMValset.Validators[:2]) + + return &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: clientHeader, + Header2: clientHeader, + } + }, + // Expect to validators who did NOT vote nil + clientTMValset.Validators[2:], + true, + }, } for _, tc := range testCases { @@ -223,21 +255,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 the bad header 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,6 +296,12 @@ 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()] + + // create an alternative validator set using less than 1/3 of the trusted validator set + altValset2 := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:1]) + altSigners2 := make(map[string]tmtypes.PrivValidator, 1) + altSigners2[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] + testCases := []struct { name string misbehaviour *ibctmtypes.Misbehaviour @@ -345,7 +379,7 @@ 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( @@ -358,22 +392,23 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { clientTMValset, clientSigners, ), - // create header using a different validator set + // the resulting Header2 will have a different BlockID + // than Header1 since doesn't share the same valset and signers Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), clientHeight, headerTs, - altValset, - altValset, + altValset2, + altValset2, clientTMValset, - altSigners, + altSigners2, ), }, - 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( @@ -386,8 +421,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { clientTMValset, clientSigners, ), - // the resulting Header2 will have a different BlockID - // than Header1 since doesn't share the same valset and signers + // 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 4bebd4b95c..8de2ec1b54 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" ) @@ -90,5 +92,36 @@ func MakeAndSignVoteWithForgedValAddress( 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 purpose +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) + } -func NilifyCommit(valAddr []) \ No newline at end of file + for _, v := range validators { + // get validator index in valset + idx, _ := valset.GetByAddress(v.Address) + s := commit.Signatures[idx] + // change BlockIDFlag to nil + s.BlockIDFlag = tmtypes.BlockIDFlagNil + // reassign the signature value + commit.Signatures[idx] = s + } + + // overwrite the commit the back in 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..4890c22171 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 { From c00c55a1b0b7ef0a57961d29a01b56f4fc64bd91 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 09:15:48 +0100 Subject: [PATCH 04/13] improve testing --- tests/integration/misbehaviour.go | 117 +++++++++++++----------------- testutil/crypto/evidence.go | 2 +- 2 files changed, 50 insertions(+), 69 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 83444d463c..d97c993138 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -27,20 +27,18 @@ func (s *CCVTestSuite) TestHandleConsumerMisbehaviour() { clientTMValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators) clientSigners := s.consumerChain.Signers - clientHeader := s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - altTime, - clientTMValset, - clientTMValset, - clientTMValset, - clientSigners, - ) - misb := &ibctmtypes.Misbehaviour{ ClientId: s.path.EndpointA.ClientID, - Header1: clientHeader, + Header1: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + altTime, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ), // create a different header by changing the header timestamp only // in order to create an equivocation, i.e. both headers have the same deterministic states Header2: s.consumerChain.CreateTMClientHeader( @@ -297,30 +295,51 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()] - // create an alternative validator set using less than 1/3 of the trusted validator set - altValset2 := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:1]) - altSigners2 := make(map[string]tmtypes.PrivValidator, 1) - altSigners2[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].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 }{ + { + "identical headers - shouldn't pass", + &ibctmtypes.Misbehaviour{ + ClientId: "clientID", + Header1: clientHeader, + Header2: clientHeader, + }, + false, + }, { "client state not found - shouldn't pass", &ibctmtypes.Misbehaviour{ ClientId: "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+1), @@ -355,16 +374,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), @@ -382,28 +392,8 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { "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, - ), - // the resulting Header2 will have a different BlockID - // than Header1 since doesn't share the same valset and signers - Header2: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, - int64(clientHeight.RevisionHeight+1), - clientHeight, - headerTs, - altValset2, - altValset2, - clientTMValset, - altSigners2, - ), + Header1: clientHeader, + Header2: clientHeaderWithNilVotes, }, false, }, @@ -411,16 +401,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { "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, - ), + Header1: clientHeader, // create header using a different validator set Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index 8de2ec1b54..13a7ae21a6 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -118,7 +118,7 @@ func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmt s := commit.Signatures[idx] // change BlockIDFlag to nil s.BlockIDFlag = tmtypes.BlockIDFlagNil - // reassign the signature value + // update the signatures commit.Signatures[idx] = s } From fc7dec72d8193a07b7c0ebe1caa48355aed0c7ab Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 09:29:16 +0100 Subject: [PATCH 05/13] nits --- tests/integration/misbehaviour.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index d97c993138..ca2870c11b 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -215,8 +215,6 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { { "validators who did vote nil should not be returned", func() *ibctmtypes.Misbehaviour { - // create a valid header with a different hash - // and commit round clientHeader := s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), @@ -228,16 +226,26 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { clientSigners, ) - // modify header commits in order to have 2/4 validators voting nil - testutil.UpdateHeaderCommitWithNilVotes(clientHeader, clientTMValset.Validators[:2]) + // 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: clientHeader, + Header2: clientHeaderWithNilVotes, } }, - // Expect to validators who did NOT vote nil + // Expect validators who did NOT vote nil clientTMValset.Validators[2:], true, }, From 1c0947fcac06734eecf69718c15592c29678fbe6 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 09:30:48 +0100 Subject: [PATCH 06/13] nits --- testutil/crypto/evidence.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index 13a7ae21a6..f4a38f990f 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -122,6 +122,6 @@ func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmt commit.Signatures[idx] = s } - // overwrite the commit the back in the header + // update the commit in client the header header.SignedHeader.Commit = commit.ToProto() } From d8cff59a3fdb8a1ce48f53c72eb1b2746e7cf8cd Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 10:08:32 +0100 Subject: [PATCH 07/13] Update tests/integration/misbehaviour.go Co-authored-by: insumity --- tests/integration/misbehaviour.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index ca2870c11b..2d40c05bd3 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -262,7 +262,7 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { s.Equal(len(tc.expByzantineValidators), len(byzantineValidators)) // For both lunatic and equivocation attacks, all the validators - // who signed the bad header and didn't vote nil should be returned + // who signed both headers and didn't vote nil should be returned if len(tc.expByzantineValidators) > 0 { expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators) s.NoError(err) From 774ff9924f52ad034e46bf0d66604cc8e621f1d8 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 10:08:38 +0100 Subject: [PATCH 08/13] Update testutil/crypto/evidence.go Co-authored-by: insumity --- testutil/crypto/evidence.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index f4a38f990f..beac816097 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -95,7 +95,7 @@ func MakeAndSignVoteWithForgedValAddress( // 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 purpose +// 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", From 52a9421d88f9cca3c8463aa28bfc3086c9a68213 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 10:17:02 +0100 Subject: [PATCH 09/13] update util func --- testutil/crypto/evidence.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go index beac816097..1a2c2cbc7e 100644 --- a/testutil/crypto/evidence.go +++ b/testutil/crypto/evidence.go @@ -115,11 +115,14 @@ func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmt for _, v := range validators { // get validator index in valset idx, _ := valset.GetByAddress(v.Address) - s := commit.Signatures[idx] - // change BlockIDFlag to nil - s.BlockIDFlag = tmtypes.BlockIDFlagNil - // update the signatures - commit.Signatures[idx] = s + 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 From 3a684c01659c1a522b50c05e037c42487f0b31fd Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 11:29:15 +0100 Subject: [PATCH 10/13] doc --- tests/integration/misbehaviour.go | 17 +++++++++-------- x/ccv/provider/keeper/misbehaviour.go | 7 ++++++- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 2d40c05bd3..910571d227 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -337,19 +337,18 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { { "identical headers - shouldn't pass", &ibctmtypes.Misbehaviour{ - ClientId: "clientID", + ClientId: s.path.EndpointA.ClientID, Header1: clientHeader, Header2: clientHeader, }, false, }, { - "client state not found - shouldn't pass", + "misbehaviour isn't for a consumer chain - shouldn't pass", &ibctmtypes.Misbehaviour{ - ClientId: "clientID", - Header1: clientHeader, - Header2: s.consumerChain.CreateTMClientHeader( - s.consumerChain.ChainID, + ClientId: s.path.EndpointA.ClientID, + Header1: s.consumerChain.CreateTMClientHeader( + "aChainID", int64(clientHeight.RevisionHeight+1), clientHeight, headerTs, @@ -358,13 +357,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { clientTMValset, altSigners, ), + Header2: clientHeader, }, false, }, { - "invalid misbehaviour with empty header1 - shouldn't pass", + "client state not found - shouldn't pass", &ibctmtypes.Misbehaviour{ - Header1: &ibctmtypes.Header{}, + ClientId: "clientID", + Header1: clientHeader, Header2: s.consumerChain.CreateTMClientHeader( s.consumerChain.ChainID, int64(clientHeight.RevisionHeight+1), diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index 4890c22171..d01689d96b 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -142,8 +142,13 @@ 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 { + // check that the misbehaviour is for an ICS consumer chain + if _, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID); !found { + return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", misbehaviour.Header1.Header.ChainID) + } + clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID()) if !found { return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID()) From 53098cf8cbbe675a75226d7414c8bc1daa4f22bd Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 13:17:43 +0100 Subject: [PATCH 11/13] check misb client ID --- tests/integration/misbehaviour.go | 2 +- x/ccv/provider/keeper/misbehaviour.go | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 910571d227..306fb06ec4 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -362,7 +362,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() { false, }, { - "client state not found - shouldn't pass", + "client ID doesn't correspond to the client ID of consumer chain - shouldn't pass", &ibctmtypes.Misbehaviour{ ClientId: "clientID", Header1: clientHeader, diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index d01689d96b..daf48c5dac 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -145,8 +145,15 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) { // 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 { // check that the misbehaviour is for an ICS consumer chain - if _, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID); !found { + 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, misbehaviour.GetClientID()) From fcc0fd6340ee12ed7bc1ca937320644a2172de26 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 13:26:20 +0100 Subject: [PATCH 12/13] Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: insumity --- x/ccv/provider/keeper/misbehaviour.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index daf48c5dac..e03c24b627 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -156,7 +156,7 @@ func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbe ) } - clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID()) + clientState, found := k.clientKeeper.GetClientState(ctx, clientId) if !found { return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID()) } From 8918cd3c36e36b252a918efe33a92c367dcb0efc Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 10 Nov 2023 13:27:37 +0100 Subject: [PATCH 13/13] nits --- x/ccv/provider/keeper/misbehaviour.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index e03c24b627..7307284655 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -145,13 +145,13 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) { // 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 { // check that the misbehaviour is for an ICS consumer chain - clientID, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID) + 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 { + } 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, + clientId, misbehaviour.ClientId, ) }