From 63e205ece4b3b48d193bb843bc9118441e85c4bf Mon Sep 17 00:00:00 2001 From: Charly Date: Mon, 23 Jan 2023 15:15:19 -0600 Subject: [PATCH 1/5] move misbehaviour check --- CHANGELOG.md | 1 + .../07-tendermint/misbehaviour_handle.go | 25 ++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35bdb13851c..e17075bc417 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#2434](https://github.com/cosmos/ibc-go/pull/2478) Removed all `TypeMsg` constants * (modules/core/exported) [#1689] (https://github.com/cosmos/ibc-go/pull/2539) Removing `GetVersions` from `ConnectionI` interface. * (core/02-connection) [#2419](https://github.com/cosmos/ibc-go/pull/2419) Add optional proof data to proto definitions of `MsgConnectionOpenTry` and `MsgConnectionOpenAck` for host state machines that are unable to introspect their own consensus state. +* (modules/light-clients/07-tendermint)[#2007] Moved non-verification misbehaviour checks to `CheckForMisbehaviour` ### Features diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle.go b/modules/light-clients/07-tendermint/misbehaviour_handle.go index 5b970de4584..76a0a9d9459 100644 --- a/modules/light-clients/07-tendermint/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/misbehaviour_handle.go @@ -51,9 +51,28 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode return true } case *Misbehaviour: - // The correctness of Misbehaviour ClientMessage types is ensured by calling VerifyClientMessage prior to this function - // Thus, here we can return true, as ClientMessage is of type Misbehaviour - return true + if msg.Header1.GetHeight().EQ(msg.Header2.GetHeight()) { + blockID1, err := tmtypes.BlockIDFromProto(&msg.Header1.SignedHeader.Commit.BlockID) + if err != nil { + return false + } + + blockID2, err := tmtypes.BlockIDFromProto(&msg.Header2.SignedHeader.Commit.BlockID) + if err != nil { + return false + } + + // Ensure that Commit Hashes are different + if !bytes.Equal(blockID1.Hash, blockID2.Hash) { + return true + } + + } else if !msg.Header1.SignedHeader.Header.Time.After(msg.Header2.SignedHeader.Header.Time) { + // Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to + // Header2 time in order to be valid misbehaviour (violation of monotonic time). + return true + } + return false } return false From 357970671b894cf170cb42d5bd2829c14a7e120a Mon Sep 17 00:00:00 2001 From: Charly Date: Mon, 23 Jan 2023 15:34:07 -0600 Subject: [PATCH 2/5] rm redundant return --- modules/light-clients/07-tendermint/misbehaviour_handle.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle.go b/modules/light-clients/07-tendermint/misbehaviour_handle.go index 76a0a9d9459..ae309d06486 100644 --- a/modules/light-clients/07-tendermint/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/misbehaviour_handle.go @@ -72,7 +72,6 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode // Header2 time in order to be valid misbehaviour (violation of monotonic time). return true } - return false } return false From 6d6a6eb93a68d86499e7d8959e23866d5317cee5 Mon Sep 17 00:00:00 2001 From: Charly Date: Mon, 23 Jan 2023 16:07:25 -0600 Subject: [PATCH 3/5] add test coverage --- .../07-tendermint/misbehaviour_handle.go | 24 ------- .../07-tendermint/misbehaviour_handle_test.go | 64 ------------------- .../07-tendermint/update_test.go | 45 +++++++++++++ 3 files changed, 45 insertions(+), 88 deletions(-) diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle.go b/modules/light-clients/07-tendermint/misbehaviour_handle.go index ae309d06486..40a7dff6442 100644 --- a/modules/light-clients/07-tendermint/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/misbehaviour_handle.go @@ -86,30 +86,6 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode // to misbehaviour.Header2 // Misbehaviour sets frozen height to {0, 1} since it is only used as a boolean value (zero or non-zero). func (cs *ClientState) verifyMisbehaviour(ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec, misbehaviour *Misbehaviour) error { - // if heights are equal check that this is valid misbehaviour of a fork - // otherwise if heights are unequal check that this is valid misbehavior of BFT time violation - if misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) { - blockID1, err := tmtypes.BlockIDFromProto(&misbehaviour.Header1.SignedHeader.Commit.BlockID) - if err != nil { - return sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour") - } - - blockID2, err := tmtypes.BlockIDFromProto(&misbehaviour.Header2.SignedHeader.Commit.BlockID) - if err != nil { - return sdkerrors.Wrap(err, "invalid block ID from header 2 in misbehaviour") - } - - // Ensure that Commit Hashes are different - if bytes.Equal(blockID1.Hash, blockID2.Hash) { - return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers block hashes are equal") - } - - } else if misbehaviour.Header1.SignedHeader.Header.Time.After(misbehaviour.Header2.SignedHeader.Header.Time) { - // Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to - // Header2 time in order to be valid misbehaviour (violation of monotonic time). - return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers are not at same height and are monotonically increasing") - } - // Regardless of the type of misbehaviour, ensure that both headers are valid and would have been accepted by light-client // Retrieve trusted consensus states for each Header in misbehaviour diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/misbehaviour_handle_test.go index 29f96e08a5d..2117571aa19 100644 --- a/modules/light-clients/07-tendermint/misbehaviour_handle_test.go +++ b/modules/light-clients/07-tendermint/misbehaviour_handle_test.go @@ -204,38 +204,6 @@ func (suite *TendermintTestSuite) TestVerifyMisbehaviour() { } }, true, }, - { - "invalid fork misbehaviour: identical headers", func() { - trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) - suite.Require().True(found) - - err := path.EndpointA.UpdateClient() - suite.Require().NoError(err) - - height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers) - misbehaviour = &ibctm.Misbehaviour{ - Header1: misbehaviourHeader, - Header2: misbehaviourHeader, - } - }, false, - }, - { - "invalid time misbehaviour: monotonically increasing time", func() { - trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) - suite.Require().True(found) - - misbehaviour = &ibctm.Misbehaviour{ - Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), - Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), - } - }, false, - }, { "invalid misbehaviour: misbehaviour from different chain", func() { trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) @@ -523,38 +491,6 @@ func (suite *TendermintTestSuite) TestVerifyMisbehaviourNonRevisionChainID() { } }, true, }, - { - "invalid fork misbehaviour: identical headers", func() { - trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) - suite.Require().True(found) - - err := path.EndpointA.UpdateClient() - suite.Require().NoError(err) - - height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers) - misbehaviour = &ibctm.Misbehaviour{ - Header1: misbehaviourHeader, - Header2: misbehaviourHeader, - } - }, false, - }, - { - "invalid time misbehaviour: monotonically increasing time", func() { - trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) - - trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) - suite.Require().True(found) - - misbehaviour = &ibctm.Misbehaviour{ - Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), - Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), - } - }, false, - }, { "invalid misbehaviour: misbehaviour from different chain", func() { trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index 77fe7090dcf..72541dd2b15 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -635,6 +635,38 @@ func (suite *TendermintTestSuite) TestCheckForMisbehaviour() { }, false, }, + { + "invalid fork misbehaviour: identical headers", func() { + trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) + + trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) + suite.Require().True(found) + + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) + + misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers) + clientMessage = &ibctm.Misbehaviour{ + Header1: misbehaviourHeader, + Header2: misbehaviourHeader, + } + }, false, + }, + { + "invalid time misbehaviour: monotonically increasing time", func() { + trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) + + trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) + suite.Require().True(found) + + clientMessage = &ibctm.Misbehaviour{ + Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), + Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), + } + }, false, + }, { "consensus state already exists, app hash mismatch", func() { @@ -705,6 +737,19 @@ func (suite *TendermintTestSuite) TestCheckForMisbehaviour() { }, true, }, + { + "valid time misbehaviour: not monotonically increasing time", func() { + trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) + + trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1) + suite.Require().True(found) + + clientMessage = &ibctm.Misbehaviour{ + Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), + Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers), + } + }, true, + }, } for _, tc := range testCases { From 57388901f096e3b52b35ac4a4a5e63ca4823739f Mon Sep 17 00:00:00 2001 From: Charly Date: Tue, 24 Jan 2023 16:54:08 -0600 Subject: [PATCH 4/5] pr review comments --- modules/light-clients/07-tendermint/misbehaviour_handle.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle.go b/modules/light-clients/07-tendermint/misbehaviour_handle.go index 40a7dff6442..6afbb531cb1 100644 --- a/modules/light-clients/07-tendermint/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/misbehaviour_handle.go @@ -15,6 +15,7 @@ import ( ) // CheckForMisbehaviour detects duplicate height misbehaviour and BFT time violation misbehaviour +// in a submitted Header message and verifies the correctness of a submitted Misbehaviour ClientMessage func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.ClientMessage) bool { switch msg := msg.(type) { case *Header: @@ -51,6 +52,8 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode return true } case *Misbehaviour: + // if heights are equal check that this is valid misbehaviour of a fork + // otherwise if heights are unequal check that this is valid misbehavior of BFT time violation if msg.Header1.GetHeight().EQ(msg.Header2.GetHeight()) { blockID1, err := tmtypes.BlockIDFromProto(&msg.Header1.SignedHeader.Commit.BlockID) if err != nil { From d2c27ab77ecdfcbc1550e4ed40446859f394f2a8 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 26 Jan 2023 21:15:44 +0100 Subject: [PATCH 5/5] add link to hyperlink --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e17075bc417..37f779bc1af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,7 +92,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#2434](https://github.com/cosmos/ibc-go/pull/2478) Removed all `TypeMsg` constants * (modules/core/exported) [#1689] (https://github.com/cosmos/ibc-go/pull/2539) Removing `GetVersions` from `ConnectionI` interface. * (core/02-connection) [#2419](https://github.com/cosmos/ibc-go/pull/2419) Add optional proof data to proto definitions of `MsgConnectionOpenTry` and `MsgConnectionOpenAck` for host state machines that are unable to introspect their own consensus state. -* (modules/light-clients/07-tendermint)[#2007] Moved non-verification misbehaviour checks to `CheckForMisbehaviour` +* (modules/light-clients/07-tendermint) [#2007](https://github.com/cosmos/ibc-go/pull/3046) Moved non-verification misbehaviour checks to `CheckForMisbehaviour` ### Features