-
Notifications
You must be signed in to change notification settings - Fork 625
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
seantking
merged 4 commits into
02-client-refactor
from
colin/668-replace-checkheaderandupdatestate
Apr 1, 2022
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9ce9259
refactor: replace CheckHeaderAndUpdateState with VerifyClientMessage,…
colin-axner 11c1d16
add changelog entry
colin-axner a617074
Merge branch '02-client-refactor' of github.com:cosmos/ibc-go into co…
colin-axner c68333e
fix tests
colin-axner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
we should ask relayers if emitting the consensus height is necessary for their operations. If we remove
GetHeight
fromClientMessage
, it'll be impossible to know what height was actually updated and with batch updates this event emission doesn't make much senseThere 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.
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 calledconsensus_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.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.
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.
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
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.
Trying to reason through the best approach for this... we could:
consensus_height
event but change theClientMessage
interface toLatestHeight()
orGetLatestHeight()
.ClientMessage
interface for multiple heights,GetHeights()
/GetBatchedHeights()
or something similar(?), and emit an additionalbatched_heights
field in the events with a list of heights.The
ClientMessage
interface is now accounting for bothHeader
andMisbehaviour
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 theMisbehaviour
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.
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 thisGetHeight
method fromClientMessage
entirely? However, any client that wishes to support batched updates would need to be accounted for by relayers, in order to unmarshal aBatchedHeaders
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.
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.
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
MsgUpdateClient
.MsgUpdateClient
can now be used for misbehaviour submission as well (MsgSubmitMisbehaviour
redirects to this handler)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 partI 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, theClientMessage
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 existingconsensus_height
tag and add a newconsensus_heights
event tagSince 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.
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! :)
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
UpdateState
interface being modified as below to return the heights added to state, omitting duplicate updates.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.
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 theconsensus_height
? Please let us know if there are there any/other breaking changes that should be expected. Thanks!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.
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