From 9bf435ae3e15f4ec043ae30e23f67f18a8e69804 Mon Sep 17 00:00:00 2001 From: Sean King Date: Sun, 13 Mar 2022 16:53:47 +0100 Subject: [PATCH 1/3] refactor: Adding CheckForMisbehaviour & UpdateStateOnMisbehavior --- .../07-tendermint/types/update.go | 102 +++++++++++------- 1 file changed, 66 insertions(+), 36 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 34568bd772d..7218ac419b0 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -60,22 +60,6 @@ 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 conflictingHeader bool - 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()) { - 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. - conflictingHeader = true - } - // get consensus state from clientStore trustedConsState, err := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight) if err != nil { @@ -88,26 +72,9 @@ func (cs ClientState) CheckHeaderAndUpdateState( return nil, nil, err } - consState := tmHeader.ConsensusState() - // Header is different from existing consensus state and also valid, so freeze the client and return - if conflictingHeader { - cs.FrozenHeight = FrozenHeight - 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.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 = FrozenHeight - return &cs, consState, nil + foundMisbehaviour := cs.CheckForMisbehaviour(ctx, cdc, clientStore, header) + if foundMisbehaviour { + return cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore) } // Check the earliest consensus state to see if it is expired, if so then set the prune height @@ -283,6 +250,69 @@ func (cs *ClientState) ValidateClientMessage( return nil } +// TODO: comment +// CheckForMisbehaviour +func (cs ClientState) CheckForMisbehaviour( + ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, + msg exported.ClientMessage, +) bool { + switch msg.(type) { + case *Header: + tmHeader := msg.(*Header) + consState := tmHeader.ConsensusState() + + // 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. + prevConsState, _ := GetConsensusState(clientStore, cdc, tmHeader.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()) { + return false + } + + // A consensus state already exists for this height, but it does not match the provided header. + // The assumption is that Header has already been validated. Thus we can return true as misbehaviour is present + return true + } + + // Check that consensus state timestamps are monotonic + prevCons, prevOk := GetPreviousConsensusState(clientStore, cdc, tmHeader.GetHeight()) + nextCons, nextOk := GetNextConsensusState(clientStore, cdc, tmHeader.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 return true + if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) { + return true + } + // 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 return true + if nextOk && !nextCons.Timestamp.After(consState.Timestamp) { + return true + } + case *Misbehaviour: + } + + return false +} + +// UpdateStateOnMisbehaviour updates state upon misbehaviour. This method should only be called on misbehaviour +// as it does not perform any misbehaviour checks. +func (cs ClientState) UpdateStateOnMisbehaviour( + _ sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, +) (*ClientState, exported.ConsensusState, error) { + // get latest consensus state from clientStore to check for expiry + consState, err := GetConsensusState(clientStore, cdc, cs.GetLatestHeight()) + if err != nil { + // if the client state does not have an associated consensus state for its latest height + // then it must be expired + return &cs, nil, err + } + + cs.FrozenHeight = FrozenHeight + return &cs, consState, nil +} + // update the consensus state from a new header and set processed time metadata func update(ctx sdk.Context, clientStore sdk.KVStore, clientState *ClientState, header *Header) (*ClientState, *ConsensusState) { height := header.GetHeight().(clienttypes.Height) From c9926d51da6e8032779e8b1db97a3db2be122e1d Mon Sep 17 00:00:00 2001 From: Sean King Date: Sun, 13 Mar 2022 18:07:48 +0100 Subject: [PATCH 2/3] refactor: adding CheckForMisbehaviour for Misbehaviour type --- .../types/misbehaviour_handle.go | 32 ++--------- .../07-tendermint/types/update.go | 53 ++++++++++++------- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go index 5d0fc3e10e5..54f9508d37b 100644 --- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go @@ -1,7 +1,6 @@ package types import ( - "bytes" "time" "github.com/cosmos/cosmos-sdk/codec" @@ -32,39 +31,16 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", misbehaviour, &Misbehaviour{}) } - // The status of the client is checked in 02-client - - // 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()) { - 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") - } + foundMisbehaviour, err := cs.CheckForMisbehaviour(ctx, cdc, clientStore, tmMisbehaviour) + if foundMisbehaviour { + return nil, err } if err := cs.ValidateClientMessage(ctx, clientStore, cdc, nil, misbehaviour, ctx.BlockTime()); err != nil { return nil, err } - cs.FrozenHeight = FrozenHeight - - return &cs, nil + return cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore), nil } // checkMisbehaviourHeader checks that a Header in Misbehaviour is valid misbehaviour given diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 7218ac419b0..89f964a3afa 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -72,9 +72,9 @@ func (cs ClientState) CheckHeaderAndUpdateState( return nil, nil, err } - foundMisbehaviour := cs.CheckForMisbehaviour(ctx, cdc, clientStore, header) + foundMisbehaviour, err := cs.CheckForMisbehaviour(ctx, cdc, clientStore, header) if foundMisbehaviour { - return cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore) + return cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore), nil, err } // Check the earliest consensus state to see if it is expired, if so then set the prune height @@ -255,7 +255,7 @@ func (cs *ClientState) ValidateClientMessage( func (cs ClientState) CheckForMisbehaviour( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.ClientMessage, -) bool { +) (bool, error) { switch msg.(type) { case *Header: tmHeader := msg.(*Header) @@ -269,12 +269,12 @@ func (cs ClientState) CheckForMisbehaviour( // 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 false + return false, nil } // A consensus state already exists for this height, but it does not match the provided header. // The assumption is that Header has already been validated. Thus we can return true as misbehaviour is present - return true + return true, nil } // Check that consensus state timestamps are monotonic @@ -283,34 +283,51 @@ func (cs ClientState) CheckForMisbehaviour( // 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 return true if prevOk && !prevCons.Timestamp.Before(consState.Timestamp) { - return true + return true, 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 return true if nextOk && !nextCons.Timestamp.After(consState.Timestamp) { - return true + return true, nil } case *Misbehaviour: + tmMisbehaviour := msg.(*Misbehaviour) + + // if heights are equal check that this is valid misbehaviour of a fork + // otherwise if heights are unequal check that this is valid misbehavior of BFT time violation + if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) { + blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID) + if err != nil { + return true, sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour") + } + blockID2, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header2.SignedHeader.Commit.BlockID) + if err != nil { + return true, 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 true, 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 true, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers are not at same height and are monotonically increasing") + } + } } - return false + return false, nil } // UpdateStateOnMisbehaviour updates state upon misbehaviour. This method should only be called on misbehaviour // as it does not perform any misbehaviour checks. func (cs ClientState) UpdateStateOnMisbehaviour( _ sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, -) (*ClientState, exported.ConsensusState, error) { - // get latest consensus state from clientStore to check for expiry - consState, err := GetConsensusState(clientStore, cdc, cs.GetLatestHeight()) - if err != nil { - // if the client state does not have an associated consensus state for its latest height - // then it must be expired - return &cs, nil, err - } - +) *ClientState { cs.FrozenHeight = FrozenHeight - return &cs, consState, nil + return &cs } // update the consensus state from a new header and set processed time metadata From 579e9f0093ded9491c55edc0434f5bffcb7e644a Mon Sep 17 00:00:00 2001 From: Sean King Date: Sun, 13 Mar 2022 18:13:52 +0100 Subject: [PATCH 3/3] nit: comment --- modules/light-clients/07-tendermint/types/update.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 89f964a3afa..7384fbf2a67 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -250,8 +250,7 @@ func (cs *ClientState) ValidateClientMessage( return nil } -// TODO: comment -// CheckForMisbehaviour +// CheckForMisbehaviour returns true with an optional error if a misbehaviour is found for the given Header or Misbehaviour type func (cs ClientState) CheckForMisbehaviour( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.ClientMessage,