ChainEndpoint trait: the role of associated type ConsensusState is ambiguous

[lightyear15]

Hello,
I am working on hermes to implement an IBC connection between cosmos chain and a non-cosmos / non-tendermint-based chain.
For the sake of the explanation I will assume I have a GenBlockChain running a consensu protocol called GenBFT.
Configuration is then:

• cosmos network with tendermint consensus protocol, running an IBC GenBFT light client that can monitor and validate events happening in the GenBlockChain
• GenBlockChain with GenBFT consensus protocol running an IBC tendermint light client that can monitor and validate events happening in cosmos.

As far as I understand the ibc-relayer crate, the key trait that I have to implement is the ChainEndpoint.

I am studying the methods and the associated types that the trait has, and I realised there is something that does not seem consistent: I am referring to the associated type ChainEndpoint::ConsensusState, and the two methods ChainEndpoint::build_consensus_state and ChainEndpoint::proven_consensus_state.

If I define the ConsensusState type to be TendermintConsensusState I wouldn't be able to implement build_consensus_state as my side of ChainEndpoint does not have access to cosmos network and it wouldn't know anything about the current state of the tendermint consensus state, nor it should know anything about tendermint or cosmos (in terms of structs and functions) as this is the role of the CosmosSDKChainEndpoint implementation.

On the other hand, if I define ConsensusState to be the GenBFTConsensusState then I wouldn't be able to implement proven_consensus_state as, again, my ChainEndpoint does not have access to the cosmos network in order to request and prove the current state of the GenBFTLightClient running on cosmos.

I am clearly missing something from the code design POV, can you please help me out understanding how my GenBlockChainEndpoint should look like?

[adizere]

Hello and thank you for opening this issue.

Briefly. I think you're right: there is something inconsistent in our definition of trait ChainEndpoint. Some explanations follow..

As far as I understand the ibc-relayer crate, the key trait that I have to implement is the ChainEndpoint.

Yes.

I am studying the methods and the associated types that the trait has, and I realised there is something that does not seem consistent: I am referring to the associated type ChainEndpoint::ConsensusState, and the two methods ChainEndpoint::build_consensus_state and ChainEndpoint::proven_consensus_state.

So far so good! I think the method name is proven_client_consensus, not proven_consensus_state.

If I define the ConsensusState type to be TendermintConsensusState I wouldn't be able to implement build_consensus_state as my side of ChainEndpoint does not have access to cosmos network and it wouldn't know anything about the current state of the tendermint consensus state, nor it should know anything about tendermint or cosmos (in terms of structs and functions) as this is the role of the CosmosSDKChainEndpoint implementation.

Correct.

On the other hand, if I define ConsensusState to be the GenBFTConsensusState then I wouldn't be able to implement proven_consensus_state as, again, my ChainEndpoint does not have access to the cosmos network in order to request and prove the current state of the GenBFTLightClient running on cosmos.

I think proven_consensus_state should return the consensus state for any client that lives on a GenBlockChain.

To complement here with additional context: Suppose the relayer wants to build a client with identifier x-genblockchain-01 which will live on cosmoshub-4, and the source chain for this client is genbft-chain-03 (a chain of type GenBlockChain). Then:

• impl ChainEndpoint for GenBlockChain::build_consensus_state: relies on a light client which the relayer runs locally to fetch and verify a light block from any chain of type GenBlockChain such as genbft-chain-03. Then the relayer builds a consensus state from that light block. Concretely the consensus state would have type GenBlockChain::ConsensusState so it should probably be GenBFTConsensusState. This is the concrete consensus state that the relayer uses to create client x-genblockchain-01 on chain cosmoshub-4. Of course, cosmoshub-4 would need to have an implementation of a light client for GenBlockChain!

We can also use other direction as an example. Suppose the relayer wants to build a client with identifier 07-tendermint-01 which will live on genbft-chain-03, and the source chain for this client is cosmoshub-4. In this case:

• The relayer calls into impl ChainEndpoint for CosmosSdkChain::build_consensus_state to obtain a consensus state from comoshub-4. Concretely the consensus state would have type CosmosSdkChain::ConsensusState ie. ics07_tendermint::ConsensusState. Then the relayer uses this consensus state to create 07-tendermint-01 on genbft-chain-03. Again, genbft-chain-03 needs to be running a light client algorithm of type 07 Tendermint for this flow to work correctly.

To summarize, the ConsensusState for GenBlockChain should probably be GenBFTConsensusState.

As for proven_client_consensus, this is probably incorrect. The relayer calls into this method to fetch the consensus state that a client stores for a certain height. Going back to our examples above, calling into proven_client_consensus for client 07-tendermint-01 (on chain genbft-chain-03) shall return a CosmosSdkChain::ConsensusState. And similarly, calling into proven_client_consensus for client x-genblockchain-01 (on chain cosmoshub-4) should return a GenBFTConsensusState.

This means that we probably have a problem in the proven_client_consensus method signature. This may be more appropriate:

diff --git a/relayer/src/chain.rs b/relayer/src/chain.rs
index b4afd859..ddff4f6b 100644
--- a/relayer/src/chain.rs
+++ b/relayer/src/chain.rs
@@ -287,11 +287,11 @@ pub trait ChainEndpoint: Sized {
     fn proven_client_consensus(
         &self,
         client_id: &ClientId,
         consensus_height: ICSHeight,
         height: ICSHeight,
-    ) -> Result<(Self::ConsensusState, MerkleProof), Error>;
+    ) -> Result<(AnyConsensusState, MerkleProof), Error>;

     fn proven_channel(
         &self,
         port_id: &PortId,
         channel_id: &ChannelId,

Let me know if there's still anything ambiguous! Otherwise we'll go ahead and modify the trait ChainEndpoint with the patch above to close this issue.

[lightyear15]

Thanks a lot @adizere,
everything's crystal clear now. 👍

The only thing I would add, is that probably the same reasoning applies to functions proven_client_state and build_client_state