From 993f5f27374d85c1264a22003c94b7b25a5d6b3a Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 19 Mar 2021 12:48:46 -0400
Subject: [PATCH 01/24] implement update client fix and start changing tests

---
 .../07-tendermint/types/update.go             |  25 ++++
 .../07-tendermint/types/update_test.go        | 121 ++++++++++--------
 2 files changed, 93 insertions(+), 53 deletions(-)

diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index f1183cdc042..8ea2b97c3c7 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -2,6 +2,7 @@ package types
 
 import (
 	"bytes"
+	"reflect"
 	"time"
 
 	"github.com/tendermint/tendermint/light"
@@ -48,6 +49,24 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 		)
 	}
 
+	// Check if the Client store already has a consensus state for the header's height
+	// If the consnensus state exists, and it matches the header then we return early
+	// since header has already been submitted in a previous UpdateClient.
+	// If the consensus state and header mismatch, then we validate new header and freeze
+	// the client if it is valid since this is proof of a fork on the counterparty chain.
+	var alreadyExists bool
+	prevConsState, err := GetConsensusState(clientStore, cdc, header.GetHeight())
+	if err != nil {
+		// This header has already been submitted and the necessary state is already stored
+		// in client store, thus we can return early without further validation.
+		if reflect.DeepEqual(prevConsState, tmHeader.ConsensusState()) {
+			return &cs, prevConsState, nil
+		}
+		// A consensus state already exists for this height, but it does not match the provided header.
+		// Thus, we must check that this header is valid, and if so we will freeze the client.
+		alreadyExists = true
+	}
+
 	// get consensus state from clientStore
 	tmConsState, err := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight)
 	if err != nil {
@@ -60,6 +79,12 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 		return nil, nil, err
 	}
 
+	// Header is different from existing consensus state and also valid, so we return a frozen client.
+	if alreadyExists {
+		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
+		return &cs, prevConsState, nil
+	}
+
 	newClientState, consensusState := update(ctx, clientStore, &cs, tmHeader)
 	return newClientState, consensusState, nil
 }
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index f72d6fba7a1..346e84e5ebe 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -1,6 +1,7 @@
 package types_test
 
 import (
+	"fmt"
 	"time"
 
 	tmtypes "github.com/tendermint/tendermint/types"
@@ -51,12 +52,12 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 
 	testCases := []struct {
 		name    string
-		setup   func()
+		setup   func(*TendermintTestSuite)
 		expPass bool
 	}{
 		{
 			name: "successful update with next height and same validator set",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -66,7 +67,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "successful update with future height and different validator set",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
@@ -76,7 +77,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "successful update with next height and different validator set",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), bothValSet.Hash())
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
@@ -86,7 +87,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "successful update for a previous height",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				consStateHeight = heightMinus3
@@ -97,7 +98,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "successful update for a previous revision",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision1, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
@@ -105,9 +106,19 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			},
 			expPass: true,
 		},
+		{
+			name: "successful update with identical header to a previous update",
+			setup: func(suite *TendermintTestSuite) {
+				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus3, suite.clientTime, suite.valSet, suite.valSet, signers)
+				currentTime = suite.now
+			},
+			expPass: true,
+		},
 		{
 			name: "unsuccessful update with incorrect header chain-id",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader("ethermint", int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -117,7 +128,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update to a future revision",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision0, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 1, height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -127,7 +138,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update: header height revision and trusted height revision mismatch",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision1, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(1, 1), commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 3, height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -137,7 +148,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches nextValSetHash",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
@@ -147,7 +158,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches different nextValSetHash",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), bothValSet.Hash())
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, bothValSet, signers)
@@ -157,7 +168,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update with future height: too much change in validator set",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, altValSet, suite.valSet, altSigners)
@@ -167,7 +178,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful updates, passed in incorrect trusted validators for given consensus state",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
@@ -177,7 +188,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update: trusting period has passed since last client timestamp",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -188,7 +199,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update: header timestamp is past current timestamp",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers)
@@ -198,7 +209,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "unsuccessful update: header timestamp is not past last client timestamp",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.clientTime, suite.valSet, suite.valSet, signers)
@@ -208,7 +219,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "header basic validation failed",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
@@ -220,7 +231,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		},
 		{
 			name: "header height < consensus height",
-			setup: func() {
+			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(height.RevisionNumber, heightPlus5.RevisionHeight), commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				// Make new header at height less than latest client state
@@ -233,49 +244,53 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 
 	for i, tc := range testCases {
 		tc := tc
+		suite.Run(fmt.Sprintf("Case: %s", tc.name), func() {
+			// reset suite to create fresh application state
+			suite.SetupTest()
 
-		consStateHeight = height // must be explicitly changed
-		// setup test
-		tc.setup()
+			consStateHeight = height // must be explicitly changed
+			// setup test
+			tc.setup(suite)
 
-		// Set current timestamp in context
-		ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
+			// Set current timestamp in context
+			ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
 
-		// Set trusted consensus state in client store
-		suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, consStateHeight, consensusState)
+			// Set trusted consensus state in client store
+			suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, consStateHeight, consensusState)
 
-		height := newHeader.GetHeight()
-		expectedConsensus := &types.ConsensusState{
-			Timestamp:          newHeader.GetTime(),
-			Root:               commitmenttypes.NewMerkleRoot(newHeader.Header.GetAppHash()),
-			NextValidatorsHash: newHeader.Header.NextValidatorsHash,
-		}
+			height := newHeader.GetHeight()
+			expectedConsensus := &types.ConsensusState{
+				Timestamp:          newHeader.GetTime(),
+				Root:               commitmenttypes.NewMerkleRoot(newHeader.Header.GetAppHash()),
+				NextValidatorsHash: newHeader.Header.NextValidatorsHash,
+			}
 
-		newClientState, consensusState, err := clientState.CheckHeaderAndUpdateState(
-			ctx,
-			suite.cdc,
-			suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientID), // pass in clientID prefixed clientStore
-			newHeader,
-		)
+			newClientState, consensusState, err := clientState.CheckHeaderAndUpdateState(
+				ctx,
+				suite.cdc,
+				suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientID), // pass in clientID prefixed clientStore
+				newHeader,
+			)
 
-		if tc.expPass {
-			suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
+			if tc.expPass {
+				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 
-			// Determine if clientState should be updated or not
-			// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
-			if height.GT(clientState.LatestHeight) {
-				// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
-				suite.Require().Equal(height, newClientState.GetLatestHeight(), "clientstate height did not update")
+				// Determine if clientState should be updated or not
+				// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
+				if height.GT(clientState.LatestHeight) {
+					// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
+					suite.Require().Equal(height, newClientState.GetLatestHeight(), "clientstate height did not update")
+				} else {
+					// Update will add past consensus state, clientState should not be updated at all
+					suite.Require().Equal(clientState.LatestHeight, newClientState.GetLatestHeight(), "client state height updated for past header")
+				}
+
+				suite.Require().Equal(expectedConsensus, consensusState, "valid test case %d failed: %s", i, tc.name)
 			} else {
-				// Update will add past consensus state, clientState should not be updated at all
-				suite.Require().Equal(clientState.LatestHeight, newClientState.GetLatestHeight(), "client state height updated for past header")
+				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
+				suite.Require().Nil(newClientState, "invalid test case %d passed: %s", i, tc.name)
+				suite.Require().Nil(consensusState, "invalid test case %d passed: %s", i, tc.name)
 			}
-
-			suite.Require().Equal(expectedConsensus, consensusState, "valid test case %d failed: %s", i, tc.name)
-		} else {
-			suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
-			suite.Require().Nil(newClientState, "invalid test case %d passed: %s", i, tc.name)
-			suite.Require().Nil(consensusState, "invalid test case %d passed: %s", i, tc.name)
-		}
+		})
 	}
 }

From 1ef071c8461491fc80d866c231fe519926287749 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 19 Mar 2021 13:35:46 -0400
Subject: [PATCH 02/24] fix bug and write identical case test

---
 .../07-tendermint/types/tendermint_test.go    | 15 +++++++-
 .../07-tendermint/types/update.go             |  4 +-
 .../07-tendermint/types/update_test.go        | 37 ++++++++++++-------
 3 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/modules/light-clients/07-tendermint/types/tendermint_test.go b/modules/light-clients/07-tendermint/types/tendermint_test.go
index b42f564c49e..2db60c4a3cc 100644
--- a/modules/light-clients/07-tendermint/types/tendermint_test.go
+++ b/modules/light-clients/07-tendermint/types/tendermint_test.go
@@ -10,12 +10,12 @@ import (
 	tmtypes "github.com/tendermint/tendermint/types"
 
 	"github.com/cosmos/cosmos-sdk/codec"
-	"github.com/cosmos/ibc-go/testing/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
 	ibctmtypes "github.com/cosmos/ibc-go/modules/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/ibc-go/testing"
 	ibctestingmock "github.com/cosmos/ibc-go/testing/mock"
+	"github.com/cosmos/ibc-go/testing/simapp"
 )
 
 const (
@@ -90,6 +90,19 @@ func (suite *TendermintTestSuite) SetupTest() {
 	suite.ctx = app.BaseApp.NewContext(checkTx, tmproto.Header{Height: 1, Time: suite.now})
 }
 
+func getSuiteSigners(suite *TendermintTestSuite) []tmtypes.PrivValidator {
+	return []tmtypes.PrivValidator{suite.privVal}
+}
+
+func getBothSigners(suite *TendermintTestSuite, altVal *tmtypes.Validator, altPrivVal tmtypes.PrivValidator) (*tmtypes.ValidatorSet, []tmtypes.PrivValidator) {
+	// Create bothValSet with both suite validator and altVal. Would be valid update
+	bothValSet := tmtypes.NewValidatorSet(append(suite.valSet.Validators, altVal))
+	// Create signer array and ensure it is in same order as bothValSet
+	_, suiteVal := suite.valSet.GetByIndex(0)
+	bothSigners := ibctesting.CreateSortedSignerArray(altPrivVal, suite.privVal, altVal, suiteVal)
+	return bothValSet, bothSigners
+}
+
 func TestTendermintTestSuite(t *testing.T) {
 	suite.Run(t, new(TendermintTestSuite))
 }
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 8ea2b97c3c7..129a7d49d58 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -55,8 +55,8 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// If the consensus state and header mismatch, then we validate new header and freeze
 	// the client if it is valid since this is proof of a fork on the counterparty chain.
 	var alreadyExists bool
-	prevConsState, err := GetConsensusState(clientStore, cdc, header.GetHeight())
-	if err != nil {
+	prevConsState, _ := GetConsensusState(clientStore, cdc, header.GetHeight())
+	if prevConsState != nil {
 		// This header has already been submitted and the necessary state is already stored
 		// in client store, thus we can return early without further validation.
 		if reflect.DeepEqual(prevConsState, tmHeader.ConsensusState()) {
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 346e84e5ebe..43f08b78b49 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -9,7 +9,6 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
 	commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types"
 	types "github.com/cosmos/ibc-go/modules/light-clients/07-tendermint/types"
-	ibctesting "github.com/cosmos/ibc-go/testing"
 	ibctestingmock "github.com/cosmos/ibc-go/testing/mock"
 )
 
@@ -36,18 +35,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 	heightPlus5 := clienttypes.NewHeight(height.RevisionNumber, height.RevisionHeight+5)
 
 	altVal := tmtypes.NewValidator(altPubKey, revisionHeight)
-
-	// Create bothValSet with both suite validator and altVal. Would be valid update
-	bothValSet := tmtypes.NewValidatorSet(append(suite.valSet.Validators, altVal))
 	// Create alternative validator set with only altVal, invalid update (too much change in valSet)
 	altValSet := tmtypes.NewValidatorSet([]*tmtypes.Validator{altVal})
-
-	signers := []tmtypes.PrivValidator{suite.privVal}
-
-	// Create signer array and ensure it is in same order as bothValSet
-	_, suiteVal := suite.valSet.GetByIndex(0)
-	bothSigners := ibctesting.CreateSortedSignerArray(altPrivVal, suite.privVal, altVal, suiteVal)
-
 	altSigners := []tmtypes.PrivValidator{altPrivVal}
 
 	testCases := []struct {
@@ -60,6 +49,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -70,6 +60,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
@@ -79,6 +70,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			name: "successful update with next height and different validator set",
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), bothValSet.Hash())
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
@@ -91,6 +83,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				consStateHeight = heightMinus3
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
@@ -101,6 +94,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision1, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				consStateHeight = heightMinus3
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
@@ -109,10 +104,14 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 		{
 			name: "successful update with identical header to a previous update",
 			setup: func(suite *TendermintTestSuite) {
-				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, heightPlus1, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
-				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus3, suite.clientTime, suite.valSet, suite.valSet, signers)
+				signers := getSuiteSigners(suite)
+				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
+				ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
+				// Store the header's consensus state in client store before UpdateClient call
+				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, newHeader.ConsensusState())
 			},
 			expPass: true,
 		},
@@ -121,6 +120,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader("ethermint", int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -131,6 +131,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision0, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 1, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -141,6 +142,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainIDRevision1, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(1, 1), commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 3, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -151,6 +153,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
@@ -160,6 +163,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			name: "unsuccessful update with next height: update header mismatches different nextValSetHash",
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				bothValSet, _ := getBothSigners(suite, altVal, altPrivVal)
+				signers := getSuiteSigners(suite)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), bothValSet.Hash())
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, bothValSet, signers)
 				currentTime = suite.now
@@ -181,6 +186,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				bothValSet, bothSigners := getBothSigners(suite, altVal, altPrivVal)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
@@ -191,6 +197,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				// make current time pass trusting period from last timestamp on clientstate
 				currentTime = suite.now.Add(trustingPeriod)
@@ -202,6 +209,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -212,6 +220,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.clientTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
@@ -222,6 +231,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				// cause new header to fail validatebasic by changing commit height to mismatch header height
 				newHeader.SignedHeader.Commit.Height = revisionHeight - 1
@@ -234,6 +244,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(height.RevisionNumber, heightPlus5.RevisionHeight), commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
 				// Make new header at height less than latest client state
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now

From d432451b06b44c0a32606efa74bf97f5ae5754a9 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 19 Mar 2021 14:02:22 -0400
Subject: [PATCH 03/24] write misbehaviour detection tests

---
 .../07-tendermint/types/update.go             |  2 +-
 .../07-tendermint/types/update_test.go        | 83 ++++++++++++++-----
 2 files changed, 63 insertions(+), 22 deletions(-)

diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 129a7d49d58..48fa2645d06 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -82,7 +82,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// Header is different from existing consensus state and also valid, so we return a frozen client.
 	if alreadyExists {
 		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
-		return &cs, prevConsState, nil
+		return &cs, tmHeader.ConsensusState(), nil
 	}
 
 	newClientState, consensusState := update(ctx, clientStore, &cs, tmHeader)
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 43f08b78b49..519b43279a1 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -40,9 +40,10 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 	altSigners := []tmtypes.PrivValidator{altPrivVal}
 
 	testCases := []struct {
-		name    string
-		setup   func(*TendermintTestSuite)
-		expPass bool
+		name      string
+		setup     func(*TendermintTestSuite)
+		expPass   bool
+		expFrozen bool
 	}{
 		{
 			name: "successful update with next height and same validator set",
@@ -53,7 +54,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
 		},
 		{
 			name: "successful update with future height and different validator set",
@@ -64,7 +66,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
 		},
 		{
 			name: "successful update with next height and different validator set",
@@ -75,7 +78,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
 		},
 		{
 			name: "successful update for a previous height",
@@ -87,7 +91,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
 		},
 		{
 			name: "successful update for a previous revision",
@@ -99,7 +104,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
 		},
 		{
 			name: "successful update with identical header to a previous update",
@@ -113,7 +119,25 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// Store the header's consensus state in client store before UpdateClient call
 				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, newHeader.ConsensusState())
 			},
-			expPass: true,
+			expPass:   true,
+			expFrozen: false,
+		},
+		{
+			name: "misbehaviour detection: header conflicts with existing consensus state",
+			setup: func(suite *TendermintTestSuite) {
+				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, heightPlus1, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				signers := getSuiteSigners(suite)
+				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
+				currentTime = suite.now
+				ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
+				// Change the consensus state of header and store in client store to create a conflict
+				conflictConsState := newHeader.ConsensusState()
+				conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
+				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, conflictConsState)
+			},
+			expPass:   true,
+			expFrozen: true,
 		},
 		{
 			name: "unsuccessful update with incorrect header chain-id",
@@ -124,7 +148,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader("ethermint", int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update to a future revision",
@@ -135,7 +160,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 1, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update: header height revision and trusted height revision mismatch",
@@ -146,7 +172,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 3, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches nextValSetHash",
@@ -157,7 +184,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches different nextValSetHash",
@@ -169,7 +197,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, bothValSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update with future height: too much change in validator set",
@@ -179,7 +208,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, altValSet, suite.valSet, altSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful updates, passed in incorrect trusted validators for given consensus state",
@@ -190,7 +220,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update: trusting period has passed since last client timestamp",
@@ -202,7 +233,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// make current time pass trusting period from last timestamp on clientstate
 				currentTime = suite.now.Add(trustingPeriod)
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is past current timestamp",
@@ -213,7 +245,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is not past last client timestamp",
@@ -224,7 +257,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.clientTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "header basic validation failed",
@@ -237,7 +271,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader.SignedHeader.Commit.Height = revisionHeight - 1
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 		{
 			name: "header height < consensus height",
@@ -249,7 +284,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expPass:   false,
+			expFrozen: false,
 		},
 	}
 
@@ -286,6 +322,11 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			if tc.expPass {
 				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 
+				if tc.expFrozen {
+					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
+					suite.Require().Equal(newClientState.(*types.ClientState).FrozenHeight, newHeader.GetHeight(), "client frozen at wrong height")
+				}
+
 				// Determine if clientState should be updated or not
 				// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
 				if height.GT(clientState.LatestHeight) {

From 593e5a5d8075f5e2125661909d3b9d122f09ed1c Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 19 Mar 2021 15:23:05 -0400
Subject: [PATCH 04/24] add misbehaviour events to UpdateClient

---
 modules/core/02-client/keeper/client.go | 37 +++++++++++++++++--------
 modules/core/02-client/types/events.go  | 12 ++++----
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index e8288da5f61..f4fd0d73161 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -73,6 +73,32 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	k.SetClientState(ctx, clientID, clientState)
 
+	// emit the full header in events
+	var headerStr string
+	if header != nil {
+		// Marshal the Header as an Any and encode the resulting bytes to hex.
+		// This prevents the event value from containing invalid UTF-8 characters
+		// which may cause data to be lost when JSON encoding/decoding.
+		headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))
+
+	}
+
+	// CheckHeaderAndUpdateState may detect misbehaviour and freeze the client. Thus, we check here if new client state
+	// is frozen. If so, emit the appropriate events and return.
+	if clientState.IsFrozen() {
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypeSubmitMisbehaviour,
+				sdk.NewAttribute(types.AttributeKeyClientID, clientID),
+				sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
+				sdk.NewAttribute(types.AttributeKeyConsensusHeight, header.GetHeight().String()),
+				sdk.NewAttribute(types.AttributeKeyMisbehaviourHeader1, headerStr),
+			),
+		)
+		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
+
+	}
+
 	var consensusHeight exported.Height
 
 	// we don't set consensus state for localhost client
@@ -96,17 +122,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 			},
 		)
 	}()
-
-	// emit the full header in events
-	var headerStr string
-	if header != nil {
-		// Marshal the Header as an Any and encode the resulting bytes to hex.
-		// This prevents the event value from containing invalid UTF-8 characters
-		// which may cause data to be lost when JSON encoding/decoding.
-		headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))
-
-	}
-
 	// emitting events in the keeper emits for both begin block and handler client updates
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
diff --git a/modules/core/02-client/types/events.go b/modules/core/02-client/types/events.go
index 464ad4d426e..1c3988ba276 100644
--- a/modules/core/02-client/types/events.go
+++ b/modules/core/02-client/types/events.go
@@ -8,11 +8,13 @@ import (
 
 // IBC client events
 const (
-	AttributeKeyClientID        = "client_id"
-	AttributeKeySubjectClientID = "subject_client_id"
-	AttributeKeyClientType      = "client_type"
-	AttributeKeyConsensusHeight = "consensus_height"
-	AttributeKeyHeader          = "header"
+	AttributeKeyClientID            = "client_id"
+	AttributeKeySubjectClientID     = "subject_client_id"
+	AttributeKeyClientType          = "client_type"
+	AttributeKeyConsensusHeight     = "consensus_height"
+	AttributeKeyHeader              = "header"
+	AttributeKeyMisbehaviourHeader1 = "misbehaviour_header1"
+	AttributeKeyMisbehaviourHeader2 = "misbehaviour_header2"
 )
 
 // IBC client events vars

From 38f6115c02b8eaab2493a8fb39cd6309cbac4c6f Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 19 Mar 2021 16:53:59 -0400
Subject: [PATCH 05/24] fix client keeper and write tests

---
 modules/core/02-client/keeper/client.go       |   7 +-
 modules/core/02-client/keeper/client_test.go  | 134 +++++++++++++++---
 .../07-tendermint/types/update_test.go        |   2 +-
 3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index f4fd0d73161..6629aa5f018 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -80,12 +80,11 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		// This prevents the event value from containing invalid UTF-8 characters
 		// which may cause data to be lost when JSON encoding/decoding.
 		headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))
-
 	}
 
 	// CheckHeaderAndUpdateState may detect misbehaviour and freeze the client. Thus, we check here if new client state
-	// is frozen. If so, emit the appropriate events and return.
-	if clientState.IsFrozen() {
+	// was frozen by this update. If so, emit the appropriate events and return.
+	if clientState.IsFrozen() && clientState.GetFrozenHeight().EQ(header.GetHeight()) {
 		ctx.EventManager().EmitEvent(
 			sdk.NewEvent(
 				types.EventTypeSubmitMisbehaviour,
@@ -97,6 +96,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		)
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
 
+		return nil
 	}
 
 	var consensusHeight exported.Height
@@ -122,6 +122,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 			},
 		)
 	}()
+
 	// emitting events in the keeper emits for both begin block and handler client updates
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index 21002d175b0..c2cfcee1c99 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -7,6 +7,7 @@ import (
 
 	tmtypes "github.com/tendermint/tendermint/types"
 
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 	"github.com/cosmos/ibc-go/modules/core/02-client/types"
 	clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
 	commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types"
@@ -15,7 +16,6 @@ import (
 	localhosttypes "github.com/cosmos/ibc-go/modules/light-clients/09-localhost/types"
 	ibctesting "github.com/cosmos/ibc-go/testing"
 	ibctestingmock "github.com/cosmos/ibc-go/testing/mock"
-	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 func (suite *KeeperTestSuite) TestCreateClient() {
@@ -68,6 +68,7 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 		name     string
 		malleate func() error
 		expPass  bool
+		freeze   bool // only true if update freezes the client to a new frozen height. false if client is already frozen and header is valid past update
 	}{
 		{"valid update", func() error {
 			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
@@ -86,7 +87,7 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 
 			updateHeader = createFutureUpdateFn(suite)
 			return err
-		}, true},
+		}, true, false},
 		{"valid past update", func() error {
 			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
 			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
@@ -114,26 +115,69 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			// clientState should not be updated
 			updateHeader = createPastUpdateFn(suite)
 			return nil
-		}, true},
-		{"client state not found", func() error {
-			updateHeader = createFutureUpdateFn(suite)
+		}, true, false},
+		{"valid duplicate update", func() error {
+			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+			suite.Require().NoError(err)
+
+			height1 := types.NewHeight(0, 1)
+
+			// store previous consensus state
+			prevConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.past,
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
+
+			height2 := types.NewHeight(0, 2)
+
+			// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
+			intermediateConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.past.Add(time.Minute),
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
 
+			// updateHeader will fill in consensus state between prevConsState and suite.consState
+			// clientState should not be updated
+			updateHeader = createPastUpdateFn(suite)
+			// set updateHeader's consensus state in store to create duplicate UpdateClient scenario
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), updateHeader.ConsensusState())
 			return nil
-		}, false},
-		{"consensus state not found", func() error {
+		}, true, false},
+		{"misbehaviour detection: conflicting header", func() error {
 			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
-			updateHeader = createFutureUpdateFn(suite)
+			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+			suite.Require().NoError(err)
 
-			return nil
-		}, false},
-		{"frozen client before update", func() error {
-			clientState = &ibctmtypes.ClientState{FrozenHeight: types.NewHeight(0, 1), LatestHeight: testClientHeight}
-			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
-			updateHeader = createFutureUpdateFn(suite)
+			height1 := types.NewHeight(0, 1)
+
+			// store previous consensus state
+			prevConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.past,
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
+
+			height2 := types.NewHeight(0, 2)
 
+			// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
+			intermediateConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.past.Add(time.Minute),
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
+
+			// updateHeader will fill in consensus state between prevConsState and suite.consState
+			// clientState should not be updated
+			updateHeader = createPastUpdateFn(suite)
+			// set conflicting consensus state in store to create misbehaviour scenario
+			conflictConsState := updateHeader.ConsensusState()
+			conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
 			return nil
-		}, false},
+		}, true, true},
 		{"valid past update before client was frozen", func() error {
 			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
 			clientState.FrozenHeight = types.NewHeight(0, testClientHeight.RevisionHeight-1)
@@ -153,7 +197,50 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			// clientState should not be updated
 			updateHeader = createPastUpdateFn(suite)
 			return nil
-		}, true},
+		}, true, false},
+		{"misbehaviour detection: conflicting header before client was frozen", func() error {
+			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+			clientState.FrozenHeight = types.NewHeight(0, testClientHeight.RevisionHeight-1)
+			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+			suite.Require().NoError(err)
+
+			height1 := types.NewHeight(0, 1)
+
+			// store previous consensus state
+			prevConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.past,
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
+
+			// updateHeader will fill in consensus state between prevConsState and suite.consState
+			// clientState should not be updated
+			updateHeader = createPastUpdateFn(suite)
+			// set conflicting consensus state in store to create misbehaviour scenario
+			conflictConsState := updateHeader.ConsensusState()
+			conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
+			return nil
+		}, true, true},
+		{"client state not found", func() error {
+			updateHeader = createFutureUpdateFn(suite)
+
+			return nil
+		}, false, false},
+		{"consensus state not found", func() error {
+			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
+			updateHeader = createFutureUpdateFn(suite)
+
+			return nil
+		}, false, false},
+		{"frozen client before update", func() error {
+			clientState = &ibctmtypes.ClientState{FrozenHeight: types.NewHeight(0, 1), LatestHeight: testClientHeight}
+			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
+			updateHeader = createFutureUpdateFn(suite)
+
+			return nil
+		}, false, false},
 		{"invalid header", func() error {
 			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
 			_, err := suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
@@ -161,7 +248,7 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			updateHeader = createPastUpdateFn(suite)
 
 			return nil
-		}, false},
+		}, false, false},
 	}
 
 	for i, tc := range cases {
@@ -202,8 +289,15 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 					suite.Require().Equal(clientState.GetLatestHeight(), newClientState.GetLatestHeight(), "client state height updated for past header")
 				}
 
-				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
-				suite.Require().Equal(expConsensusState, consensusState, "consensus state should have been updated on case %s", tc.name)
+				// If the update freezes the client, check that client was frozen to update header's height.
+				// Otherwise check that consensus state is stored as expected.
+				if tc.freeze {
+					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
+					suite.Require().Equal(newClientState.GetFrozenHeight(), updateHeader.GetHeight(), "client frozen at wrong height")
+				} else {
+					suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
+					suite.Require().Equal(expConsensusState, consensusState, "consensus state should have been updated on case %s", tc.name)
+				}
 			} else {
 				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
 			}
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 519b43279a1..3fbb9e3956b 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -324,7 +324,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 
 				if tc.expFrozen {
 					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
-					suite.Require().Equal(newClientState.(*types.ClientState).FrozenHeight, newHeader.GetHeight(), "client frozen at wrong height")
+					suite.Require().Equal(newClientState.GetFrozenHeight(), newHeader.GetHeight(), "client frozen at wrong height")
 				}
 
 				// Determine if clientState should be updated or not

From 4a56b57fd5bc6e9bfdf9059e5e21859e626c6435 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Thu, 25 Mar 2021 16:14:37 -0400
Subject: [PATCH 06/24] add Freeze to ClientState interface

---
 modules/core/02-client/keeper/client.go       | 69 +++++++++++--------
 modules/core/exported/client.go               |  1 +
 .../06-solomachine/types/client_state.go      |  6 ++
 .../07-tendermint/types/client_state.go       |  6 ++
 .../07-tendermint/types/update.go             |  4 +-
 .../07-tendermint/types/update_test.go        | 69 ++++++-------------
 .../09-localhost/types/client_state.go        |  5 ++
 7 files changed, 81 insertions(+), 79 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 6629aa5f018..edf0544a4b7 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"encoding/hex"
+	"reflect"
 
 	"github.com/armon/go-metrics"
 
@@ -66,13 +67,19 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
+	var (
+		existingConsState exported.ConsensusState
+		exists            bool
+	)
+	if header != nil {
+		existingConsState, exists = k.GetClientConsensusState(ctx, clientID, header.GetHeight())
+	}
+
 	clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
 	}
 
-	k.SetClientState(ctx, clientID, clientState)
-
 	// emit the full header in events
 	var headerStr string
 	if header != nil {
@@ -82,9 +89,13 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))
 	}
 
-	// CheckHeaderAndUpdateState may detect misbehaviour and freeze the client. Thus, we check here if new client state
-	// was frozen by this update. If so, emit the appropriate events and return.
-	if clientState.IsFrozen() && clientState.GetFrozenHeight().EQ(header.GetHeight()) {
+	// if there already exists a consensus state in client store for the header height
+	// and it does not match the updated consensus state, this is evidence of misbehaviour
+	// and we must freeze the client and emit appropriate events.
+	if exists && !reflect.DeepEqual(existingConsState, consensusState) {
+		clientState.Freeze(header)
+		k.SetClientState(ctx, clientID, clientState)
+
 		ctx.EventManager().EmitEvent(
 			sdk.NewEvent(
 				types.EventTypeSubmitMisbehaviour,
@@ -95,22 +106,31 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 			),
 		)
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
-
-		return nil
-	}
-
-	var consensusHeight exported.Height
-
-	// we don't set consensus state for localhost client
-	if header != nil && clientID != exported.Localhost {
-		k.SetClientConsensusState(ctx, clientID, header.GetHeight(), consensusState)
-		consensusHeight = header.GetHeight()
-	} else {
-		consensusHeight = types.GetSelfHeight(ctx)
+	} else if !exists {
+		// if update is not misbehaviour then update the consensus state
+		// we don't set consensus state for localhost client
+		var consensusHeight exported.Height
+		if header != nil && clientID != exported.Localhost {
+			k.SetClientConsensusState(ctx, clientID, header.GetHeight(), consensusState)
+			consensusHeight = header.GetHeight()
+		} else {
+			consensusHeight = types.GetSelfHeight(ctx)
+		}
+		k.SetClientState(ctx, clientID, clientState)
+
+		// emitting events in the keeper emits for both begin block and handler client updates
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypeUpdateClient,
+				sdk.NewAttribute(types.AttributeKeyClientID, clientID),
+				sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
+				sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+				sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
+			),
+		)
+		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
 	}
 
-	k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
-
 	defer func() {
 		telemetry.IncrCounterWithLabels(
 			[]string{"ibc", "client", "update"},
@@ -123,17 +143,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		)
 	}()
 
-	// emitting events in the keeper emits for both begin block and handler client updates
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypeUpdateClient,
-			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
-			sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
-			sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
-		),
-	)
-
 	return nil
 }
 
diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go
index 3d552b07724..f9ab322eef7 100644
--- a/modules/core/exported/client.go
+++ b/modules/core/exported/client.go
@@ -31,6 +31,7 @@ type ClientState interface {
 	GetLatestHeight() Height
 	IsFrozen() bool
 	GetFrozenHeight() Height
+	Freeze(Header)
 	Validate() error
 	GetProofSpecs() []*ics23.ProofSpec
 
diff --git a/modules/light-clients/06-solomachine/types/client_state.go b/modules/light-clients/06-solomachine/types/client_state.go
index d008ac81be9..7f7968ab173 100644
--- a/modules/light-clients/06-solomachine/types/client_state.go
+++ b/modules/light-clients/06-solomachine/types/client_state.go
@@ -52,6 +52,12 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return clienttypes.NewHeight(0, cs.FrozenSequence)
 }
 
+// Freeze takes a misbehaving header and freezes the client at the header height
+// NOTE: Header must be verified as misbehaviour before calling this function.
+func (cs *ClientState) Freeze(header exported.Header) {
+	cs.FrozenSequence = header.GetHeight().GetRevisionHeight()
+}
+
 // GetProofSpecs returns nil proof specs since client state verification uses signatures.
 func (cs ClientState) GetProofSpecs() []*ics23.ProofSpec {
 	return nil
diff --git a/modules/light-clients/07-tendermint/types/client_state.go b/modules/light-clients/07-tendermint/types/client_state.go
index 8a21ef9a679..f06f1e019d5 100644
--- a/modules/light-clients/07-tendermint/types/client_state.go
+++ b/modules/light-clients/07-tendermint/types/client_state.go
@@ -68,6 +68,12 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return cs.FrozenHeight
 }
 
+// Freeze takes a misbehaving header and freezes the client at the header height
+// NOTE: Header must be verified as misbehaviour before calling this function.
+func (cs *ClientState) Freeze(header exported.Header) {
+	cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
+}
+
 // IsExpired returns whether or not the client has passed the trusting period since the last
 // update (in which case no headers are considered valid).
 func (cs ClientState) IsExpired(latestTimestamp, now time.Time) bool {
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 48fa2645d06..558ed1a0885 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -79,9 +79,9 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 		return nil, nil, err
 	}
 
-	// Header is different from existing consensus state and also valid, so we return a frozen client.
+	// Header is different from existing consensus state and also valid, so return the consensus state of the header.
+	// ClientKeeper must freeze the client based on the header.
 	if alreadyExists {
-		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
 		return &cs, tmHeader.ConsensusState(), nil
 	}
 
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 3fbb9e3956b..07403e08205 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -40,10 +40,9 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 	altSigners := []tmtypes.PrivValidator{altPrivVal}
 
 	testCases := []struct {
-		name      string
-		setup     func(*TendermintTestSuite)
-		expPass   bool
-		expFrozen bool
+		name    string
+		setup   func(*TendermintTestSuite)
+		expPass bool
 	}{
 		{
 			name: "successful update with next height and same validator set",
@@ -54,8 +53,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "successful update with future height and different validator set",
@@ -66,8 +64,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "successful update with next height and different validator set",
@@ -78,8 +75,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "successful update for a previous height",
@@ -91,8 +87,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "successful update for a previous revision",
@@ -104,8 +99,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "successful update with identical header to a previous update",
@@ -119,8 +113,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// Store the header's consensus state in client store before UpdateClient call
 				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, newHeader.ConsensusState())
 			},
-			expPass:   true,
-			expFrozen: false,
+			expPass: true,
 		},
 		{
 			name: "misbehaviour detection: header conflicts with existing consensus state",
@@ -136,8 +129,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
 				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, conflictConsState)
 			},
-			expPass:   true,
-			expFrozen: true,
+			expPass: true,
 		},
 		{
 			name: "unsuccessful update with incorrect header chain-id",
@@ -148,8 +140,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader("ethermint", int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update to a future revision",
@@ -160,8 +151,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 1, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update: header height revision and trusted height revision mismatch",
@@ -172,8 +162,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 3, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches nextValSetHash",
@@ -184,8 +173,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches different nextValSetHash",
@@ -197,8 +185,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, bothValSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update with future height: too much change in validator set",
@@ -208,8 +195,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, altValSet, suite.valSet, altSigners)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful updates, passed in incorrect trusted validators for given consensus state",
@@ -220,8 +206,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update: trusting period has passed since last client timestamp",
@@ -233,8 +218,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// make current time pass trusting period from last timestamp on clientstate
 				currentTime = suite.now.Add(trustingPeriod)
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is past current timestamp",
@@ -245,8 +229,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is not past last client timestamp",
@@ -257,8 +240,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.clientTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "header basic validation failed",
@@ -271,8 +253,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader.SignedHeader.Commit.Height = revisionHeight - 1
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 		{
 			name: "header height < consensus height",
@@ -284,8 +265,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass:   false,
-			expFrozen: false,
+			expPass: false,
 		},
 	}
 
@@ -322,11 +302,6 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			if tc.expPass {
 				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 
-				if tc.expFrozen {
-					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
-					suite.Require().Equal(newClientState.GetFrozenHeight(), newHeader.GetHeight(), "client frozen at wrong height")
-				}
-
 				// Determine if clientState should be updated or not
 				// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
 				if height.GT(clientState.LatestHeight) {
diff --git a/modules/light-clients/09-localhost/types/client_state.go b/modules/light-clients/09-localhost/types/client_state.go
index 6336a213ffc..ef0e2368731 100644
--- a/modules/light-clients/09-localhost/types/client_state.go
+++ b/modules/light-clients/09-localhost/types/client_state.go
@@ -53,6 +53,11 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return clienttypes.ZeroHeight()
 }
 
+// Freeze is a no-op for localhost clients
+func (cs *ClientState) Freeze(exported.Header) {
+	return
+}
+
 // Validate performs a basic validation of the client state fields.
 func (cs ClientState) Validate() error {
 	if strings.TrimSpace(cs.ChainId) == "" {

From a29d312b3d4cfcaf570b0d1185adaa18236d7f2d Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 26 Mar 2021 15:18:42 -0400
Subject: [PATCH 07/24] add cache context and fix events

---
 modules/core/02-client/keeper/client.go | 56 ++++++++++++++-----------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index edf0544a4b7..3bbf8fc08af 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -70,12 +70,15 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 	var (
 		existingConsState exported.ConsensusState
 		exists            bool
+		consensusHeight   exported.Height
+		eventType         string
 	)
 	if header != nil {
 		existingConsState, exists = k.GetClientConsensusState(ctx, clientID, header.GetHeight())
 	}
 
-	clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
+	cacheCtx, writeFn := ctx.CacheContext()
+	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(cacheCtx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
 	}
@@ -87,50 +90,53 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		// This prevents the event value from containing invalid UTF-8 characters
 		// which may cause data to be lost when JSON encoding/decoding.
 		headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))
+		// set default consensus height with header height
+		consensusHeight = header.GetHeight()
+
 	}
 
 	// if there already exists a consensus state in client store for the header height
 	// and it does not match the updated consensus state, this is evidence of misbehaviour
 	// and we must freeze the client and emit appropriate events.
-	if exists && !reflect.DeepEqual(existingConsState, consensusState) {
+	if exists && !reflect.DeepEqual(existingConsState, newConsensusState) {
 		clientState.Freeze(header)
 		k.SetClientState(ctx, clientID, clientState)
 
-		ctx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				types.EventTypeSubmitMisbehaviour,
-				sdk.NewAttribute(types.AttributeKeyClientID, clientID),
-				sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
-				sdk.NewAttribute(types.AttributeKeyConsensusHeight, header.GetHeight().String()),
-				sdk.NewAttribute(types.AttributeKeyMisbehaviourHeader1, headerStr),
-			),
-		)
+		// set consensus height and set eventType to SubmitMisbehaviour
+		eventType = types.EventTypeSubmitMisbehaviour
+
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
 	} else if !exists {
+		// write any cached state changes from CheckHeaderAndUpdateState
+		// to store metadata in client store for new consensus state.
+		writeFn()
+
 		// if update is not misbehaviour then update the consensus state
 		// we don't set consensus state for localhost client
-		var consensusHeight exported.Height
 		if header != nil && clientID != exported.Localhost {
-			k.SetClientConsensusState(ctx, clientID, header.GetHeight(), consensusState)
-			consensusHeight = header.GetHeight()
+			k.SetClientConsensusState(ctx, clientID, header.GetHeight(), newConsensusState)
 		} else {
 			consensusHeight = types.GetSelfHeight(ctx)
 		}
-		k.SetClientState(ctx, clientID, clientState)
+		k.SetClientState(ctx, clientID, newClientState)
+
+		// set eventType to UpdateClient
+		eventType = types.EventTypeUpdateClient
 
-		// emitting events in the keeper emits for both begin block and handler client updates
-		ctx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				types.EventTypeUpdateClient,
-				sdk.NewAttribute(types.AttributeKeyClientID, clientID),
-				sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
-				sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
-				sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
-			),
-		)
 		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
 	}
 
+	// emitting events in the keeper emits for both begin block and handler client updates
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			eventType,
+			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
+			sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+			sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
+		),
+	)
+
 	defer func() {
 		telemetry.IncrCounterWithLabels(
 			[]string{"ibc", "client", "update"},

From b6a46359b869ab89be1882970c63d1298f2f91e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?colin=20axn=C3=A9r?=
 <25233464+colin-axner@users.noreply.github.com>
Date: Tue, 30 Mar 2021 11:35:10 +0200
Subject: [PATCH 08/24] Update
 modules/light-clients/07-tendermint/types/update.go

Co-authored-by: Zarko Milosevic <zarko@informal.systems>
---
 modules/light-clients/07-tendermint/types/update.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 558ed1a0885..c4f8310b56c 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -50,7 +50,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	}
 
 	// Check if the Client store already has a consensus state for the header's height
-	// If the consnensus state exists, and it matches the header then we return early
+	// If the consensus state exists, and it matches the header then we return early
 	// since header has already been submitted in a previous UpdateClient.
 	// If the consensus state and header mismatch, then we validate new header and freeze
 	// the client if it is valid since this is proof of a fork on the counterparty chain.

From 9edfaf6c97fbbea55c6d0e232b6fe5be71c1da75 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Tue, 30 Mar 2021 15:13:20 -0400
Subject: [PATCH 09/24] address colin comments

---
 modules/core/02-client/keeper/client.go       | 27 +++++++++----------
 modules/core/02-client/types/events.go        | 12 ++++-----
 .../07-tendermint/types/update.go             |  4 +--
 3 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 3bbf8fc08af..761a4b8915b 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -71,8 +71,8 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		existingConsState exported.ConsensusState
 		exists            bool
 		consensusHeight   exported.Height
-		eventType         string
 	)
+	eventType := types.EventTypeUpdateClient
 	if header != nil {
 		existingConsState, exists = k.GetClientConsensusState(ctx, clientID, header.GetHeight())
 	}
@@ -95,18 +95,12 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	}
 
-	// if there already exists a consensus state in client store for the header height
+	// If an existing consensus state doesn't exist, then write the update state changes,
+	// and set new consensus state.
+	// Else if there already exists a consensus state in client store for the header height
 	// and it does not match the updated consensus state, this is evidence of misbehaviour
 	// and we must freeze the client and emit appropriate events.
-	if exists && !reflect.DeepEqual(existingConsState, newConsensusState) {
-		clientState.Freeze(header)
-		k.SetClientState(ctx, clientID, clientState)
-
-		// set consensus height and set eventType to SubmitMisbehaviour
-		eventType = types.EventTypeSubmitMisbehaviour
-
-		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
-	} else if !exists {
+	if !exists {
 		// write any cached state changes from CheckHeaderAndUpdateState
 		// to store metadata in client store for new consensus state.
 		writeFn()
@@ -120,10 +114,15 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		}
 		k.SetClientState(ctx, clientID, newClientState)
 
-		// set eventType to UpdateClient
-		eventType = types.EventTypeUpdateClient
-
 		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
+	} else if exists && !reflect.DeepEqual(existingConsState, newConsensusState) {
+		clientState.Freeze(header)
+		k.SetClientState(ctx, clientID, clientState)
+
+		// set consensus height and set eventType to SubmitMisbehaviour
+		eventType = types.EventTypeSubmitMisbehaviour
+
+		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
 	}
 
 	// emitting events in the keeper emits for both begin block and handler client updates
diff --git a/modules/core/02-client/types/events.go b/modules/core/02-client/types/events.go
index 1c3988ba276..464ad4d426e 100644
--- a/modules/core/02-client/types/events.go
+++ b/modules/core/02-client/types/events.go
@@ -8,13 +8,11 @@ import (
 
 // IBC client events
 const (
-	AttributeKeyClientID            = "client_id"
-	AttributeKeySubjectClientID     = "subject_client_id"
-	AttributeKeyClientType          = "client_type"
-	AttributeKeyConsensusHeight     = "consensus_height"
-	AttributeKeyHeader              = "header"
-	AttributeKeyMisbehaviourHeader1 = "misbehaviour_header1"
-	AttributeKeyMisbehaviourHeader2 = "misbehaviour_header2"
+	AttributeKeyClientID        = "client_id"
+	AttributeKeySubjectClientID = "subject_client_id"
+	AttributeKeyClientType      = "client_type"
+	AttributeKeyConsensusHeight = "consensus_height"
+	AttributeKeyHeader          = "header"
 )
 
 // IBC client events vars
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 558ed1a0885..20c70629e97 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -50,10 +50,8 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	}
 
 	// Check if the Client store already has a consensus state for the header's height
-	// If the consnensus state exists, and it matches the header then we return early
+	// If the consensus state exists, and it matches the header then we return early
 	// since header has already been submitted in a previous UpdateClient.
-	// If the consensus state and header mismatch, then we validate new header and freeze
-	// the client if it is valid since this is proof of a fork on the counterparty chain.
 	var alreadyExists bool
 	prevConsState, _ := GetConsensusState(clientStore, cdc, header.GetHeight())
 	if prevConsState != nil {

From 7172807044a276aefb7f25993cb52505822ebba1 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Tue, 30 Mar 2021 16:20:52 -0400
Subject: [PATCH 10/24] freeze entire client on misbehaviour

---
 modules/core/02-client/keeper/client.go       |  6 +--
 modules/core/02-client/keeper/client_test.go  | 44 -------------------
 .../07-tendermint/types/client_state.go       |  2 +-
 .../07-tendermint/types/client_state_test.go  | 14 +++---
 .../07-tendermint/types/misbehaviour.go       |  4 --
 .../07-tendermint/types/misbehaviour_test.go  | 10 -----
 6 files changed, 11 insertions(+), 69 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 761a4b8915b..26527e36af1 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -62,8 +62,8 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
 	}
 
-	// prevent update if the client is frozen before or at header height
-	if clientState.IsFrozen() && clientState.GetFrozenHeight().LTE(header.GetHeight()) {
+	// prevent update if the client is frozen
+	if clientState.IsFrozen() {
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
@@ -119,7 +119,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		clientState.Freeze(header)
 		k.SetClientState(ctx, clientID, clientState)
 
-		// set consensus height and set eventType to SubmitMisbehaviour
+		// set eventType to SubmitMisbehaviour
 		eventType = types.EventTypeSubmitMisbehaviour
 
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index c2cfcee1c99..77da1cc5765 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -178,50 +178,6 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
 			return nil
 		}, true, true},
-		{"valid past update before client was frozen", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientState.FrozenHeight = types.NewHeight(0, testClientHeight.RevisionHeight-1)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
-
-			height1 := types.NewHeight(0, 1)
-
-			// store previous consensus state
-			prevConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.past,
-				NextValidatorsHash: suite.valSetHash,
-			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
-
-			// updateHeader will fill in consensus state between prevConsState and suite.consState
-			// clientState should not be updated
-			updateHeader = createPastUpdateFn(suite)
-			return nil
-		}, true, false},
-		{"misbehaviour detection: conflicting header before client was frozen", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientState.FrozenHeight = types.NewHeight(0, testClientHeight.RevisionHeight-1)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
-
-			height1 := types.NewHeight(0, 1)
-
-			// store previous consensus state
-			prevConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.past,
-				NextValidatorsHash: suite.valSetHash,
-			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
-
-			// updateHeader will fill in consensus state between prevConsState and suite.consState
-			// clientState should not be updated
-			updateHeader = createPastUpdateFn(suite)
-			// set conflicting consensus state in store to create misbehaviour scenario
-			conflictConsState := updateHeader.ConsensusState()
-			conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
-			return nil
-		}, true, true},
 		{"client state not found", func() error {
 			updateHeader = createFutureUpdateFn(suite)
 
diff --git a/modules/light-clients/07-tendermint/types/client_state.go b/modules/light-clients/07-tendermint/types/client_state.go
index f06f1e019d5..6059b58af02 100644
--- a/modules/light-clients/07-tendermint/types/client_state.go
+++ b/modules/light-clients/07-tendermint/types/client_state.go
@@ -508,7 +508,7 @@ func produceVerificationArgs(
 		)
 	}
 
-	if cs.IsFrozen() && !cs.FrozenHeight.GT(height) {
+	if cs.IsFrozen() {
 		return commitmenttypes.MerkleProof{}, nil, clienttypes.ErrClientFrozen
 	}
 
diff --git a/modules/light-clients/07-tendermint/types/client_state_test.go b/modules/light-clients/07-tendermint/types/client_state_test.go
index feb1e7db803..4dc49448531 100644
--- a/modules/light-clients/07-tendermint/types/client_state_test.go
+++ b/modules/light-clients/07-tendermint/types/client_state_test.go
@@ -174,7 +174,7 @@ func (suite *TendermintTestSuite) TestVerifyClientConsensusState() {
 		},
 		{
 			name:        "client is frozen",
-			clientState: &types.ClientState{LatestHeight: height, FrozenHeight: clienttypes.NewHeight(height.RevisionNumber, height.RevisionHeight-1)},
+			clientState: &types.ClientState{LatestHeight: height, FrozenHeight: clienttypes.NewHeight(height.RevisionNumber, height.RevisionHeight+100)},
 			consensusState: &types.ConsensusState{
 				Root: commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()),
 			},
@@ -239,7 +239,7 @@ func (suite *TendermintTestSuite) TestVerifyConnectionState() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
@@ -317,7 +317,7 @@ func (suite *TendermintTestSuite) TestVerifyChannelState() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
@@ -411,7 +411,7 @@ func (suite *TendermintTestSuite) TestVerifyPacketCommitment() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
@@ -510,7 +510,7 @@ func (suite *TendermintTestSuite) TestVerifyPacketAcknowledgement() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
@@ -614,7 +614,7 @@ func (suite *TendermintTestSuite) TestVerifyPacketReceiptAbsence() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
@@ -717,7 +717,7 @@ func (suite *TendermintTestSuite) TestVerifyNextSeqRecv() {
 		},
 		{
 			"client is frozen", func() {
-				clientState.FrozenHeight = clienttypes.NewHeight(0, 1)
+				clientState.FrozenHeight = clienttypes.NewHeight(0, 1000)
 			}, false,
 		},
 		{
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour.go b/modules/light-clients/07-tendermint/types/misbehaviour.go
index 51e59612f11..46f2945de88 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour.go
@@ -93,10 +93,6 @@ func (misbehaviour Misbehaviour) ValidateBasic() error {
 			sdkerrors.Wrap(err, "header 2 failed validation").Error(),
 		)
 	}
-	// Ensure that Heights are the same
-	if misbehaviour.Header1.GetHeight() != misbehaviour.Header2.GetHeight() {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "headers in misbehaviour are on different heights (%d ≠ %d)", misbehaviour.Header1.GetHeight(), misbehaviour.Header2.GetHeight())
-	}
 
 	blockID1, err := tmtypes.BlockIDFromProto(&misbehaviour.Header1.SignedHeader.Commit.BlockID)
 	if err != nil {
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
index 1b67b72973c..83e1acea873 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
@@ -151,16 +151,6 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			func(misbehaviour *types.Misbehaviour) error { return nil },
 			false,
 		},
-		{
-			"mismatched heights",
-			&types.Misbehaviour{
-				Header1:  suite.header,
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, 6, clienttypes.NewHeight(0, 4), suite.now, suite.valSet, suite.valSet, signers),
-				ClientId: clientID,
-			},
-			func(misbehaviour *types.Misbehaviour) error { return nil },
-			false,
-		},
 		{
 			"same block id",
 			&types.Misbehaviour{

From b3e50b1c706885823df289e9d7b6d61ebe8da94b Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Tue, 30 Mar 2021 17:28:26 -0400
Subject: [PATCH 11/24] add time misbehaviour and tests

---
 modules/core/02-client/keeper/client.go       |  4 +-
 modules/core/02-client/keeper/client_test.go  | 17 +++++++
 .../07-tendermint/types/misbehaviour.go       |  6 ++-
 .../types/misbehaviour_handle.go              | 35 ++++++++++++--
 .../types/misbehaviour_handle_test.go         | 47 ++++++++++++++++++-
 .../07-tendermint/types/misbehaviour_test.go  | 22 ++++++++-
 6 files changed, 121 insertions(+), 10 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 26527e36af1..0d340d8c96a 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -208,8 +208,8 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
 	}
 
-	if clientState.IsFrozen() && clientState.GetFrozenHeight().LTE(misbehaviour.GetHeight()) {
-		return sdkerrors.Wrapf(types.ErrInvalidMisbehaviour, "client is already frozen at height ≤ misbehaviour height (%s ≤ %s)", clientState.GetFrozenHeight(), misbehaviour.GetHeight())
+	if clientState.IsFrozen() {
+		return sdkerrors.Wrapf(types.ErrInvalidMisbehaviour, "client is already frozen at height %s", clientState.GetFrozenHeight())
 	}
 
 	clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, misbehaviour.GetClientID()), misbehaviour)
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index 77da1cc5765..d1c761ecb72 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -482,6 +482,23 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			},
 			true,
 		},
+		{
+			"time misbehavior should pass",
+			&ibctmtypes.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+5), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				ClientId: clientID,
+			},
+			func() error {
+				suite.consensusState.NextValidatorsHash = bothValsHash
+				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+
+				return err
+			},
+			true,
+		},
+
 		{
 			"misbehavior at later height should pass",
 			&ibctmtypes.Misbehaviour{
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour.go b/modules/light-clients/07-tendermint/types/misbehaviour.go
index 46f2945de88..d4907f953e9 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour.go
@@ -35,8 +35,6 @@ func (misbehaviour Misbehaviour) GetClientID() string {
 }
 
 // GetHeight returns the height at which misbehaviour occurred
-//
-// NOTE: assumes that misbehaviour headers have the same height
 func (misbehaviour Misbehaviour) GetHeight() exported.Height {
 	return misbehaviour.Header1.GetHeight()
 }
@@ -93,6 +91,10 @@ func (misbehaviour Misbehaviour) ValidateBasic() error {
 			sdkerrors.Wrap(err, "header 2 failed validation").Error(),
 		)
 	}
+	// Ensure that Height1 is greater than or equal to Height2
+	if misbehaviour.Header1.GetHeight().LT(misbehaviour.Header2.GetHeight()) {
+		return sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "Header1 height is less than Header2 height (%d < %d)", misbehaviour.Header1.GetHeight(), misbehaviour.Header2.GetHeight())
+	}
 
 	blockID1, err := tmtypes.BlockIDFromProto(&misbehaviour.Header1.SignedHeader.Commit.BlockID)
 	if err != nil {
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
index 0622372a237..68fb0718afe 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -1,6 +1,7 @@
 package types
 
 import (
+	"bytes"
 	"time"
 
 	tmtypes "github.com/tendermint/tendermint/types"
@@ -30,12 +31,37 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", misbehaviour, &Misbehaviour{})
 	}
 
-	// If client is already frozen at earlier height than misbehaviour, return with error
-	if cs.IsFrozen() && cs.FrozenHeight.LTE(misbehaviour.GetHeight()) {
-		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour,
-			"client is already frozen at earlier height %s than misbehaviour height %s", cs.FrozenHeight, misbehaviour.GetHeight())
+	// If client is already frozen, return with error
+	if cs.IsFrozen() {
+		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "client is already frozen at height %s", cs.FrozenHeight)
 	}
 
+	// if heights are equal check that this is valid misbehaviour of a fork
+	// if heights are unequal check that this is valid misbehavior of BFT time violation
+	if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) {
+		blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID)
+		if err != nil {
+			return nil, sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour")
+		}
+		blockID2, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header2.SignedHeader.Commit.BlockID)
+		if err != nil {
+			return nil, 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 nil, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers block hashes are equal")
+		}
+	} else {
+		// 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).
+		if tmMisbehaviour.Header1.SignedHeader.Header.Time.After(tmMisbehaviour.Header2.SignedHeader.Header.Time) {
+			return nil, 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
 	// and unmarshal from clientStore
 
@@ -68,6 +94,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 	}
 
 	cs.FrozenHeight = tmMisbehaviour.GetHeight().(clienttypes.Height)
+
 	return &cs, nil
 }
 
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
index da1cd6fbfe6..8583e008bb2 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
@@ -50,7 +50,7 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		expPass         bool
 	}{
 		{
-			"valid misbehavior misbehaviour",
+			"valid fork misbehaviour",
 			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
@@ -64,6 +64,21 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			suite.now,
 			true,
 		},
+		{
+			"valid time misbehaviour",
+			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			&types.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+3), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				ClientId: chainID,
+			},
+			suite.now,
+			true,
+		},
 		{
 			"valid misbehavior at height greater than last consensusState",
 			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
@@ -154,6 +169,36 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			suite.now,
 			true,
 		},
+		{
+			"invalid fork misbehaviour: identical headers",
+			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			&types.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				ClientId: chainID,
+			},
+			suite.now,
+			false,
+		},
+		{
+			"invalid time misbehaviour: monotonically increasing time",
+			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			&types.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+3), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				ClientId: chainID,
+			},
+			suite.now,
+			false,
+		},
 		{
 			"invalid misbehavior misbehaviour from different chain",
 			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
index 83e1acea873..26e2e0c0625 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
@@ -60,7 +60,7 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 		expPass              bool
 	}{
 		{
-			"valid misbehaviour",
+			"valid fork misbehaviour",
 			&types.Misbehaviour{
 				Header1:  suite.header,
 				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers),
@@ -69,6 +69,16 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			func(misbehaviour *types.Misbehaviour) error { return nil },
 			true,
 		},
+		{
+			"valid time misbehaviour",
+			&types.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+5), heightMinus1, suite.now, suite.valSet, suite.valSet, signers),
+				Header2:  suite.header,
+				ClientId: clientID,
+			},
+			func(misbehaviour *types.Misbehaviour) error { return nil },
+			true,
+		},
 		{
 			"misbehaviour Header1 is nil",
 			types.NewMisbehaviour(clientID, nil, suite.header),
@@ -151,6 +161,16 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			func(misbehaviour *types.Misbehaviour) error { return nil },
 			false,
 		},
+		{
+			"header2 height is greater",
+			&types.Misbehaviour{
+				Header1:  suite.header,
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, 6, clienttypes.NewHeight(0, 4), suite.now, suite.valSet, suite.valSet, signers),
+				ClientId: clientID,
+			},
+			func(misbehaviour *types.Misbehaviour) error { return nil },
+			false,
+		},
 		{
 			"same block id",
 			&types.Misbehaviour{

From 937364d7c7d5644710b9cc7cea7ed5d20742308e Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Wed, 31 Mar 2021 16:53:32 -0400
Subject: [PATCH 12/24] enforce trusted height less than current height in
 header.ValidateBasic

---
 modules/core/02-client/keeper/client_test.go  | 30 ++++----
 .../07-tendermint/types/header.go             |  7 +-
 .../07-tendermint/types/header_test.go        |  4 +-
 .../07-tendermint/types/misbehaviour.go       |  5 --
 .../types/misbehaviour_handle.go              |  2 +-
 .../types/misbehaviour_handle_test.go         | 72 +++++++++----------
 .../07-tendermint/types/misbehaviour_test.go  | 14 +---
 7 files changed, 59 insertions(+), 75 deletions(-)

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index d1c761ecb72..df8c32ede06 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -469,8 +469,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"trusting period misbehavior should pass",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -486,7 +486,7 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			"time misbehavior should pass",
 			&ibctmtypes.Misbehaviour{
 				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+5), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -502,8 +502,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"misbehavior at later height should pass",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), testClientHeight, altTime, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, valSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -528,8 +528,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"misbehavior at later height with different trusted heights should pass",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), testClientHeight, altTime, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, valSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -554,8 +554,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"trusted ConsensusState1 not found",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), heightPlus3, altTime, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -570,8 +570,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"trusted ConsensusState2 not found",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), testClientHeight, altTime, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, valSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -592,8 +592,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"client already frozen at earlier height",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
 				ClientId: clientID,
 			},
 			func() error {
@@ -611,8 +611,8 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 		{
 			"misbehaviour check failed",
 			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), altValSet, bothValSet, altSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), altValSet, bothValSet, altSigners),
 				ClientId: clientID,
 			},
 			func() error {
diff --git a/modules/light-clients/07-tendermint/types/header.go b/modules/light-clients/07-tendermint/types/header.go
index 9bd59708ee5..d3662bb72a8 100644
--- a/modules/light-clients/07-tendermint/types/header.go
+++ b/modules/light-clients/07-tendermint/types/header.go
@@ -62,10 +62,9 @@ func (h Header) ValidateBasic() error {
 		return sdkerrors.Wrap(err, "header failed basic validation")
 	}
 
-	// TrustedHeight is less than Header for updates
-	// and less than or equal to Header for misbehaviour
-	if h.TrustedHeight.GT(h.GetHeight()) {
-		return sdkerrors.Wrapf(ErrInvalidHeaderHeight, "TrustedHeight %d must be less than or equal to header height %d",
+	// TrustedHeight is less than Header for updates and misbehaviour
+	if h.TrustedHeight.GTE(h.GetHeight()) {
+		return sdkerrors.Wrapf(ErrInvalidHeaderHeight, "TrustedHeight %d must be less than header height %d",
 			h.TrustedHeight, h.GetHeight())
 	}
 
diff --git a/modules/light-clients/07-tendermint/types/header_test.go b/modules/light-clients/07-tendermint/types/header_test.go
index 487b27943dc..37a7082d62a 100644
--- a/modules/light-clients/07-tendermint/types/header_test.go
+++ b/modules/light-clients/07-tendermint/types/header_test.go
@@ -43,8 +43,8 @@ func (suite *TendermintTestSuite) TestHeaderValidateBasic() {
 			header = suite.chainA.LastHeader
 			header.SignedHeader.Commit = nil
 		}, false},
-		{"trusted height is greater than header height", func() {
-			header.TrustedHeight = header.GetHeight().(clienttypes.Height).Increment().(clienttypes.Height)
+		{"trusted height is equal to header height", func() {
+			header.TrustedHeight = header.GetHeight().(clienttypes.Height)
 		}, false},
 		{"validator set nil", func() {
 			header.ValidatorSet = nil
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour.go b/modules/light-clients/07-tendermint/types/misbehaviour.go
index d4907f953e9..e500fb69e69 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour.go
@@ -1,7 +1,6 @@
 package types
 
 import (
-	"bytes"
 	"time"
 
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -105,10 +104,6 @@ func (misbehaviour Misbehaviour) ValidateBasic() error {
 		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")
-	}
 	if err := validCommit(misbehaviour.Header1.Header.ChainID, *blockID1,
 		misbehaviour.Header1.Commit, misbehaviour.Header1.ValidatorSet); err != nil {
 		return err
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
index 68fb0718afe..1626b497cbd 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -37,7 +37,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 	}
 
 	// if heights are equal check that this is valid misbehaviour of a fork
-	// if heights are unequal check that this is valid misbehavior of BFT time violation
+	// otherwise if heights are unequal check that this is valid misbehavior of BFT time violation
 	if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) {
 		blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID)
 		if err != nil {
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
index 8583e008bb2..7dad544fa4e 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
@@ -57,8 +57,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -73,7 +73,7 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			height,
 			&types.Misbehaviour{
 				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+3), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -87,8 +87,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			heightMinus1,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -102,8 +102,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), suite.valsHash),
 			heightMinus3,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus3, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus3, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -117,8 +117,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), suite.valsHash),
 			heightMinus3,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight), heightMinus3, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainIDRevision0, int64(height.RevisionHeight+1), heightMinus3, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -162,8 +162,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), suite.valsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, suite.valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, suite.valSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -177,8 +177,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -193,7 +193,7 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			height,
 			&types.Misbehaviour{
 				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+3), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -207,8 +207,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader("ethermint", int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader("ethermint", int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader("ethermint", int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader("ethermint", int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -222,8 +222,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), suite.valsHash),
 			heightMinus3,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -237,8 +237,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), suite.valsHash),
 			heightMinus3,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus3, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus3, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -252,8 +252,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -267,8 +267,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -293,8 +293,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -308,8 +308,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), heightMinus1, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now.Add(trustingPeriod),
@@ -323,8 +323,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, suite.valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, suite.valSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, suite.valSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -338,8 +338,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, altValSet, bothValSet, altSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, altValSet, bothValSet, altSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), bothValSet, bothValSet, bothSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -353,8 +353,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), altValSet, bothValSet, altSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), altValSet, bothValSet, altSigners),
 				ClientId: chainID,
 			},
 			suite.now,
@@ -368,8 +368,8 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
 			height,
 			&types.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now, altValSet, bothValSet, altSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), height, suite.now.Add(time.Minute), altValSet, bothValSet, altSigners),
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now, altValSet, bothValSet, altSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Minute), altValSet, bothValSet, altSigners),
 				ClientId: chainID,
 			},
 			suite.now,
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
index 26e2e0c0625..b46e25333a0 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
@@ -60,7 +60,7 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 		expPass              bool
 	}{
 		{
-			"valid fork misbehaviour",
+			"valid fork misbehaviour, two headers at same height have different time",
 			&types.Misbehaviour{
 				Header1:  suite.header,
 				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight), heightMinus1, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers),
@@ -70,7 +70,7 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			true,
 		},
 		{
-			"valid time misbehaviour",
+			"valid time misbehaviour, both headers at different heights are at same time",
 			&types.Misbehaviour{
 				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+5), heightMinus1, suite.now, suite.valSet, suite.valSet, signers),
 				Header2:  suite.header,
@@ -171,16 +171,6 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			func(misbehaviour *types.Misbehaviour) error { return nil },
 			false,
 		},
-		{
-			"same block id",
-			&types.Misbehaviour{
-				Header1:  suite.header,
-				Header2:  suite.header,
-				ClientId: clientID,
-			},
-			func(misbehaviour *types.Misbehaviour) error { return nil },
-			false,
-		},
 		{
 			"header 1 doesn't have 2/3 majority",
 			&types.Misbehaviour{

From 5c793ae507739505776fe2f98c1ecee2d47c9707 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Wed, 31 Mar 2021 17:38:18 -0400
Subject: [PATCH 13/24] cleanup and tests

---
 modules/core/02-client/keeper/client.go         |  6 +++++-
 modules/core/02-client/keeper/client_test.go    | 17 ++++++++++++++++-
 .../07-tendermint/types/misbehaviour_test.go    |  2 +-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 0d340d8c96a..807cbbdd5b8 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -209,7 +209,11 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
 	}
 
 	if clientState.IsFrozen() {
-		return sdkerrors.Wrapf(types.ErrInvalidMisbehaviour, "client is already frozen at height %s", clientState.GetFrozenHeight())
+		return sdkerrors.Wrap(types.ErrInvalidMisbehaviour, "client is already frozen")
+	}
+
+	if err := misbehaviour.ValidateBasic(); err != nil {
+		return err
 	}
 
 	clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, misbehaviour.GetClientID()), misbehaviour)
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index df8c32ede06..e0fe48eb271 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -498,7 +498,6 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			},
 			true,
 		},
-
 		{
 			"misbehavior at later height should pass",
 			&ibctmtypes.Misbehaviour{
@@ -551,6 +550,22 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			},
 			true,
 		},
+		{
+			"misbehavior ValidateBasic fails: misbehaviour height is at same height as trusted height",
+			&ibctmtypes.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
+				ClientId: clientID,
+			},
+			func() error {
+				suite.consensusState.NextValidatorsHash = bothValsHash
+				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+
+				return err
+			},
+			false,
+		},
 		{
 			"trusted ConsensusState1 not found",
 			&ibctmtypes.Misbehaviour{
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
index b46e25333a0..30deec2cb90 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_test.go
@@ -165,7 +165,7 @@ func (suite *TendermintTestSuite) TestMisbehaviourValidateBasic() {
 			"header2 height is greater",
 			&types.Misbehaviour{
 				Header1:  suite.header,
-				Header2:  suite.chainA.CreateTMClientHeader(chainID, 6, clienttypes.NewHeight(0, 4), suite.now, suite.valSet, suite.valSet, signers),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, 6, clienttypes.NewHeight(0, height.RevisionHeight+1), suite.now, suite.valSet, suite.valSet, signers),
 				ClientId: clientID,
 			},
 			func(misbehaviour *types.Misbehaviour) error { return nil },

From 906a41364d7c4c40693968abd153718e3cbc43fd Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Mon, 26 Apr 2021 14:08:24 -0400
Subject: [PATCH 14/24] fix print statement

---
 modules/light-clients/07-tendermint/types/misbehaviour.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/light-clients/07-tendermint/types/misbehaviour.go b/modules/light-clients/07-tendermint/types/misbehaviour.go
index e500fb69e69..b14593e1635 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour.go
@@ -92,7 +92,7 @@ func (misbehaviour Misbehaviour) ValidateBasic() error {
 	}
 	// Ensure that Height1 is greater than or equal to Height2
 	if misbehaviour.Header1.GetHeight().LT(misbehaviour.Header2.GetHeight()) {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "Header1 height is less than Header2 height (%d < %d)", misbehaviour.Header1.GetHeight(), misbehaviour.Header2.GetHeight())
+		return sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "Header1 height is less than Header2 height (%s < %s)", misbehaviour.Header1.GetHeight(), misbehaviour.Header2.GetHeight())
 	}
 
 	blockID1, err := tmtypes.BlockIDFromProto(&misbehaviour.Header1.SignedHeader.Commit.BlockID)

From 155e4613e3e34958441817c0c7d711b0225d00dd Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Mon, 26 Apr 2021 19:08:50 -0400
Subject: [PATCH 15/24] enforce monotonicity in update

---
 modules/core/02-client/keeper/client.go       |  27 ++--
 modules/core/02-client/keeper/client_test.go  |  58 ++++++---
 .../07-tendermint/types/store.go              |  12 +-
 .../07-tendermint/types/update.go             |  34 +++--
 .../07-tendermint/types/update_test.go        | 118 +++++++++++++-----
 5 files changed, 169 insertions(+), 80 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 807cbbdd5b8..5f7224c8968 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"encoding/hex"
-	"reflect"
 
 	"github.com/armon/go-metrics"
 
@@ -67,15 +66,8 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
-	var (
-		existingConsState exported.ConsensusState
-		exists            bool
-		consensusHeight   exported.Height
-	)
+	var consensusHeight exported.Height
 	eventType := types.EventTypeUpdateClient
-	if header != nil {
-		existingConsState, exists = k.GetClientConsensusState(ctx, clientID, header.GetHeight())
-	}
 
 	cacheCtx, writeFn := ctx.CacheContext()
 	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(cacheCtx, k.cdc, k.ClientStore(ctx, clientID), header)
@@ -95,12 +87,11 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	}
 
-	// If an existing consensus state doesn't exist, then write the update state changes,
-	// and set new consensus state.
-	// Else if there already exists a consensus state in client store for the header height
-	// and it does not match the updated consensus state, this is evidence of misbehaviour
-	// and we must freeze the client and emit appropriate events.
-	if !exists {
+	k.SetClientState(ctx, clientID, newClientState)
+	// If client state is not frozen after clientState CheckHeaderAndUpdateState,
+	// then write the update state changes, and set new consensus state.
+	// Else the update was proof of misbehaviour and we must emit appropriate misbehaviour events.
+	if !newClientState.IsFrozen() {
 		// write any cached state changes from CheckHeaderAndUpdateState
 		// to store metadata in client store for new consensus state.
 		writeFn()
@@ -112,13 +103,9 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		} else {
 			consensusHeight = types.GetSelfHeight(ctx)
 		}
-		k.SetClientState(ctx, clientID, newClientState)
 
 		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
-	} else if exists && !reflect.DeepEqual(existingConsState, newConsensusState) {
-		clientState.Freeze(header)
-		k.SetClientState(ctx, clientID, clientState)
-
+	} else {
 		// set eventType to SubmitMisbehaviour
 		eventType = types.EventTypeSubmitMisbehaviour
 
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index fabd4914633..b51f70f8fe2 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -178,6 +178,28 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
 			return nil
 		}, true, true},
+		{"misbehaviour detection: monotonic time violation", func() error {
+			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+
+			// store intermediate consensus state at a time greater than updateHeader time
+			// this will break time monotonicity
+			incrementedClientHeight := testClientHeight.Increment().(types.Height)
+			intermediateConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          suite.header.Header.Time.Add(2 * time.Hour),
+				NextValidatorsHash: suite.valSetHash,
+			}
+			suite.keeper.SetClientConsensusState(suite.ctx, clientID, incrementedClientHeight, intermediateConsState)
+			// set iteration key
+			clientStore := suite.keeper.ClientStore(suite.ctx, clientID)
+			ibctmtypes.SetIterationKey(clientStore, incrementedClientHeight)
+
+			clientState.LatestHeight = incrementedClientHeight
+			suite.keeper.SetClientState(suite.ctx, clientID, clientState)
+
+			updateHeader = createFutureUpdateFn(suite)
+			return err
+		}, true, true},
 		{"client state not found", func() error {
 			updateHeader = createFutureUpdateFn(suite)
 
@@ -224,33 +246,33 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			if tc.expPass {
 				suite.Require().NoError(err, err)
 
-				expConsensusState := &ibctmtypes.ConsensusState{
-					Timestamp:          updateHeader.GetTime(),
-					Root:               commitmenttypes.NewMerkleRoot(updateHeader.Header.GetAppHash()),
-					NextValidatorsHash: updateHeader.Header.NextValidatorsHash,
-				}
-
 				newClientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
 				suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
 
-				consensusState, found := suite.keeper.GetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight())
-				suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
-
-				// Determine if clientState should be updated or not
-				if updateHeader.GetHeight().GT(clientState.GetLatestHeight()) {
-					// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
-					suite.Require().Equal(updateHeader.GetHeight(), newClientState.GetLatestHeight(), "clientstate height did not update")
-				} else {
-					// Update will add past consensus state, clientState should not be updated at all
-					suite.Require().Equal(clientState.GetLatestHeight(), newClientState.GetLatestHeight(), "client state height updated for past header")
-				}
-
 				// If the update freezes the client, check that client was frozen to update header's height.
 				// Otherwise check that consensus state is stored as expected.
 				if tc.freeze {
 					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
 					suite.Require().Equal(newClientState.GetFrozenHeight(), updateHeader.GetHeight(), "client frozen at wrong height")
 				} else {
+					expConsensusState := &ibctmtypes.ConsensusState{
+						Timestamp:          updateHeader.GetTime(),
+						Root:               commitmenttypes.NewMerkleRoot(updateHeader.Header.GetAppHash()),
+						NextValidatorsHash: updateHeader.Header.NextValidatorsHash,
+					}
+
+					consensusState, found := suite.keeper.GetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight())
+					suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
+
+					// Determine if clientState should be updated or not
+					if updateHeader.GetHeight().GT(clientState.GetLatestHeight()) {
+						// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
+						suite.Require().Equal(updateHeader.GetHeight(), newClientState.GetLatestHeight(), "clientstate height did not update")
+					} else {
+						// Update will add past consensus state, clientState should not be updated at all
+						suite.Require().Equal(clientState.GetLatestHeight(), newClientState.GetLatestHeight(), "client state height updated for past header")
+					}
+
 					suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 					suite.Require().Equal(expConsensusState, consensusState, "consensus state should have been updated on case %s", tc.name)
 				}
diff --git a/modules/light-clients/07-tendermint/types/store.go b/modules/light-clients/07-tendermint/types/store.go
index 726fdfd6c34..cb4a99f91ae 100644
--- a/modules/light-clients/07-tendermint/types/store.go
+++ b/modules/light-clients/07-tendermint/types/store.go
@@ -1,6 +1,7 @@
 package types
 
 import (
+	"bytes"
 	"encoding/binary"
 	"strings"
 
@@ -191,12 +192,19 @@ func GetNextConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, h
 	iterateStore := prefix.NewStore(clientStore, []byte(KeyIterateConsensusStatePrefix))
 	iterator := iterateStore.Iterator(bigEndianHeightBytes(height), nil)
 	defer iterator.Close()
-	// ignore the consensus state at current height and get next height
-	iterator.Next()
 	if !iterator.Valid() {
 		return nil, false
 	}
 
+	// if iterator is at current height, ignore the consensus state at current height and get next height
+	// if iterator value is not at current height, it is already at next height.
+	if bytes.Equal(iterator.Value(), host.ConsensusStateKey(height)) {
+		iterator.Next()
+		if !iterator.Valid() {
+			return nil, false
+		}
+	}
+
 	csKey := iterator.Value()
 
 	return getTmConsensusState(clientStore, cdc, csKey)
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 5c669dafb27..7903ccb9d37 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -57,7 +57,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// Check if the Client store already has a consensus state for the header's height
 	// If the consensus state exists, and it matches the header then we return early
 	// since header has already been submitted in a previous UpdateClient.
-	var alreadyExists bool
+	var conflictingHeader bool
 	prevConsState, _ := GetConsensusState(clientStore, cdc, header.GetHeight())
 	if prevConsState != nil {
 		// This header has already been submitted and the necessary state is already stored
@@ -67,27 +67,43 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 		}
 		// A consensus state already exists for this height, but it does not match the provided header.
 		// Thus, we must check that this header is valid, and if so we will freeze the client.
-		alreadyExists = true
+		conflictingHeader = true
 	}
 
 	// get consensus state from clientStore
-	tmConsState, err := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight)
+	trustedConsState, err := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight)
 	if err != nil {
 		return nil, nil, sdkerrors.Wrapf(
 			err, "could not get consensus state from clientstore at TrustedHeight: %s", tmHeader.TrustedHeight,
 		)
 	}
 
-	if err := checkValidity(&cs, tmConsState, tmHeader, ctx.BlockTime()); err != nil {
+	if err := checkValidity(&cs, trustedConsState, tmHeader, ctx.BlockTime()); err != nil {
 		return nil, nil, err
 	}
 
-	// Header is different from existing consensus state and also valid, so return the consensus state of the header.
-	// ClientKeeper must freeze the client based on the header.
-	if alreadyExists {
-		return &cs, tmHeader.ConsensusState(), nil
+	consState := tmHeader.ConsensusState()
+	// Header is different from existing consensus state and also valid, so freeze the client and return
+	if conflictingHeader {
+		cs.Freeze(header)
+		return &cs, consState, nil
 	}
-	
+	// Check that consensus state timestamps are monotonic
+	prevCons, prevOk := GetPreviousConsensusState(clientStore, cdc, header.GetHeight())
+	nextCons, nextOk := GetNextConsensusState(clientStore, cdc, header.GetHeight())
+	// if previous consensus state exists, check consensus state time is greater than previous consensus state time
+	// if previous consensus state is not before current consensus state, freeze the client and return.
+	if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) {
+		cs.Freeze(header)
+		return &cs, consState, nil
+	}
+	// if next consensus state exists, check consensus state time is less than next consensus state time
+	// if next consensus state is not after current consensus state, freeze the client and return.
+	if nextOk && !nextCons.Timestamp.After(consState.Timestamp) {
+		cs.Freeze(header)
+		return &cs, consState, nil
+	}
+
 	// Check the earliest consensus state to see if it is expired, if so then set the prune height
 	// so that we can delete consensus state and all associated metadata.
 	var (
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 15d8758a619..0918b27f743 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -44,9 +44,10 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 	altSigners := []tmtypes.PrivValidator{altPrivVal}
 
 	testCases := []struct {
-		name    string
-		setup   func(*TendermintTestSuite)
-		expPass bool
+		name      string
+		setup     func(*TendermintTestSuite)
+		expFrozen bool
+		expPass   bool
 	}{
 		{
 			name: "successful update with next height and same validator set",
@@ -56,7 +57,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expFrozen: false,
+			expPass:   true,
 		},
 		{
 			name: "successful update with future height and different validator set",
@@ -66,7 +68,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expFrozen: false,
+			expPass:   true,
 		},
 		{
 			name: "successful update with next height and different validator set",
@@ -76,7 +79,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expFrozen: false,
+			expPass:   true,
 		},
 		{
 			name: "successful update for a previous height",
@@ -87,7 +91,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), heightMinus3, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: true,
+			expFrozen: false,
+			expPass:   true,
 		},
 		{
 			name: "successful update for a previous revision",
@@ -111,7 +116,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// Store the header's consensus state in client store before UpdateClient call
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, newHeader.ConsensusState())
 			},
-			expPass: true,
+			expFrozen: false,
+			expPass:   true,
 		},
 		{
 			name: "misbehaviour detection: header conflicts with existing consensus state",
@@ -126,7 +132,42 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, conflictConsState)
 			},
-			expPass: true,
+			expFrozen: true,
+			expPass:   true,
+		},
+		{
+			name: "misbehaviour detection: previous consensus state time is not before header time. time monotonicity violation",
+			setup: func(suite *TendermintTestSuite) {
+				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				// swap header and client time to create violation
+				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
+				currentTime = suite.now
+				prevConsensusState := types.NewConsensusState(suite.headerTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
+				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus1, prevConsensusState)
+				clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, clientID)
+				types.SetIterationKey(clientStore, heightPlus1)
+			},
+			expFrozen: true,
+			expPass:   true,
+		},
+		{
+			name: "misbehaviour detection: next consensus state time is not after header time. time monotonicity violation",
+			setup: func(suite *TendermintTestSuite) {
+				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+				// swap header and client time to create violation
+				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
+				currentTime = suite.now
+				nextConsensusState := types.NewConsensusState(suite.headerTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
+				ctx := suite.chainA.GetContext().WithBlockTime(currentTime)
+				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(ctx, clientID, heightPlus5, nextConsensusState)
+				clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, clientID)
+				types.SetIterationKey(clientStore, heightPlus5)
+			},
+			expFrozen: true,
+			expPass:   true,
 		},
 		{
 			name: "unsuccessful update with incorrect header chain-id",
@@ -136,7 +177,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader("ethermint", int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update to a future revision",
@@ -156,7 +198,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainIDRevision1, 3, height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches nextValSetHash",
@@ -166,7 +209,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, bothValSet, suite.valSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update with next height: update header mismatches different nextValSetHash",
@@ -176,7 +220,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, bothValSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update with future height: too much change in validator set",
@@ -186,7 +231,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, altValSet, suite.valSet, altSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful updates, passed in incorrect trusted validators for given consensus state",
@@ -196,7 +242,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, bothValSet, bothValSet, bothSigners)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update: trusting period has passed since last client timestamp",
@@ -207,7 +254,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				// make current time pass trusting period from last timestamp on clientstate
 				currentTime = suite.now.Add(trustingPeriod)
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is past current timestamp",
@@ -217,7 +265,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.now.Add(time.Minute), suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "unsuccessful update: header timestamp is not past last client timestamp",
@@ -227,7 +276,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.clientTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "header basic validation failed",
@@ -239,7 +289,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader.SignedHeader.Commit.Height = revisionHeight - 1
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 		{
 			name: "header height < consensus height",
@@ -250,7 +301,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightMinus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
 			},
-			expPass: false,
+			expFrozen: false,
+			expPass:   false,
 		},
 	}
 
@@ -266,7 +318,6 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			_, suiteVal := suite.valSet.GetByIndex(0)
 			bothSigners = ibctesting.CreateSortedSignerArray(altPrivVal, suite.privVal, altVal, suiteVal)
 
-
 			consStateHeight = height // must be explicitly changed
 			// setup test
 			tc.setup(suite)
@@ -294,17 +345,22 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			if tc.expPass {
 				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 
-				// Determine if clientState should be updated or not
-				// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
-				if height.GT(clientState.LatestHeight) {
-					// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
-					suite.Require().Equal(height, newClientState.GetLatestHeight(), "clientstate height did not update")
-				} else {
-					// Update will add past consensus state, clientState should not be updated at all
-					suite.Require().Equal(clientState.LatestHeight, newClientState.GetLatestHeight(), "client state height updated for past header")
-				}
+				suite.Require().Equal(tc.expFrozen, newClientState.IsFrozen(), "client state status is unexpected after update")
 
-				suite.Require().Equal(expectedConsensus, consensusState, "valid test case %d failed: %s", i, tc.name)
+				// further writes only happen if update is not misbehaviour
+				if !tc.expFrozen {
+					// Determine if clientState should be updated or not
+					// TODO: check the entire Height struct once GetLatestHeight returns clienttypes.Height
+					if height.GT(clientState.LatestHeight) {
+						// Header Height is greater than clientState latest Height, clientState should be updated with header.GetHeight()
+						suite.Require().Equal(height, newClientState.GetLatestHeight(), "clientstate height did not update")
+					} else {
+						// Update will add past consensus state, clientState should not be updated at all
+						suite.Require().Equal(clientState.LatestHeight, newClientState.GetLatestHeight(), "client state height updated for past header")
+					}
+
+					suite.Require().Equal(expectedConsensus, consensusState, "valid test case %d failed: %s", i, tc.name)
+				}
 			} else {
 				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
 				suite.Require().Nil(newClientState, "invalid test case %d passed: %s", i, tc.name)

From 326e5fb9d537b7a579d5537f77b2ae67a8635854 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Mon, 26 Apr 2021 19:24:22 -0400
Subject: [PATCH 16/24] add docs and remove unnecessary interface function

---
 modules/core/02-client/keeper/client.go               |  3 ++-
 modules/core/exported/client.go                       |  1 -
 .../06-solomachine/types/client_state.go              |  6 ------
 .../light-clients/07-tendermint/types/client_state.go |  6 ------
 modules/light-clients/07-tendermint/types/update.go   | 11 ++++++++---
 .../light-clients/09-localhost/types/client_state.go  |  5 -----
 6 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 5f7224c8968..903357f8dfa 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -87,9 +87,10 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	}
 
+	// set new client state regardless of if update is valid update or misbehaviour
 	k.SetClientState(ctx, clientID, newClientState)
 	// If client state is not frozen after clientState CheckHeaderAndUpdateState,
-	// then write the update state changes, and set new consensus state.
+	// then update was valid. Write the update state changes, and set new consensus state.
 	// Else the update was proof of misbehaviour and we must emit appropriate misbehaviour events.
 	if !newClientState.IsFrozen() {
 		// write any cached state changes from CheckHeaderAndUpdateState
diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go
index f9ab322eef7..3d552b07724 100644
--- a/modules/core/exported/client.go
+++ b/modules/core/exported/client.go
@@ -31,7 +31,6 @@ type ClientState interface {
 	GetLatestHeight() Height
 	IsFrozen() bool
 	GetFrozenHeight() Height
-	Freeze(Header)
 	Validate() error
 	GetProofSpecs() []*ics23.ProofSpec
 
diff --git a/modules/light-clients/06-solomachine/types/client_state.go b/modules/light-clients/06-solomachine/types/client_state.go
index b544a5eb145..efa740cab6a 100644
--- a/modules/light-clients/06-solomachine/types/client_state.go
+++ b/modules/light-clients/06-solomachine/types/client_state.go
@@ -52,12 +52,6 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return clienttypes.NewHeight(0, cs.FrozenSequence)
 }
 
-// Freeze takes a misbehaving header and freezes the client at the header height
-// NOTE: Header must be verified as misbehaviour before calling this function.
-func (cs *ClientState) Freeze(header exported.Header) {
-	cs.FrozenSequence = header.GetHeight().GetRevisionHeight()
-}
-
 // GetProofSpecs returns nil proof specs since client state verification uses signatures.
 func (cs ClientState) GetProofSpecs() []*ics23.ProofSpec {
 	return nil
diff --git a/modules/light-clients/07-tendermint/types/client_state.go b/modules/light-clients/07-tendermint/types/client_state.go
index 3d602ab59ab..f7fb6a24597 100644
--- a/modules/light-clients/07-tendermint/types/client_state.go
+++ b/modules/light-clients/07-tendermint/types/client_state.go
@@ -69,12 +69,6 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return cs.FrozenHeight
 }
 
-// Freeze takes a misbehaving header and freezes the client at the header height
-// NOTE: Header must be verified as misbehaviour before calling this function.
-func (cs *ClientState) Freeze(header exported.Header) {
-	cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
-}
-
 // IsExpired returns whether or not the client has passed the trusting period since the last
 // update (in which case no headers are considered valid).
 func (cs ClientState) IsExpired(latestTimestamp, now time.Time) bool {
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index 7903ccb9d37..ce085617786 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -39,6 +39,11 @@ import (
 // Tendermint client validity checking uses the bisection algorithm described
 // in the [Tendermint spec](https://github.com/tendermint/spec/blob/master/spec/consensus/light-client.md).
 //
+// Misbehaviour Detection:
+// UpdateClient will detect implicit misbehaviour by enforcing certain invariants on any new update call and will return a frozen client.
+// 1. Any valid update that creates a different consensus state for an already existing height is evidence of misbehaviour and will freeze client.
+// 2. Any valid update that breaks time monotonicity with respect to its neighboring consensus states is evidence of misbehaviour and will freeze client.
+//
 // Pruning:
 // UpdateClient will additionally retrieve the earliest consensus state for this clientID and check if it is expired. If it is,
 // that consensus state will be pruned from store along with all associated metadata. This will prevent the client store from
@@ -85,7 +90,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	consState := tmHeader.ConsensusState()
 	// Header is different from existing consensus state and also valid, so freeze the client and return
 	if conflictingHeader {
-		cs.Freeze(header)
+		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
 		return &cs, consState, nil
 	}
 	// Check that consensus state timestamps are monotonic
@@ -94,13 +99,13 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// if previous consensus state exists, check consensus state time is greater than previous consensus state time
 	// if previous consensus state is not before current consensus state, freeze the client and return.
 	if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) {
-		cs.Freeze(header)
+		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
 		return &cs, consState, nil
 	}
 	// if next consensus state exists, check consensus state time is less than next consensus state time
 	// if next consensus state is not after current consensus state, freeze the client and return.
 	if nextOk && !nextCons.Timestamp.After(consState.Timestamp) {
-		cs.Freeze(header)
+		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
 		return &cs, consState, nil
 	}
 
diff --git a/modules/light-clients/09-localhost/types/client_state.go b/modules/light-clients/09-localhost/types/client_state.go
index ef0e2368731..6336a213ffc 100644
--- a/modules/light-clients/09-localhost/types/client_state.go
+++ b/modules/light-clients/09-localhost/types/client_state.go
@@ -53,11 +53,6 @@ func (cs ClientState) GetFrozenHeight() exported.Height {
 	return clienttypes.ZeroHeight()
 }
 
-// Freeze is a no-op for localhost clients
-func (cs *ClientState) Freeze(exported.Header) {
-	return
-}
-
 // Validate performs a basic validation of the client state fields.
 func (cs ClientState) Validate() error {
 	if strings.TrimSpace(cs.ChainId) == "" {

From 2eaa2c90850b49096c66cf90b8163ffac2714fc8 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Wed, 28 Apr 2021 12:09:09 -0400
Subject: [PATCH 17/24] first round of review comments

---
 modules/core/02-client/keeper/client.go       | 46 +++++++++++++------
 .../types/misbehaviour_handle.go              |  5 --
 .../types/misbehaviour_handle_test.go         | 17 ++++++-
 .../07-tendermint/types/store.go              |  5 +-
 .../07-tendermint/types/update_test.go        |  6 ++-
 5 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 903357f8dfa..5891f7877e0 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -66,9 +66,10 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
-	var consensusHeight exported.Height
 	eventType := types.EventTypeUpdateClient
 
+	// Cache the context to ensure that any writes in CheckHeaderAndUpdateState are only written if
+	// the update is successful and the update is not evidence of misbehaviour.
 	cacheCtx, writeFn := ctx.CacheContext()
 	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(cacheCtx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
@@ -76,7 +77,10 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 	}
 
 	// emit the full header in events
-	var headerStr string
+	var (
+		headerStr       string
+		consensusHeight exported.Height
+	)
 	if header != nil {
 		// Marshal the Header as an Any and encode the resulting bytes to hex.
 		// This prevents the event value from containing invalid UTF-8 characters
@@ -106,11 +110,36 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		}
 
 		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
+
+		defer func() {
+			telemetry.IncrCounterWithLabels(
+				[]string{"ibc", "client", "update"},
+				1,
+				[]metrics.Label{
+					telemetry.NewLabel("client-type", clientState.ClientType()),
+					telemetry.NewLabel("client-id", clientID),
+					telemetry.NewLabel("update-type", "msg"),
+				},
+			)
+		}()
+
 	} else {
 		// set eventType to SubmitMisbehaviour
 		eventType = types.EventTypeSubmitMisbehaviour
 
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID, "height", header.GetHeight().String())
+
+		defer func() {
+			telemetry.IncrCounterWithLabels(
+				[]string{"ibc", "client", "misbehaviour"},
+				1,
+				[]metrics.Label{
+					telemetry.NewLabel("client-type", clientState.ClientType()),
+					telemetry.NewLabel("client-id", clientID),
+					telemetry.NewLabel("msg-type", "update"),
+				},
+			)
+		}()
 	}
 
 	// emitting events in the keeper emits for both begin block and handler client updates
@@ -123,19 +152,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 			sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
 		),
 	)
-
-	defer func() {
-		telemetry.IncrCounterWithLabels(
-			[]string{"ibc", "client", "update"},
-			1,
-			[]metrics.Label{
-				telemetry.NewLabel("client-type", clientState.ClientType()),
-				telemetry.NewLabel("client-id", clientID),
-				telemetry.NewLabel("update-type", "msg"),
-			},
-		)
-	}()
-
 	return nil
 }
 
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
index 1626b497cbd..a33db3d5fd5 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -31,11 +31,6 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", misbehaviour, &Misbehaviour{})
 	}
 
-	// If client is already frozen, return with error
-	if cs.IsFrozen() {
-		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidMisbehaviour, "client is already frozen at height %s", cs.FrozenHeight)
-	}
-
 	// 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 tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) {
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
index cc719016be3..4d52695f0ae 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
@@ -79,6 +79,21 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 			suite.now,
 			true,
 		},
+		{
+			"valid time misbehaviour header 1 stricly less than header 2",
+			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			types.NewConsensusState(suite.now, commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), bothValsHash),
+			height,
+			&types.Misbehaviour{
+				Header1:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+3), height, suite.now, bothValSet, bothValSet, bothSigners),
+				Header2:  suite.chainA.CreateTMClientHeader(chainID, int64(height.RevisionHeight+1), height, suite.now.Add(time.Hour), bothValSet, bothValSet, bothSigners),
+				ClientId: chainID,
+			},
+			suite.now,
+			true,
+		},
 		{
 			"valid misbehavior at height greater than last consensusState",
 			types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
@@ -397,7 +412,7 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 
 			clientState, err := tc.clientState.CheckMisbehaviourAndUpdateState(
 				ctx,
-				suite.cdc,
+				suite.chainA.App.AppCodec(),
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, clientID), // pass in clientID prefixed clientStore
 				tc.misbehaviour,
 			)
diff --git a/modules/light-clients/07-tendermint/types/store.go b/modules/light-clients/07-tendermint/types/store.go
index cb4a99f91ae..81be3a3e8f0 100644
--- a/modules/light-clients/07-tendermint/types/store.go
+++ b/modules/light-clients/07-tendermint/types/store.go
@@ -171,6 +171,8 @@ func GetHeightFromIterationKey(iterKey []byte) exported.Height {
 	return clienttypes.NewHeight(revision, height)
 }
 
+// IterateConsensusStateAscending iterates through the consensus states in ascending order. It calls the provided
+// callback on each height, until stop=true is returned.
 func IterateConsensusStateAscending(clientStore sdk.KVStore, cb func(height exported.Height) (stop bool)) error {
 	iterator := sdk.KVStorePrefixIterator(clientStore, []byte(KeyIterateConsensusStatePrefix))
 	defer iterator.Close()
@@ -187,7 +189,8 @@ func IterateConsensusStateAscending(clientStore sdk.KVStore, cb func(height expo
 
 // GetNextConsensusState returns the lowest consensus state that is larger than the given height.
 // The Iterator returns a storetypes.Iterator which iterates from start (inclusive) to end (exclusive).
-// Thus, to get the next consensus state, we must first call iterator.Next() and then get the value.
+// If the starting height exists in store, we need to call iterator.Next() to get the next consenus state.
+// Otherwise, the iterator is already at the next consensus state so we can call iterator.Value() immediately.
 func GetNextConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, height exported.Height) (*ConsensusState, bool) {
 	iterateStore := prefix.NewStore(clientStore, []byte(KeyIterateConsensusStatePrefix))
 	iterator := iterateStore.Iterator(bigEndianHeightBytes(height), nil)
diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 0918b27f743..45c614a654e 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -139,7 +139,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			name: "misbehaviour detection: previous consensus state time is not before header time. time monotonicity violation",
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
-				// swap header and client time to create violation
+				// create an intermediate consensus state with the same time as the newHeader to create a time violation.
+				// header time is after client time
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus5.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now
@@ -156,7 +157,8 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			name: "misbehaviour detection: next consensus state time is not after header time. time monotonicity violation",
 			setup: func(suite *TendermintTestSuite) {
 				clientState = types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
-				// swap header and client time to create violation
+				// create the next consensus state with the same time as the intermediate newHeader to create a time violation.
+				// header time is after clientTime
 				consensusState = types.NewConsensusState(suite.clientTime, commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), suite.valsHash)
 				newHeader = suite.chainA.CreateTMClientHeader(chainID, int64(heightPlus1.RevisionHeight), height, suite.headerTime, suite.valSet, suite.valSet, signers)
 				currentTime = suite.now

From f63ae679884a3e5c33fa8bebd5f2f47db35034e5 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Wed, 28 Apr 2021 16:20:37 -0400
Subject: [PATCH 18/24] CHANGELOG

---
 CHANGELOG.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a4fbd915164..6b782fc076e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -51,6 +51,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### State Machine Breaking
 
 * (modules/light-clients/07-tendermint) [\#99](https://github.com/cosmos/ibc-go/pull/99) Enforce maximum chain-id length for tendermint client. 
+* (modules/light-clients/07-tendermint) [\#141](https://github.com/cosmos/ibc-go/pull/141) Allow a new form of misbehaviour that proves counterparty chain breaks time monotonicity, automatically enforce monotonicity in UpdateClient and freeze client if monotonicity is broken.
+* (modules/light-clients/07-tendermint) [\#141](https://github.com/cosmos/ibc-go/pull/141) Freeze the client if there's a conflicting header submitted for an existing consensus state.
 * (modules/core/02-client) [\#8405](https://github.com/cosmos/cosmos-sdk/pull/8405) Refactor IBC client update governance proposals to use a substitute client to update a frozen or expired client.
 * (modules/core/02-client) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added.
 
@@ -58,6 +60,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (modules/core/04-channel) [\#7949](https://github.com/cosmos/cosmos-sdk/issues/7949) Standardized channel `Acknowledgement` moved to its own file. Codec registration redundancy removed.
 * (modules/light-clients/07-tendermint) [\#125](https://github.com/cosmos/ibc-go/pull/125) Implement efficient iteration of consensus states and pruning of earliest expired consensus state on UpdateClient.
+* (modules/light-clients/07-tendermint) [\#141](https://github.com/cosmos/ibc-go/pull/141) Return early in case there's a duplicate update call to save Gas.
+
 
 ## IBC in the Cosmos SDK Repository
 

From c3ac1ebd634404404a02a266fa5eb5df36ca394e Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Thu, 29 Apr 2021 12:06:56 -0400
Subject: [PATCH 19/24] update updateclient test

---
 modules/core/02-client/keeper/client_test.go | 238 ++++++++-----------
 testing/chain.go                             |  12 +-
 2 files changed, 114 insertions(+), 136 deletions(-)

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index b51f70f8fe2..04e8527660f 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -42,216 +42,186 @@ func (suite *KeeperTestSuite) TestCreateClient() {
 }
 
 func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
-	// Must create header creation functions since suite.header gets recreated on each test case
-	createFutureUpdateFn := func(s *KeeperTestSuite) *ibctmtypes.Header {
-		heightPlus3 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()+3)
-		height := suite.header.GetHeight().(clienttypes.Height)
+	var (
+		path         *ibctesting.Path
+		updateHeader *ibctmtypes.Header
+	)
 
-		return suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus3.RevisionHeight), height, suite.header.Header.Time.Add(time.Hour),
-			suite.valSet, suite.valSet, []tmtypes.PrivValidator{suite.privVal})
+	// Must create header creation functions since suite.header gets recreated on each test case
+	createFutureUpdateFn := func(trustedHeight clienttypes.Height) *ibctmtypes.Header {
+		header, err := suite.chainA.ConstructUpdateTMClientHeaderWithTrustedHeight(path.EndpointB.Chain, path.EndpointA.ClientID, trustedHeight)
+		suite.Require().NoError(err)
+		return header
 	}
-	createPastUpdateFn := func(s *KeeperTestSuite) *ibctmtypes.Header {
-		heightMinus2 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()-2)
-		heightMinus4 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()-4)
+	createPastUpdateFn := func(fillHeight, trustedHeight clienttypes.Height) *ibctmtypes.Header {
+		consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, trustedHeight)
+		suite.Require().True(found)
 
-		return suite.chainA.CreateTMClientHeader(testChainID, int64(heightMinus2.RevisionHeight), heightMinus4, suite.header.Header.Time,
-			suite.valSet, suite.valSet, []tmtypes.PrivValidator{suite.privVal})
+		return suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(fillHeight.RevisionHeight), trustedHeight, consState.(*ibctmtypes.ConsensusState).Timestamp.Add(time.Second*5),
+			suite.chainB.Vals, suite.chainB.Vals, suite.chainB.Signers)
 	}
-	var (
-		updateHeader *ibctmtypes.Header
-		clientState  *ibctmtypes.ClientState
-		clientID     string
-		err          error
-	)
 
 	cases := []struct {
-		name     string
-		malleate func() error
-		expPass  bool
-		freeze   bool // only true if update freezes the client to a new frozen height. false if client is already frozen and header is valid past update
+		name      string
+		malleate  func()
+		expPass   bool
+		expFreeze bool
 	}{
-		{"valid update", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+		{"valid update", func() {
+			clientState := path.EndpointA.GetClientState().(*ibctmtypes.ClientState)
+			trustHeight := clientState.GetLatestHeight().(types.Height)
 
 			// store intermediate consensus state to check that trustedHeight does not need to be highest consensus state before header height
-			incrementedClientHeight := testClientHeight.Increment().(types.Height)
-			intermediateConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.now.Add(time.Minute),
-				NextValidatorsHash: suite.valSetHash,
-			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, incrementedClientHeight, intermediateConsState)
+			path.EndpointA.UpdateClient()
 
-			clientState.LatestHeight = incrementedClientHeight
-			suite.keeper.SetClientState(suite.ctx, clientID, clientState)
-
-			updateHeader = createFutureUpdateFn(suite)
-			return err
+			updateHeader = createFutureUpdateFn(trustHeight)
 		}, true, false},
-		{"valid past update", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
+		{"valid past update", func() {
+			clientState := path.EndpointA.GetClientState()
+			trustedHeight := clientState.GetLatestHeight().(types.Height)
 
-			height1 := types.NewHeight(0, 1)
+			currHeight := suite.chainB.CurrentHeader.Height
+			fillHeight := types.NewHeight(clientState.GetLatestHeight().GetRevisionNumber(), uint64(currHeight))
 
-			// store previous consensus state
-			prevConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.past,
-				NextValidatorsHash: suite.valSetHash,
-			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
+			// commit a couple blocks to allow client to fill in gaps
+			suite.coordinator.CommitBlock(suite.chainB) // this height is not filled in yet
+			suite.coordinator.CommitBlock(suite.chainB) // this height is filled in by the update below
 
-			height2 := types.NewHeight(0, 2)
+			path.EndpointA.UpdateClient()
 
-			// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
-			intermediateConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.past.Add(time.Minute),
-				NextValidatorsHash: suite.valSetHash,
-			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
+			// ensure fill height not set
+			_, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, fillHeight)
+			suite.Require().False(found)
 
 			// updateHeader will fill in consensus state between prevConsState and suite.consState
 			// clientState should not be updated
-			updateHeader = createPastUpdateFn(suite)
-			return nil
+			updateHeader = createPastUpdateFn(fillHeight, trustedHeight)
 		}, true, false},
-		{"valid duplicate update", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
+		{"valid duplicate update", func() {
+			clientID := path.EndpointA.ClientID
 
 			height1 := types.NewHeight(0, 1)
 
 			// store previous consensus state
 			prevConsState := &ibctmtypes.ConsensusState{
 				Timestamp:          suite.past,
-				NextValidatorsHash: suite.valSetHash,
+				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
-
-			height2 := types.NewHeight(0, 2)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, height1, prevConsState)
 
-			// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
-			intermediateConsState := &ibctmtypes.ConsensusState{
+			height5 := types.NewHeight(0, 5)
+			// store next consensus state to check that trustedHeight does not need to be hightest consensus state before header height
+			nextConsState := &ibctmtypes.ConsensusState{
 				Timestamp:          suite.past.Add(time.Minute),
-				NextValidatorsHash: suite.valSetHash,
+				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, height5, nextConsState)
 
+			height3 := types.NewHeight(0, 3)
 			// updateHeader will fill in consensus state between prevConsState and suite.consState
 			// clientState should not be updated
-			updateHeader = createPastUpdateFn(suite)
+			updateHeader = createPastUpdateFn(height3, height1)
 			// set updateHeader's consensus state in store to create duplicate UpdateClient scenario
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), updateHeader.ConsensusState())
-			return nil
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, updateHeader.GetHeight(), updateHeader.ConsensusState())
 		}, true, false},
-		{"misbehaviour detection: conflicting header", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
+		{"misbehaviour detection: conflicting header", func() {
+			clientID := path.EndpointA.ClientID
 
 			height1 := types.NewHeight(0, 1)
-
 			// store previous consensus state
 			prevConsState := &ibctmtypes.ConsensusState{
 				Timestamp:          suite.past,
-				NextValidatorsHash: suite.valSetHash,
+				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, height1, prevConsState)
 
-			height2 := types.NewHeight(0, 2)
-
-			// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
-			intermediateConsState := &ibctmtypes.ConsensusState{
+			height5 := types.NewHeight(0, 5)
+			// store next consensus state to check that trustedHeight does not need to be hightest consensus state before header height
+			nextConsState := &ibctmtypes.ConsensusState{
 				Timestamp:          suite.past.Add(time.Minute),
-				NextValidatorsHash: suite.valSetHash,
+				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, height5, nextConsState)
 
+			height3 := types.NewHeight(0, 3)
 			// updateHeader will fill in consensus state between prevConsState and suite.consState
 			// clientState should not be updated
-			updateHeader = createPastUpdateFn(suite)
+			updateHeader = createPastUpdateFn(height3, height1)
 			// set conflicting consensus state in store to create misbehaviour scenario
 			conflictConsState := updateHeader.ConsensusState()
 			conflictConsState.Root = commitmenttypes.NewMerkleRoot([]byte("conflicting apphash"))
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight(), conflictConsState)
-			return nil
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, updateHeader.GetHeight(), conflictConsState)
 		}, true, true},
-		{"misbehaviour detection: monotonic time violation", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
+		{"misbehaviour detection: monotonic time violation", func() {
+			clientState := path.EndpointA.GetClientState().(*ibctmtypes.ClientState)
+			clientID := path.EndpointA.ClientID
+			trustedHeight := clientState.GetLatestHeight().(types.Height)
 
 			// store intermediate consensus state at a time greater than updateHeader time
 			// this will break time monotonicity
-			incrementedClientHeight := testClientHeight.Increment().(types.Height)
+			incrementedClientHeight := clientState.GetLatestHeight().Increment().(types.Height)
 			intermediateConsState := &ibctmtypes.ConsensusState{
 				Timestamp:          suite.header.Header.Time.Add(2 * time.Hour),
-				NextValidatorsHash: suite.valSetHash,
+				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
-			suite.keeper.SetClientConsensusState(suite.ctx, clientID, incrementedClientHeight, intermediateConsState)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, incrementedClientHeight, intermediateConsState)
 			// set iteration key
 			clientStore := suite.keeper.ClientStore(suite.ctx, clientID)
 			ibctmtypes.SetIterationKey(clientStore, incrementedClientHeight)
 
 			clientState.LatestHeight = incrementedClientHeight
-			suite.keeper.SetClientState(suite.ctx, clientID, clientState)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), clientID, clientState)
 
-			updateHeader = createFutureUpdateFn(suite)
-			return err
+			updateHeader = createFutureUpdateFn(trustedHeight)
 		}, true, true},
-		{"client state not found", func() error {
-			updateHeader = createFutureUpdateFn(suite)
+		{"client state not found", func() {
+			updateHeader = createFutureUpdateFn(path.EndpointA.GetClientState().GetLatestHeight().(types.Height))
 
-			return nil
+			path.EndpointA.ClientID = ibctesting.InvalidID
 		}, false, false},
-		{"consensus state not found", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
-			updateHeader = createFutureUpdateFn(suite)
-
-			return nil
+		{"consensus state not found", func() {
+			clientState := path.EndpointA.GetClientState()
+			tmClient, ok := clientState.(*ibctmtypes.ClientState)
+			suite.Require().True(ok)
+			tmClient.LatestHeight = tmClient.LatestHeight.Increment().(types.Height)
+
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState)
+			updateHeader = createFutureUpdateFn(clientState.GetLatestHeight().(types.Height))
 		}, false, false},
-		{"frozen client before update", func() error {
-			clientState = &ibctmtypes.ClientState{FrozenHeight: types.NewHeight(0, 1), LatestHeight: testClientHeight}
-			suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
-			updateHeader = createFutureUpdateFn(suite)
-
-			return nil
+		{"client is not active", func() {
+			clientState := path.EndpointA.GetClientState().(*ibctmtypes.ClientState)
+			clientState.FrozenHeight = types.NewHeight(0, 1)
+			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState)
+			updateHeader = createFutureUpdateFn(clientState.GetLatestHeight().(types.Height))
 		}, false, false},
-		{"invalid header", func() error {
-			clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-			_, err := suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-			suite.Require().NoError(err)
-			updateHeader = createPastUpdateFn(suite)
-
-			return nil
+		{"invalid header", func() {
+			updateHeader = createFutureUpdateFn(path.EndpointA.GetClientState().GetLatestHeight().(types.Height))
+			updateHeader.TrustedHeight = updateHeader.TrustedHeight.Increment().(types.Height)
 		}, false, false},
 	}
 
-	for i, tc := range cases {
+	for _, tc := range cases {
 		tc := tc
-		i := i
 		suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
 			suite.SetupTest()
-			clientID = testClientID // must be explicitly changed
+			path = ibctesting.NewPath(suite.chainA, suite.chainB)
+			suite.coordinator.SetupClients(path)
 
-			err := tc.malleate()
-			suite.Require().NoError(err)
+			tc.malleate()
 
-			suite.ctx = suite.ctx.WithBlockTime(updateHeader.Header.Time.Add(time.Minute))
+			var clientState exported.ClientState
+			if tc.expPass {
+				clientState = path.EndpointA.GetClientState()
+			}
 
-			err = suite.keeper.UpdateClient(suite.ctx, clientID, updateHeader)
+			err := suite.chainA.App.GetIBCKeeper().ClientKeeper.UpdateClient(suite.chainA.GetContext(), path.EndpointA.ClientID, updateHeader)
 
 			if tc.expPass {
 				suite.Require().NoError(err, err)
 
-				newClientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
-				suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
+				newClientState := path.EndpointA.GetClientState()
 
-				// If the update freezes the client, check that client was frozen to update header's height.
-				// Otherwise check that consensus state is stored as expected.
-				if tc.freeze {
+				if tc.expFreeze {
 					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
 					suite.Require().Equal(newClientState.GetFrozenHeight(), updateHeader.GetHeight(), "client frozen at wrong height")
 				} else {
@@ -261,8 +231,8 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 						NextValidatorsHash: updateHeader.Header.NextValidatorsHash,
 					}
 
-					consensusState, found := suite.keeper.GetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight())
-					suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
+					consensusState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, updateHeader.GetHeight())
+					suite.Require().True(found)
 
 					// Determine if clientState should be updated or not
 					if updateHeader.GetHeight().GT(clientState.GetLatestHeight()) {
@@ -273,11 +243,11 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 						suite.Require().Equal(clientState.GetLatestHeight(), newClientState.GetLatestHeight(), "client state height updated for past header")
 					}
 
-					suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
+					suite.Require().NoError(err)
 					suite.Require().Equal(expConsensusState, consensusState, "consensus state should have been updated on case %s", tc.name)
 				}
 			} else {
-				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
+				suite.Require().Error(err)
 			}
 		})
 	}
@@ -536,7 +506,7 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 				// store intermediate consensus state to check that trustedHeight does not need to be highest consensus state before header height
 				intermediateConsState := &ibctmtypes.ConsensusState{
 					Timestamp:          suite.now.Add(time.Minute),
-					NextValidatorsHash: suite.valSetHash,
+					NextValidatorsHash: suite.chainB.Vals.Hash(),
 				}
 				suite.keeper.SetClientConsensusState(suite.ctx, clientID, heightPlus3, intermediateConsState)
 
diff --git a/testing/chain.go b/testing/chain.go
index 19ee21839f6..92228eed849 100644
--- a/testing/chain.go
+++ b/testing/chain.go
@@ -313,9 +313,17 @@ func (chain *TestChain) GetPrefix() commitmenttypes.MerklePrefix {
 // ConstructUpdateTMClientHeader will construct a valid 07-tendermint Header to update the
 // light client on the source chain.
 func (chain *TestChain) ConstructUpdateTMClientHeader(counterparty *TestChain, clientID string) (*ibctmtypes.Header, error) {
+	return chain.ConstructUpdateTMClientHeaderWithTrustedHeight(counterparty, clientID, clienttypes.ZeroHeight())
+}
+
+// ConstructUpdateTMClientHeader will construct a valid 07-tendermint Header to update the
+// light client on the source chain.
+func (chain *TestChain) ConstructUpdateTMClientHeaderWithTrustedHeight(counterparty *TestChain, clientID string, trustedHeight clienttypes.Height) (*ibctmtypes.Header, error) {
 	header := counterparty.LastHeader
-	// Relayer must query for LatestHeight on client to get TrustedHeight
-	trustedHeight := chain.GetClientState(clientID).GetLatestHeight().(clienttypes.Height)
+	// Relayer must query for LatestHeight on client to get TrustedHeight if the trusted height is not set
+	if trustedHeight.IsZero() {
+		trustedHeight = chain.GetClientState(clientID).GetLatestHeight().(clienttypes.Height)
+	}
 	var (
 		tmTrustedVals *tmtypes.ValidatorSet
 		ok            bool

From 75bf94adaaa3bb28f851c64dad4575030b5e0645 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Colin=20Axn=C3=A9r?=
 <25233464+colin-axner@users.noreply.github.com>
Date: Fri, 30 Apr 2021 12:01:59 +0200
Subject: [PATCH 20/24] bump tendermint to 0.34.10

---
 go.mod | 4 ++--
 go.sum | 9 +++++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/go.mod b/go.mod
index f9c9b147f21..7e3ed90ff0a 100644
--- a/go.mod
+++ b/go.mod
@@ -18,9 +18,9 @@ require (
 	github.com/spf13/cobra v1.1.3
 	github.com/spf13/viper v1.7.1
 	github.com/stretchr/testify v1.7.0
-	github.com/tendermint/tendermint v0.34.8
+	github.com/tendermint/tendermint v0.34.10
 	github.com/tendermint/tm-db v0.6.4
 	google.golang.org/genproto v0.0.0-20210114201628-6edceaf6022f
-	google.golang.org/grpc v1.36.0
+	google.golang.org/grpc v1.37.0
 	google.golang.org/protobuf v1.25.0
 )
diff --git a/go.sum b/go.sum
index 8461dc8aaee..0bc14202e10 100644
--- a/go.sum
+++ b/go.sum
@@ -186,6 +186,7 @@ github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymF
 github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98=
 github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
+github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
 github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
 github.com/ethereum/go-ethereum v1.9.23/go.mod h1:JIfVb6esrqALTExdz9hRYvrP0xBDf6wCncIu1hNwHpM=
 github.com/facebookgo/ensure v0.0.0-20160127193407-b4ab57deab51 h1:0JZ+dUmQeA8IIVUMzysrX4/AKuQwWhV2dYQuPZdvdSQ=
@@ -291,6 +292,8 @@ github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/
 github.com/google/gofuzz v1.1.1-0.20200604201612-c04b05f3adfa h1:Q75Upo5UN4JbPFURXZ8nLKYUvF85dyFRop/vQ0Rv+64=
 github.com/google/gofuzz v1.1.1-0.20200604201612-c04b05f3adfa/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs=
+github.com/google/orderedcode v0.0.1 h1:UzfcAexk9Vhv8+9pNOgRu41f16lHq725vPwnSeiG/Us=
+github.com/google/orderedcode v0.0.1/go.mod h1:iVyU4/qPKHY5h/wSd6rZZCDcLJNxiWO6dvsYES2Sb20=
 github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
@@ -682,8 +685,9 @@ github.com/tendermint/go-amino v0.16.0/go.mod h1:TQU0M1i/ImAo+tYpZi73AU3V/dKeCoM
 github.com/tendermint/tendermint v0.34.0-rc4/go.mod h1:yotsojf2C1QBOw4dZrTcxbyxmPUrT4hNuOQWX9XUwB4=
 github.com/tendermint/tendermint v0.34.0-rc6/go.mod h1:ugzyZO5foutZImv0Iyx/gOFCX6mjJTgbLHTwi17VDVg=
 github.com/tendermint/tendermint v0.34.0/go.mod h1:Aj3PIipBFSNO21r+Lq3TtzQ+uKESxkbA3yo/INM4QwQ=
-github.com/tendermint/tendermint v0.34.8 h1:PMWgUx47FrNTsfhxCWzoiIlVAC1SE9+WBlnsF9oQW0I=
 github.com/tendermint/tendermint v0.34.8/go.mod h1:JVuu3V1ZexOaZG8VJMRl8lnfrGw6hEB2TVnoUwKRbss=
+github.com/tendermint/tendermint v0.34.10 h1:wBOc/It8sh/pVH9np2V5fBvRmIyFN/bUrGPx+eAHexs=
+github.com/tendermint/tendermint v0.34.10/go.mod h1:aeHL7alPh4uTBIJQ8mgFEE8VwJLXI1VD3rVOmH2Mcy0=
 github.com/tendermint/tm-db v0.6.2/go.mod h1:GYtQ67SUvATOcoY8/+x6ylk8Qo02BQyLrAs+yAcLvGI=
 github.com/tendermint/tm-db v0.6.3/go.mod h1:lfA1dL9/Y/Y8wwyPp2NMLyn5P5Ptr/gvDFNWtrCWSf8=
 github.com/tendermint/tm-db v0.6.4 h1:3N2jlnYQkXNQclQwd/eKV/NzlqPlfK21cpRRIx80XXQ=
@@ -964,8 +968,9 @@ google.golang.org/grpc v1.32.0/go.mod h1:N36X2cJ7JwdamYAgDz+s+rVMFjt3numwzf/HckM
 google.golang.org/grpc v1.33.1/go.mod h1:fr5YgcSWrqhRRxogOsw7RzIpsmvOZ6IcH4kBYTpR3n0=
 google.golang.org/grpc v1.33.2/go.mod h1:JMHMWHQWaTccqQQlmk3MJZS+GWXOdAesneDmEnv2fbc=
 google.golang.org/grpc v1.35.0/go.mod h1:qjiiYl8FncCW8feJPdyg3v6XW24KsRHe+dy9BAGRRjU=
-google.golang.org/grpc v1.36.0 h1:o1bcQ6imQMIOpdrO3SWf2z5RV72WbDwdXuK0MDlc8As=
 google.golang.org/grpc v1.36.0/go.mod h1:qjiiYl8FncCW8feJPdyg3v6XW24KsRHe+dy9BAGRRjU=
+google.golang.org/grpc v1.37.0 h1:uSZWeQJX5j11bIQ4AJoj+McDBo29cY1MCoC1wO3ts+c=
+google.golang.org/grpc v1.37.0/go.mod h1:NREThFqKR1f3iQ6oBuvc5LadQuXVGo9rkm5ZGrQdJfM=
 google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
 google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
 google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM=

From eab22f0d4f36d341b3e57e12257d8f15d7fc4b22 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 30 Apr 2021 15:50:25 -0400
Subject: [PATCH 21/24] remove caching and specific frozen height

---
 modules/core/02-client/keeper/client.go                | 10 +---------
 modules/core/02-client/keeper/client_test.go           |  5 +----
 .../07-tendermint/types/misbehaviour_handle.go         |  3 ++-
 .../07-tendermint/types/misbehaviour_handle_test.go    |  2 --
 modules/light-clients/07-tendermint/types/update.go    |  7 ++++---
 5 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index 5891f7877e0..b1220597e4a 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -68,10 +68,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	eventType := types.EventTypeUpdateClient
 
-	// Cache the context to ensure that any writes in CheckHeaderAndUpdateState are only written if
-	// the update is successful and the update is not evidence of misbehaviour.
-	cacheCtx, writeFn := ctx.CacheContext()
-	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(cacheCtx, k.cdc, k.ClientStore(ctx, clientID), header)
+	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
 	}
@@ -97,10 +94,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 	// then update was valid. Write the update state changes, and set new consensus state.
 	// Else the update was proof of misbehaviour and we must emit appropriate misbehaviour events.
 	if !newClientState.IsFrozen() {
-		// write any cached state changes from CheckHeaderAndUpdateState
-		// to store metadata in client store for new consensus state.
-		writeFn()
-
 		// if update is not misbehaviour then update the consensus state
 		// we don't set consensus state for localhost client
 		if header != nil && clientID != exported.Localhost {
@@ -122,7 +115,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 				},
 			)
 		}()
-
 	} else {
 		// set eventType to SubmitMisbehaviour
 		eventType = types.EventTypeSubmitMisbehaviour
diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
index 04e8527660f..ff059e09ea0 100644
--- a/modules/core/02-client/keeper/client_test.go
+++ b/modules/core/02-client/keeper/client_test.go
@@ -161,7 +161,7 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 			// this will break time monotonicity
 			incrementedClientHeight := clientState.GetLatestHeight().Increment().(types.Height)
 			intermediateConsState := &ibctmtypes.ConsensusState{
-				Timestamp:          suite.header.Header.Time.Add(2 * time.Hour),
+				Timestamp:          suite.coordinator.CurrentTime.Add(2 * time.Hour),
 				NextValidatorsHash: suite.chainB.Vals.Hash(),
 			}
 			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, incrementedClientHeight, intermediateConsState)
@@ -223,7 +223,6 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 
 				if tc.expFreeze {
 					suite.Require().True(newClientState.IsFrozen(), "client did not freeze after conflicting header was submitted to UpdateClient")
-					suite.Require().Equal(newClientState.GetFrozenHeight(), updateHeader.GetHeight(), "client frozen at wrong height")
 				} else {
 					expConsensusState := &ibctmtypes.ConsensusState{
 						Timestamp:          updateHeader.GetTime(),
@@ -657,8 +656,6 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
 				clientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
 				suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
 				suite.Require().True(clientState.IsFrozen(), "valid test case %d failed: %s", i, tc.name)
-				suite.Require().Equal(tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight(),
-					"valid test case %d failed: %s. Expected FrozenHeight %s got %s", tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight())
 			} else {
 				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
 			}
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
index a33db3d5fd5..dd347f2aa4b 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -20,6 +20,7 @@ import (
 // of misbehaviour.Header1
 // Similarly, consensusState2 is the trusted consensus state that corresponds
 // 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) CheckMisbehaviourAndUpdateState(
 	ctx sdk.Context,
 	cdc codec.BinaryMarshaler,
@@ -88,7 +89,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 		return nil, sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed")
 	}
 
-	cs.FrozenHeight = tmMisbehaviour.GetHeight().(clienttypes.Height)
+	cs.FrozenHeight = clienttypes.NewHeight(0, 1)
 
 	return &cs, nil
 }
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
index 4d52695f0ae..398f8b3d8e8 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go
@@ -421,8 +421,6 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() {
 				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 				suite.Require().NotNil(clientState, "valid test case %d failed: %s", i, tc.name)
 				suite.Require().True(clientState.IsFrozen(), "valid test case %d failed: %s", i, tc.name)
-				suite.Require().Equal(tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight(),
-					"valid test case %d failed: %s. Expected FrozenHeight %s got %s", tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight())
 			} else {
 				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
 				suite.Require().Nil(clientState, "invalid test case %d passed: %s", i, tc.name)
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index ce085617786..d637cae04a1 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -43,6 +43,7 @@ import (
 // UpdateClient will detect implicit misbehaviour by enforcing certain invariants on any new update call and will return a frozen client.
 // 1. Any valid update that creates a different consensus state for an already existing height is evidence of misbehaviour and will freeze client.
 // 2. Any valid update that breaks time monotonicity with respect to its neighboring consensus states is evidence of misbehaviour and will freeze client.
+// Misbehaviour sets frozen height to {0, 1} since it is only used as a boolean value (zero or non-zero).
 //
 // Pruning:
 // UpdateClient will additionally retrieve the earliest consensus state for this clientID and check if it is expired. If it is,
@@ -90,7 +91,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	consState := tmHeader.ConsensusState()
 	// Header is different from existing consensus state and also valid, so freeze the client and return
 	if conflictingHeader {
-		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
+		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
 		return &cs, consState, nil
 	}
 	// Check that consensus state timestamps are monotonic
@@ -99,13 +100,13 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// if previous consensus state exists, check consensus state time is greater than previous consensus state time
 	// if previous consensus state is not before current consensus state, freeze the client and return.
 	if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) {
-		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
+		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
 		return &cs, consState, nil
 	}
 	// if next consensus state exists, check consensus state time is less than next consensus state time
 	// if next consensus state is not after current consensus state, freeze the client and return.
 	if nextOk && !nextCons.Timestamp.After(consState.Timestamp) {
-		cs.FrozenHeight = header.GetHeight().(clienttypes.Height)
+		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
 		return &cs, consState, nil
 	}
 

From c7f8bd29a94fd066d56b874773a4189e32dcd26c Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 30 Apr 2021 16:00:14 -0400
Subject: [PATCH 22/24] document in go code

---
 modules/core/02-client/keeper/client.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
index b1220597e4a..c44d6c70f6f 100644
--- a/modules/core/02-client/keeper/client.go
+++ b/modules/core/02-client/keeper/client.go
@@ -68,6 +68,8 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 	eventType := types.EventTypeUpdateClient
 
+	// Any writes made in CheckHeaderAndUpdateState are persisted on both valid updates and misbehaviour updates.
+	// Light client implementations are responsible for writing the correct metadata (if any) in either case.
 	newClientState, newConsensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)

From 76e932a7131f26c0120bf061f616eb3c9a550fd0 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Mon, 3 May 2021 10:40:01 -0400
Subject: [PATCH 23/24] DRY FrozenHeight

---
 modules/light-clients/07-tendermint/types/misbehaviour.go   | 3 +++
 .../07-tendermint/types/misbehaviour_handle.go              | 2 +-
 modules/light-clients/07-tendermint/types/update.go         | 6 +++---
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/modules/light-clients/07-tendermint/types/misbehaviour.go b/modules/light-clients/07-tendermint/types/misbehaviour.go
index b14593e1635..9a5ef74c948 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour.go
@@ -14,6 +14,9 @@ import (
 
 var _ exported.Misbehaviour = &Misbehaviour{}
 
+// Use the same FrozenHeight for all misbehaviour
+var FrozenHeight = clienttypes.NewHeight(0, 1)
+
 // NewMisbehaviour creates a new Misbehaviour instance.
 func NewMisbehaviour(clientID string, header1, header2 *Header) *Misbehaviour {
 	return &Misbehaviour{
diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
index dd347f2aa4b..f86ca12a093 100644
--- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -89,7 +89,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 		return nil, sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed")
 	}
 
-	cs.FrozenHeight = clienttypes.NewHeight(0, 1)
+	cs.FrozenHeight = FrozenHeight
 
 	return &cs, nil
 }
diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
index d637cae04a1..6333a9998d6 100644
--- a/modules/light-clients/07-tendermint/types/update.go
+++ b/modules/light-clients/07-tendermint/types/update.go
@@ -91,7 +91,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	consState := tmHeader.ConsensusState()
 	// Header is different from existing consensus state and also valid, so freeze the client and return
 	if conflictingHeader {
-		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
+		cs.FrozenHeight = FrozenHeight
 		return &cs, consState, nil
 	}
 	// Check that consensus state timestamps are monotonic
@@ -100,13 +100,13 @@ func (cs ClientState) CheckHeaderAndUpdateState(
 	// if previous consensus state exists, check consensus state time is greater than previous consensus state time
 	// if previous consensus state is not before current consensus state, freeze the client and return.
 	if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) {
-		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
+		cs.FrozenHeight = FrozenHeight
 		return &cs, consState, nil
 	}
 	// if next consensus state exists, check consensus state time is less than next consensus state time
 	// if next consensus state is not after current consensus state, freeze the client and return.
 	if nextOk && !nextCons.Timestamp.After(consState.Timestamp) {
-		cs.FrozenHeight = clienttypes.NewHeight(0, 1)
+		cs.FrozenHeight = FrozenHeight
 		return &cs, consState, nil
 	}
 

From 2f90b44ee6f3df42fdc58f1246572d3665be4aff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Colin=20Axn=C3=A9r?=
 <25233464+colin-axner@users.noreply.github.com>
Date: Wed, 5 May 2021 12:48:03 +0200
Subject: [PATCH 24/24] fix build

---
 modules/light-clients/07-tendermint/types/update_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go
index 45c614a654e..95a159ef97c 100644
--- a/modules/light-clients/07-tendermint/types/update_test.go
+++ b/modules/light-clients/07-tendermint/types/update_test.go
@@ -347,7 +347,7 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {
 			if tc.expPass {
 				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
 
-				suite.Require().Equal(tc.expFrozen, newClientState.IsFrozen(), "client state status is unexpected after update")
+				suite.Require().Equal(tc.expFrozen, !newClientState.(*types.ClientState).FrozenHeight.IsZero(), "client state status is unexpected after update")
 
 				// further writes only happen if update is not misbehaviour
 				if !tc.expFrozen {