Time Monotonicity Enforcement (#141)

* implement update client fix and start changing tests

* fix bug and write identical case test

* write misbehaviour detection tests

* add misbehaviour events to UpdateClient

* fix client keeper and write tests

* add Freeze to ClientState interface

* add cache context and fix events

* Update modules/light-clients/07-tendermint/types/update.go

Co-authored-by: Zarko Milosevic <[email protected]>

* address colin comments

* freeze entire client on misbehaviour

* add time misbehaviour and tests

* enforce trusted height less than current height in header.ValidateBasic

* cleanup and tests

* fix print statement

* enforce monotonicity in update

* add docs and remove unnecessary interface function

* first round of review comments

* CHANGELOG

* update updateclient test

* bump tendermint to 0.34.10

* remove caching and specific frozen height

* document in go code

* DRY FrozenHeight

* fix build

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Zarko Milosevic <[email protected]>

CHANGELOG.md

@@ -52,6 +52,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.
 
@@ -60,6 +62,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/core/04-channel) [\#144](https://github.com/cosmos/ibc-go/pull/144) Introduced a `packet_data_hex` attribute to emit the hex-encoded packet data in events. This allows for raw binary (proto-encoded message) to be sent over events and decoded correctly on relayer. Original `packet_data` is DEPRECATED. All relayers and IBC event consumers are encouraged to switch to `packet_data_hex` as soon as possible.
 * (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
 

modules/core/02-client/keeper/client.go

@@ -67,58 +67,86 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot update client (%s) with status %s", clientID, status)
 	}
 
-	clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, clientStore, header)
+	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, clientStore, header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
 	}
 
-	k.SetClientState(ctx, clientID, clientState)
-
-	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)
-	}
-
-	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"),
-			},
-		)
-	}()
-
 	// 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
 		// 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()
 
 	}
 
+	// 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 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 status := newClientState.Status(ctx, clientStore, k.cdc); status != exported.Frozen {
+		// 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 {
+			k.SetClientConsensusState(ctx, clientID, header.GetHeight(), newConsensusState)
+		} else {
+			consensusHeight = types.GetSelfHeight(ctx)
+		}
+
+		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
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
-			types.EventTypeUpdateClient,
+			eventType,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
 			sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
 			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
 			sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
 		),
 	)
-
 	return nil
 }
 
@@ -186,6 +214,10 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
 		return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot process misbehaviour for client (%s) with status %s", misbehaviour.GetClientID(), status)
 	}
 
+	if err := misbehaviour.ValidateBasic(); err != nil {
+		return err
+	}
+
 	clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, misbehaviour)
 	if err != nil {
 		return err