[ICS07] Detach verifier field from Tendermint ClientState struct

[Farhad-Shabani]

Overview

Relevant Context: #958

The current Tendermint ClientState struct includes a verifier field that serves as a handle for calling underlying verification functions. This design, while functional, poses challenges in terms of consistency with the proto-generated ClientState struct and hinders the support for customized header/misbehaviour verifications as requested here.

Proposal

Ideally, Tendermint clients should implement a Verifier trait with a verifier method to give callers a handle and an associated type so they can determine underlying ed25519 verifier implementation. We can then provide a built-in/standard implementation of our interfaces on the Tendermint ClientState struct, utilizing the default ProdVerifier, which is intended for the majority of users who would typically import it. Though, still projects would have the flexibility to opt for a more cost-effective verifier implementation of their choice.

How to get our design there:

1. Refactor ClientState methods
   • Extract the existing implementation within ClientState methods into standalone, reusable, and publicly accessible functions.
2. Define the Verifier trait
   • Articulate a Verifier trait that encapsulates the verification logic and offers a handle to the verifier of the user's chosen type.
3. Implement ClientState with default ProdVerifier
   • Present a readily implemented ClientState with the default verifier (ProdVerifier), catering to the needs of the majority of users.
4. Support customized verification
   • The ClientState struct can be easily re-implemented by users requiring customized verification.

[seanchen1991]

For the methods included as part of the new Verifier trait, would we only move the ClientState methods that currently use the verifier field into the trait?

These would include:

• verify_header: https://github.com/cosmos/ibc-rs/blob/main/ibc-clients/ics07-tendermint/src/client_state/update_client.rs#L16
• verify_misbehaviour_header: https://github.com/cosmos/ibc-rs/blob/main/ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs#L64

Are there other methods that are appropriate to be added to this new trait?

[Farhad-Shabani]

Indeed, the verifier is invoked within these two methods.
Initially, I was thinking of abstracting things based on the similar trait as follows:

pub trait TmVerifier {
    type Verifier: tendermint_light_client_verifier::Verifier;

    fn verifier(&self) -> Self::Verifier;
}

impl TmVerifier for ClientState {
    type Verifier = ProdVerifier;

    fn verifier(&self) -> Self::Verifier {
        ProdVerifier::default()
    }
}

However, I am uncertain about the optimal strategy. Ideally, default implementors should not be required to introduce additional elements. At the top, we might require to set a type Verifier, and the remaining details should be resolved through our design. I mention this because the implementation logic for both verify_header and verify_misbehaviour_header always remains consistent with any kind of verifier. The only variation lies in the location where this verifier (e.g. here) is called, which may not necessarily follow the default ProdVerifier.

[seanchen1991]

The only variation lies in the location where this verifier is called, which may not necessarily follow the default ProdVerifier

So depending on the underlying verifier that is used, it might be called at different points during the flow of verify_header and verify_misbehaviour_header?

[Farhad-Shabani]

So depending on the underlying verifier that is used, it might be called at different points during the flow of verify_header and verify_misbehaviour_header?

Still the verifier would be called at the same point within both verify_header and verify_misbehaviour_header.

[seanchen1991]

The approach you outlined works well for any hosts that are fine with utilizing the default ProdVerifier implementation. I'm not really sure how to go about experimenting with a different verifier, though, in order to get a sense of what the experience is like in that scenario.

[Farhad-Shabani]

The approach you outlined works well for any hosts that are fine with utilizing the default ProdVerifier implementation. I'm not really sure how to go about experimenting with a different verifier, though, to get a sense of what the experience is like in that scenario.

That's exactly why I was not sure if that was the optimal approach. It makes anyone seeking a custom verifier re-implement all the ClientState traits. We could streamline this process by transforming each of our implemented methods into stand-alone importable functions. So, it becomes a matter of wiring things up. However, still not optimal!

[seanchen1991]

I'm a bit confused on why all of the ClientState traits need to be re-implemented in the case the host wants to make use of a custom verifier.