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

Tracking issue for verification functions of TendermintClient #76

Closed
4 of 10 tasks
yito88 opened this issue Jul 20, 2021 · 10 comments
Closed
4 of 10 tasks

Tracking issue for verification functions of TendermintClient #76

yito88 opened this issue Jul 20, 2021 · 10 comments
Assignees

Comments

@yito88
Copy link
Contributor

yito88 commented Jul 20, 2021

Crate

IBC

Summary of Bug

Almost all verification functions in ibc::ics07_tendermint::client_def::TendermintClient are not implemented.

Version

0.6.0

Steps to Reproduce

Call verification functions such as ibc::ics03_connection::handler::verify::verify_proofs with a TendermintClient

Acceptance Criteria

All verification functions are implemented


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere
Copy link
Contributor

adizere commented Jul 23, 2021

Just a note on progress on this PR: we are on it informalsystems/hermes#1120. This first PR should be done by the end of this milestone (end of July).

More follow-up work will be necessary for ICS03 / ICS04 specific verification methods.

@adizere adizere changed the title Verification functions of TendermintClient are not implemented Tracking issue for verification functions of TendermintClient Oct 12, 2021
@lightyear15
Copy link

Hello,
I have been following the hermes project quite closely, taking it as a starting point for a non-cosmos-chain project. see this.
I am taking this issue as an ice-breaker to ask a few questions:

  • What is the purpose of the ClientDef trait and their implementation?
  • Isn't verifying states a duty that the client itself is in charge of, not something the relayer should bother about?
  • in light of the recently approved new spec document on wasm-based clients, how will the hermes realyer implementation for this new client type be able to fulfill the requirements for this a trait? (TL;DR for the wasm client: the client is implemented in wasm and runs in the cosmos chain as a smart contract, the ibc-go go module is an opaque sub-module that works as a proxy, forwarding all the client functions calls to the smart contract) How can the ics08-wasm module provides an implementation of ClientDef?

Thanks

@adizere
Copy link
Contributor

adizere commented Nov 11, 2021

Hey, the ClientDef trait can be difficult to understand because this trait is a central interface between off-chain (relayer) and on-chain (modules) IBC components. Our repository comprises both on- and off-chain implementations, hence it's not always clear what's essential for the IBC relayer, and what pertains to the IBC module.

What is the purpose of the ClientDef trait and their implementation?

The purpose is to capture all the methods that any IBC client implementation must provide. In other words, ClientDef is almost a 1:1 mapping of the ICS02 specification.

All of the following components belong to our ibc module (on-chain) implementation:

Isn't verifying states a duty that the client itself is in charge of, not something the relayer should bother about?

The off-chain IBC component (the relayer, i.e., Hermes) imports and uses all of these different types and methods because the relayer itself must perform header verification (which is a method that clients provide) for all the chains it communicates with. Hermes must verify states for several basic purposes:

  1. to be able to fetch, deserialize, syntactically validate headers from a chain
    • and then construct client update transactions based on these headers, which the relayer submits to the counterparty chain
    • the functionality of updating clients is an essential pre-requisite for transfering packets (such as ICS20 token transfer packets).
  2. to semantically verify headers and avoid submitting fraudulent headers in client update transactions (these transactions would be rejected, potentially wasting fees, and is in effect a sign of faulty behavior)
  3. to run misbehavior detection (which is optional, most relayer implementations do not support this, but Hermes does).

how will the hermes realyer implementation for this new client type be able to fulfill the requirements for this a trait?

Briefly, in order to support WASM clients for Hermes, it is essential to add the following components:

  • pub struct WasmClient comprising the verification predicates that this client must implement to verify Wasm-chains.
  • impl ClientDef for WasmClient comprising method forwarding between ICS02 interface and the verification predicates in struct WasmClient.

@lightyear15
Copy link

lightyear15 commented Nov 11, 2021

thanks @adizere for your explanation.
The very last point of your post is still very hard to digest for me.
To my knowledge, the whole idea of a wasm-based client is to remove from the on-chain IBC module the need for functions and algorithms necessary to understand and verify any consensus mechanisms used in non-tendermint chains.
So in my head, your statement

Briefly, in order to support WASM clients for Hermes, it is essential to add the following components:

  • pub struct WasmClient comprising the verification predicates that this client must implement to verify Wasm-chains.
  • impl ClientDef for WasmClient comprising method forwarding between ICS02 interface and the verification predicates in struct WasmClient.

conflicts with the idea of having a complete transparent wasmclient proxy in the ibc-go module that simply forwards all the calls to a smart contract. You can have a look at a first implementation draft here.
For instance, if you look at the implementation of the ClientState interface for ics08-wasm here ), you can see that the module itself does not contain any logic about proofs verification, it simply forwards calls to the given smart contract.
My conclusion is then:

  • I accept the idea of implementing an on-chain ics08-wasm component in here just to mirror the ( yet to be merged) implementation in cosmos/ibc-go repo, ( even though I see quite a few obstacles along this path as there is no cosmos-sdk implementation in rust, so I am not sure how I would deal with VM and smart contract calls)
  • I can't picture in my mind a feasible implementation of ClientDef for WasmClient that can fit into the relayer, unless we accept the idea that, when dealing with ics08-wasm clients we have to delegate the verification and misbehavior detection to and only to the clients (resulting in an empty impl ClientDef for WasmClient implementation ).

As last point: I would argue that the relayer is not in charge of verifying anything as this is a duty the light-clients are in charge of, misbehaviour detection included. Otherwise IBC protocol wouldn't be a "mechanism to create trust-less bridges" but yet another trusted bridge protocol, as you are now relaying on the relayer (sorry for the pun) to pass truthful messages.
Regarding the waste of fees when submitting fraudulent headers or, more in general, messages: I think it's up to the malicious actor to pay for the fees in the attempt to corrupt the client. As long as the benevolent relayer is connected to benevolent nodes of the two networks, gathering correct headers, it shouldn't be worried about fraudulent attacks made by malicious relayers trying to break the bridge. Of course, ibc modules and light clients running on the two chains must be robust enough to resist against any sort of attack.

@lightyear15
Copy link

Hi @adizere ,
any update on this on how the ics08-wasm could provide the verify_XXX functions when compiling the hermes binary?

@adizere
Copy link
Contributor

adizere commented Nov 22, 2021

Hi @adizere ,
any update on this on how the ics08-wasm could provide the verify_XXX functions when compiling the hermes binary?

Continued the discussion here: informalsystems/hermes#1318 (comment)

@romac
Copy link
Member

romac commented Nov 22, 2021

@lightyear15 I just want to point out that the plan for supporting WASM clients is indeed to call into the WASM code rather than hardcode anything in the ClientDef impl for WasmClient. More thought and investigate work is needed to figure out how to do this properly and whether or not the current ChainEndpoint + ClientDef traits are suitable for this purpose. We'll come back to you with a better plan once we get around to dedicate time to work on that. In the meantime, we're happy to hear any more suggestions or feedback you may have for us :)

@lightyear15
Copy link

Hi @romac ,
thanks for your help.
I already have an almost-working implementation of wasm client based on hermes 0.7.2.
Happy to discuss the issues and struggles I went through 😄

@romac
Copy link
Member

romac commented Nov 22, 2021

@lightyear15 Oh wow, that's amazing 🎉 Yeah we would definitely love to hear about all that! Would be so kind as to (eventually) open a PR here?

PS: Sorry to hear about the struggles part, please let us know how we could have been of better help.

@lightyear15
Copy link

No problem at all...
I will try to split the changes I made in small and narrow-scoped PRs

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

No branches or pull requests

5 participants