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.

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

Replace CheckHeaderAndUpdateState with new ClientState functions #1208

Merged

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Mar 31, 2022

Description

closes: #668
closes: #284


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

… CheckForMisbehaviour, UpdateStateOnMisbehaviour, and UpdateState
@@ -53,12 +53,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!

}()

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

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

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

@colin-axner colin-axner marked this pull request as ready for review March 31, 2022 14:43
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Awesome, super clean PR. Really nice to see this come together

@@ -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 :))

@@ -53,12 +53,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
Member

Choose a reason for hiding this comment

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

Agree, makes sense!

Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

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

Nice! We are nearly there 🤝

@seantking seantking merged commit e2f37b8 into 02-client-refactor Apr 1, 2022
@seantking seantking deleted the colin/668-replace-checkheaderandupdatestate branch April 1, 2022 10:51
seunlanlege pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022
…mos#1208)

* refactor: replace CheckHeaderAndUpdateState with VerifyClientMessage, CheckForMisbehaviour, UpdateStateOnMisbehaviour, and UpdateState

* add changelog entry

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants