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

Verify ClientState on Connection Handshake #6556

Closed
wants to merge 1 commit into from

Conversation

AdityaSripal
Copy link
Member

Description

closes: #6524


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

@AdityaSripal
Copy link
Member Author

AdityaSripal commented Jul 1, 2020

As per comment here: #6524 (comment) ,
we have decided to verify the ClientState in the connection handshake, as opposed to the consensusState and then verify that the clientstate is a valid client of our chain.

However unlike with consensusState, I need to also pass in the clientState in addition to the proof of the clientState
since the clientState the counterparty has for us cannot be constructed by us (we don't know certain lite-client level parameters like TrustLevel as these can vary from client to client). Note we were able to completely construct the exact expected ConsensusState on our own, so there wasn't an analogous problem with consensus states.

This means a relayer has to ultimately pass the clientState over the wire in an SDK msg (namely MsgConnOpenTry and MsgConnOpenAck) (edited)
The problem is the connection msgs are proto structs, and the ClientState is a go interface...
Since this validation is happening by an SDK chain, I could just pass the clientState as bytes in the msg and decode it on the SDK as tendermint.ClientState and return error if this fails. Other implementations on other platforms could do a similar decoding of clientstate bytes to match their own expected type.

This is a bit hacky, so want some feedback on the issue described and other potential solutions.

cc: @fedekunze

@AdityaSripal AdityaSripal added x/ibc S:blocked Status: Blocked labels Jul 1, 2020
@AdityaSripal
Copy link
Member Author

Blocked on #6254

@alexanderbez alexanderbez removed the WIP label Jul 7, 2020
@colin-axner
Copy link
Contributor

@AdityaSripal this should be unblocked after #6948 right? Will this still be an issue? Why can't the MsgConnOpenTry have exported.ClientState as a field? Also, at the time of above comments writing, the MsgConnOpenTry were being amino encoded, the flip to proto happened a few days ago. Not sure if this changes anything

@colin-axner colin-axner removed the S:blocked Status: Blocked label Aug 10, 2020
@AdityaSripal
Copy link
Member Author

Superseded by #7057

@colin-axner colin-axner deleted the aditya/verify-clientstate branch October 8, 2020 09:03
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.

Determine what in ClientState needs to be verified by counterparty
3 participants