Remove Hardcoding of Tendermint Client Validation in Connection Handshake

[AdityaSripal]

Summary

The connection handshake validates the client on the counterparty against its own consensus parameters. This is to ensure that the counterparty client trying to connect to us is a valid client of our own chain. Currently we hardcode assumptions that the counterparty is a tendermint client. Moreover, it is enforced that the counterparty is exactly a go tendermint client as implemented in the ibc-go repository.

Problem Definition

This is a well-known problem that ibc-go assumes its underlying consensus is Tendermint. This will need to be changed if we want ibc-go to be mounted on non-Tendermint chains. Moreover, it is also blocking for Tendermint chains that want to make some modifications to the default tendermint client or use a different implementation entirely (e.g. wasm tendermint client)

This is now a concrete blocker for rollkit integration.

Rollkit will be running ibc-go with SDK but the counterparty client verifying a rollkit rollup will have to be a WASM rollup client
Celestia is an SDK chain with tendermint, however the celestia client we want to use for verifying rollups will need some modifications, namely it will need to store the DataHash which we will want to put inside the ConsensusState.

In both cases, the connection handshake will fail since we are making very strict checks that the client state and consensus state are exactly what we expect in the 07-tendermint case.

Direct unmarshalling and checking of 07-tendermint client state prevents counterparties from using a different or modified client (e.g. wasm tendermint client)
Changes to the tendermint client state are also unsupported since we cast directly to a tmtypes.ClientState
Changes to the consensus state are not possible since we construct directly to our expected value and verify

Related links:

ClientState Validation:

https://github.com/cosmos/ibc-go/blob/main/modules/core/03-connection/keeper/handshake.go#L97
https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/keeper.go#L283

ConsensusState Verification:

https://github.com/cosmos/ibc-go/blob/main/modules/core/03-connection/keeper/handshake.go#L101

ibc-go/modules/core/02-client/keeper/keeper.go

Line 270 in 1f24a5a

consensusState := &ibctm.ConsensusState{

Note, here the counterparty consensus state isn't even passed in by relayer. We construct it on our own end because we assume it must be a 07-tendermint consensus state.

Proposal

1. Extend ConnectionOpenTry and ConnectionOpenAck messages to pass in a counterparty consensus state from the relayer.

After this there are two approaches, each with their own pros and cons.

Strict Approach:

This approach continues making strict checks on the client state and consensus state, but allows the chain developer to define their own function that can be registered on the connection keeper on initialization. The chain developer can then validate the finite and predefined types of clients they want to support. So for example, a chain developer can write a function that will validate a go-tendermint client/consensus or a wasm-tendermint client/consensus.

The benefit here is that we can still unmarshal the client state into an exact type and make any checks on the fields that we wish. The con is that the chain developer still has to add new validation logic for each clienttype they want to support, as well as accomodate any customizations they want to allow on the counterparty.

Flexible Approach:

In this approach we remain with a customizable function (since we want to support consensus beyond TM). However, we do not unmarshal the clientstate and consensus state into concrete types. Instead we expose functions on the interfaces that allow us to verify only what must be true about the counterparty client (e.g. unbonding period, validator hash, app hash, etc).

This allows chain developers to support arbitrary types of counterparty clients while still making essential checks. However, it requires that we come to consensus on what those essential checks are. It also requires extending the ClientState and ConsensusState interfaces.

Given that this requires more thought and is an extension of the changes necessary in the strict approach. I would advocate that we start with the strict approach requiring that chain developers register a function on the connection keeper that does concrete checks on a limited set of supported clients and move from there. We should also provide a default validation function chains can use.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[damiannolan]

Adding this back to the rollkit milestone and adding to current iteration. Myself and @chatton have been looking at this today.

[damiannolan]

Merged #5836 to feat branch to start bundling for builds. We can revisit the direction for API etc later as well as naming...

Some food for thought: SelfClientValidator -> ConsensusHost

// pkg 02-client/types

type ConsensusHost interface {
    GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error)
    ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error   
}

something like that could work 🤷🏻‍♂️ This can at least be a temporary solution for getting around the current constraints of the connection handshake for wasm clients - until such a time where wasm clients can encode with the 08-wasm envelope/wrapper