Replace CheckHeaderAndUpdateState with new ClientState functions #1208
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, clientMsg exported.ClientMessage) error { | ||
| clientState, found := k.GetClientState(ctx, clientID) | ||
| if !found { | ||
| return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID) | ||
|
|
@@ -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) | ||
| } | ||
|
|
||
| if err := clientState.VerifyClientMessage(ctx, k.cdc, clientStore, clientMsg); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // 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)) | ||
|
|
||
| // set default consensus height with header height | ||
| consensusHeight := clientMsg.GetHeight() | ||
|
|
||
| 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) | ||
|
|
||
|
|
@@ -129,9 +96,30 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.C | |
| ) | ||
| }() | ||
|
|
||
| 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) | ||
|
There was a problem hiding this comment. we should ask relayers if emitting the consensus height is necessary for their operations. If we remove There was a problem hiding this comment. 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 There was a problem hiding this comment. We run misbehavior only if the client update event includes a header (older versions didn't) so hermes could use the header height.
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? There was a problem hiding this comment. 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 There was a problem hiding this comment. Trying to reason through the best approach for this... we could:
The I'm thinking based on @ancazamfir's comment, it may be cleaner for relayers to pull the height from the Would be useful to hear thoughts before progressing on this. There was a problem hiding this comment.
Yes this makes sense. I think when I wrote the original comment on this thread I forgot we emit the entire header.
Yes, sorry for the confusion.
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.
We are currently refactoring our 02-client module and its associated interfaces to allow clients to update many heights in a single
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 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 Another approach is to modify the 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 There was a problem hiding this comment. Thanks a lot for the detailed response @colin-axner, much appreciated! :)
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.
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 There was a problem hiding this comment.
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 There was a problem hiding this comment. Hey @adizere!
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 | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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 :))