Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace CheckHeaderAndUpdateState with new ClientState functions #1208

Merged
merged 4 commits into from
Apr 1, 2022
Merged

Replace CheckHeaderAndUpdateState with new ClientState functions #1208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ce9259
refactor: replace CheckHeaderAndUpdateState with VerifyClientMessage,…
colin-axner Mar 31, 2022
11c1d16
add changelog entry
colin-axner Mar 31, 2022
a617074
Merge branch '02-client-refactor' of github.com:cosmos/ibc-go into co…
colin-axner Mar 31, 2022
c68333e
fix tests
colin-axner Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### State Machine Breaking

### Improvements

* (02-client) [\#1208](https://github.com/cosmos/ibc-go/pull/1208) Replace `CheckHeaderAndUpdateState` usage in 02-client with calls to `VerifyClientMessage`, `CheckForMisbehaviour`, `UpdateStateOnMisbehaviour` and `UpdateState`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we do a changelog audit before merging the feat branch? Some inconsistencies with (02-client) and (modules/core/02-client), among others. Would be nice to have consistent for easier reading :))

* (modules/light-clients/09-localhost) [\#1187](https://github.com/cosmos/ibc-go/pull/1187/) Removing localhost light client implementation as it is not functional.
* [\#1186](https://github.com/cosmos/ibc-go/pull/1186/files) Removing `GetRoot` function from ConsensusState interface in `02-client`. `GetRoot` is unused by core IBC.
* (modules/core/02-client) [\#1196](https://github.com/cosmos/ibc-go/pull/1196) Adding VerifyClientMessage to ClientState interface.
Expand Down
80 changes: 34 additions & 46 deletions modules/core/02-client/keeper/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (k Keeper) CreateClient(
}

// UpdateClient updates the consensus state and the state root from a provided header.
func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.ClientMessage) error {
func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exported.ClientMessage) error {
clientState, found := k.GetClientState(ctx, clientID)
if !found {
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
Expand All @@ -66,54 +66,21 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.C
return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot update client (%s) with status %s", clientID, status)
}

// 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)
}

// emit the full header in events
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.MustMarshalClientMessage(k.cdc, header))
// set default consensus height with header height
consensusHeight = header.GetHeight()

if err := clientState.VerifyClientMessage(ctx, k.cdc, clientStore, clientMsg); err != nil {
return err
}

// 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
k.SetClientConsensusState(ctx, clientID, header.GetHeight(), newConsensusState)

k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
// Marshal the ClientMessage 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.
clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "update"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelUpdateType, "msg"),
},
)
}()
// set default consensus height with header height
consensusHeight := clientMsg.GetHeight()

// emitting events in the keeper emits for both begin block and handler client updates
EmitUpdateClientEvent(ctx, clientID, newClientState, consensusHeight, headerStr)
} else {
foundMisbehaviour := clientState.CheckForMisbehaviour(ctx, k.cdc, clientStore, clientMsg)
if foundMisbehaviour {
clientState.UpdateStateOnMisbehaviour(ctx, k.cdc, clientStore, clientMsg)

k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID)

Expand All @@ -129,9 +96,30 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.C
)
}()

EmitSubmitMisbehaviourEventOnUpdate(ctx, clientID, newClientState, consensusHeight, headerStr)
EmitSubmitMisbehaviourEventOnUpdate(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)

return nil
}

clientState.UpdateState(ctx, k.cdc, clientStore, clientMsg)

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(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelUpdateType, "msg"),
},
)
}()

// emitting events in the keeper emits for both begin block and handler client updates
EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should ask relayers if emitting the consensus height is necessary for their operations. If we remove GetHeight from ClientMessage, it'll be impossible to know what height was actually updated and with batch updates this event emission doesn't make much sense

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Briefly, I think emitting the consensus height is necessary for relayers, as part of misbehavior detection.

Specifically, the misbehavior detection worker in Hermes needs to know the consensus height at which a client was just updated. For each event with type_str = update_client, we are searching the field called consensus_height. The misbehavior detection task cross-checks the details of the header from that consensus height with the header from the original chain. If the two headers are not consistent, that is used as evidence of misbehavior. Hermes does this check upon every update client event found as part of a deliverTx batch of events.

Copy link

@ancazamfir ancazamfir Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run misbehavior only if the client update event includes a header (older versions didn't) so hermes could use the header height.

If we remove GetHeight from ClientMessage, it'll be impossible to know what height was actually updated and with batch updates this event emission doesn't make much sense

I don't understand this, could you please clarify? You mean multiple client updates in a Tx? And why the event wouldn't make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still emit information about which heights got added. Since it is now possible for multiple heights to get added in a single client msg.

We could do this in a backward compatible way:

Current height tag is the latest height to get added, and we have a separate field that has the list of all added heights.

Or we could break the events:

Height field now is a list of all heights added by the clientMsg

Copy link
Member

@damiannolan damiannolan Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to reason through the best approach for this... we could:

  • Retain consensus_height event but change the ClientMessage interface to LatestHeight() or GetLatestHeight().
  • Add a new, additional method to the ClientMessage interface for multiple heights, GetHeights() / GetBatchedHeights() or something similar(?), and emit an additional batched_heights field in the events with a list of heights.

The ClientMessage interface is now accounting for both Header and Misbehaviour types; at least in the case of the tendermint client, so both methods for latest height and multiple heights would need to be implemented on both concrete types. I have slight concerns that this may not be the cleanest approach given the Misbehaviour type.
In a "batched update" scenario the protobuf Any on MsgUpdateClient would need to be unmarshalled to a type containing a list of headers. Something like below, as you cannot directly unmarshal a protobuf Any to a list.
Both interfaces would need to be implemented on this type also.

message BatchedHeaders {
    repeated Header headers = 1;
}

I'm thinking based on @ancazamfir's comment, it may be cleaner for relayers to pull the height from the Header which is already parsed in Hermes and we just remove this GetHeight method from ClientMessage entirely? However, any client that wishes to support batched updates would need to be accounted for by relayers, in order to unmarshal a BatchedHeaders type like above and take the latest height from the end of the list for example, assuming ordered is respected.

Would be useful to hear thoughts before progressing on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run misbehavior only if the client update event includes a header (older versions didn't) so hermes could use the header height.

Yes this makes sense. I think when I wrote the original comment on this thread I forgot we emit the entire header.

I don't understand this, could you please clarify? You mean multiple client updates in a Tx? And why the event wouldn't make sense?

Yes, sorry for the confusion.

If we remove GetHeight from ClientMessage, it'll be impossible to know what height was actually updated

What I said is incorrect since we emit the full header. But that does assume the consumer of that full header understands how to correlate the properties of that header with which height was updated.

with batch updates this event emission doesn't make much sense

We are currently refactoring our 02-client module and its associated interfaces to allow clients to update many heights in a single MsgUpdateClient. MsgUpdateClient can now be used for misbehaviour submission as well (MsgSubmitMisbehaviour redirects to this handler)

I think we should still emit information about which heights got added. Since it is now possible for multiple heights to get added in a single client msg.

Initially I agree with this. But if the consensus heights emitted are primarily for misbehaviour detection and we emit the full header, I wonder if it is necessary? What purpose does it serve?

Detecting misbehaviour is interesting because the consumer of this detection already needs to have a good idea of what is considered misbehaviour for a light client. My initial concern with relying on emission of the full header (now called ClientMessage) for obtaining the consensus heights is the requirement for understanding that client message. Perhaps implementing misbehaviour detection must always be specific to type of light client it is detecting for or maybe it can be generalized for the most part

I think it is probably hard to predict what misbehaviour detection looks like for non-tendermint. We can make future changes when we have that feedback. To be on the safe side we could emit the consensus heights.

Thanks @damiannolan for the write up on potential approaches. That was very useful. Personally I don't like the idea of adding an interface to ClientMessage. It is possible that heights in a client message don't actually get updated due to no-ops (duplicate updates). As @damiannolan points out, the ClientMessage interface also affects misbehaviour which we don't need to emit consensus heights for.

Another approach is to modify the UpdateState method to return the consensus heights updated by the light client. We could leave/deprecate the existing consensus_height tag and add a new consensus_heights event tag

Since hermes only does misbehaviour detection for tendermint at the moment and tendermint will only update 1 height at a time, we can simply take the first consensus height returned in the consensus heights for the deprecated event tag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the detailed response @colin-axner, much appreciated! :)

Initially I agree with this. But if the consensus heights emitted are primarily for misbehaviour detection and we emit the full header, I wonder if it is necessary? What purpose does it serve?

I agree, and also the same questions came to mind. As you say tho, this pushes a requirement onto relayers for understanding each client message type to extract the consensus heights, and I think there is value in emitting them in a more basic format.

Personally I don't like the idea of adding an interface to ClientMessage. It is possible that heights in a client message don't actually get updated due to no-ops (duplicate updates).

Agree, and this also a good point regarding no-ops.

I'm happy to move ahead with the proposed solution here if there's no outstanding remarks. This would involve the new UpdateState interface being modified as below to return the heights added to state, omitting duplicate updates.

UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) ([]Height, error)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is probably hard to predict what misbehaviour detection looks like for non-tendermint. We can make future changes when we have that feedback. To be on the safe side we could emit the consensus heights.

I got a bit lost in the discussion, and I'm tempted to consider this statement as a good summary. I think the approach summarized above is sound.

Just to confirm them: Do I understand correctly that the expectation is that ibc-go will continue emitting the consensus_height? Please let us know if there are there any/other breaking changes that should be expected. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @adizere!

Do I understand correctly that the expectation is that ibc-go will continue emitting the consensus_height?

Yep! But we will mark as deprecated and remove at some point in the future. But who knows what that time will be. I've summarised in the following issue if you want to take a look. See #594


return nil
}

Expand Down
10 changes: 5 additions & 5 deletions modules/core/02-client/keeper/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
}

// EmitUpdateClientEvent emits an update client event
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientState exported.ClientState, consensusHeight exported.Height, headerStr string) {
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, clientMsgStr string) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeUpdateClient,
sdk.NewAttribute(types.AttributeKeyClientID, clientID),
sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
sdk.NewAttribute(types.AttributeKeyClientType, clientType),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
sdk.NewAttribute(types.AttributeKeyHeader, clientMsgStr),
),
sdk.NewEvent(
sdk.EventTypeMessage,
Expand Down Expand Up @@ -80,12 +80,12 @@ func EmitSubmitMisbehaviourEvent(ctx sdk.Context, clientID string, clientState e
}

// EmitSubmitMisbehaviourEventOnUpdate emits a client misbehaviour event on a client update event
func EmitSubmitMisbehaviourEventOnUpdate(ctx sdk.Context, clientID string, clientState exported.ClientState, consensusHeight exported.Height, headerStr string) {
func EmitSubmitMisbehaviourEventOnUpdate(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, headerStr string) {
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeSubmitMisbehaviour,
sdk.NewAttribute(types.AttributeKeyClientID, clientID),
sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
sdk.NewAttribute(types.AttributeKeyClientType, clientType),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
),
Expand Down
2 changes: 1 addition & 1 deletion modules/core/02-client/legacy/v100/solomachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode

// UpdateStateOnMisbehaviour panics!
func (cs *ClientState) UpdateStateOnMisbehaviour(
_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore,
_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientMessage,
) {
panic("legacy solo machine is deprecated!")
}
Expand Down
4 changes: 2 additions & 2 deletions modules/core/exported/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ type ClientState interface {
ExportMetadata(sdk.KVStore) []GenesisMetadata

// UpdateStateOnMisbehaviour should perform appropriate state changes on a client state given that misbehaviour has been detected and verified
UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore)
UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg ClientMessage)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible a light client needs the misbehaviour ClientMessage to take appropriate action

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, makes sense!


// VerifyClientMessage verifies a ClientMessage. A ClientMessage could be a Header, Misbehaviour, or batch update.
VerifyClientMessage(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg ClientMessage) error

// UpdateState updates and stores as necessary any associated information for an IBC client, such as the ClientState and corresponding ConsensusState.
// UpdateState updates and stores as necessary any associated information for an IBC client, such as the ClientState and corresponding ConsensusState.
// An error is returned if ClientMessage is of type Misbehaviour
UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) error

Expand Down
4 changes: 2 additions & 2 deletions modules/light-clients/06-solomachine/types/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func (cs ClientState) CheckHeaderAndUpdateState(

foundMisbehaviour := cs.CheckForMisbehaviour(ctx, cdc, clientStore, msg)
if foundMisbehaviour {
cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore)
cs.UpdateStateOnMisbehaviour(ctx, cdc, clientStore, msg)
return &cs, cs.ConsensusState, nil
}

Expand Down Expand Up @@ -142,7 +142,7 @@ func (cs ClientState) CheckForMisbehaviour(_ sdk.Context, _ codec.BinaryCodec, _

// 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(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore) {
func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, _ exported.ClientMessage) {
cs.IsFrozen = true

clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))
Expand Down
2 changes: 1 addition & 1 deletion modules/light-clients/06-solomachine/types/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ func (suite *SoloMachineTestSuite) TestUpdateStateOnMisbehaviour() {

tc.malleate()

clientState.UpdateStateOnMisbehaviour(suite.chainA.GetContext(), suite.chainA.Codec, suite.store)
clientState.UpdateStateOnMisbehaviour(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, nil)

if tc.expPass {
clientStateBz := suite.store.Get(host.ClientStateKey())
Expand Down
2 changes: 1 addition & 1 deletion modules/light-clients/07-tendermint/types/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode

// UpdateStateOnMisbehaviour updates state upon misbehaviour, freezing the ClientState. This method should only be called when misbehaviour is detected
// as it does not perform any misbehaviour checks.
func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore) {
func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, _ exported.ClientMessage) {
cs.FrozenHeight = FrozenHeight

clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))
Expand Down
6 changes: 1 addition & 5 deletions modules/light-clients/07-tendermint/types/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,11 +792,7 @@ func (suite *TendermintTestSuite) TestUpdateStateOnMisbehaviour() {
clientState := path.EndpointA.GetClientState()
clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)

// TODO: remove casting when 'UpdateState' is an interface function.
tmClientState, ok := clientState.(*types.ClientState)
suite.Require().True(ok)

tmClientState.UpdateStateOnMisbehaviour(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore)
clientState.UpdateStateOnMisbehaviour(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, nil)

if tc.expPass {
clientStateBz := clientStore.Get(host.ClientStateKey())
Expand Down