Restructure 02-client to a more generalized light client handler

[benluelo]

08-wasm currently

We have been using wasm light clients internally for about 6 months now, writing light clients for multiple chains. While wasm clients do work well, there are several pain points in the general IBC interface (not just relating to wasm clients).

The main issue that we have come across is that the 08-wasm module isn't just a direct data transfer layer between ibc-go and the smart contracts:

• When relaying, it becomes impossible to know what the type of the light client is since all clients are 08-wasm-N. We have resorted to including a global in the wasm bytecode directly and parsing that, which is far from ideal - but it enables us to statelessly know how to parse the inner types without requiring an extra indirection with Any.

  • given that 08-wasm is intended to be a "proxy light client", one could say that the inner types aren't supposed to be parsed outside of the contracts - but how else is one supposed to construct the correct messages?

• Since wasm clients are now expected to write their own states in instantiate (imp: delegate store operation of clientState and consensusState in initialize to contract #4033), there is a possible discrepancy between the message types (wasm.*, wrapping the actual types) and the stored states. This makes both relaying and verification more complex, since either the relayer needs to unpack the wasm wrappers when sending messages to counterparties, and repack them when sending back, or the receiving end needs to do the packing and unpacking manually (which can get very expensive when working in highly restrictive environments). Counterparty clients (and/or the relayer) are also now expected to know whether or not the wasm client unwraps it's state, since it's no longer standard (i.e. always wrapped in wasm.*) - adding an additional layer of complexity.

This boils down to these 4 points:

1. The current implementation of 08-wasm light clients leaks implementation details into it's ibc interface, when it would ideally be a completely opaque wrapper around the wasm contracts, just sending and receiving bytes.

2. The ClientState envelope leaks information specific to the contract (code_hash), since the wasm module doesn't store any sort of mapping between client id and contract address. It also stores LatestHeight directly, instead of querying the contract, requiring duplication of information in a client state (a tendermint light client in wasm would have the LatestHeight stored both in tendermint.ClientState and wasm.ClientState!)

3. 08-wasm clients are always of type 08-wasm, which makes routing very difficult.

4. ConsensusState and ClientMessage have been whittled down to just being thin envelopes around the data passed to the contract, serving no purpose other than routing the message to the 08-wasm module.

Proposal

Add a field client_type to MsgCreateClient, and make the states arbitrary bytes:

type MsgCreateClient struct {
    // new
    ClientType string

    // now arbitrary bytes
    ClientState []byte
    ConsensusState []byte

    Signer string
}

This is used to route to the various light client modules, instead of downcasting types.

For the wasm module, add a MsgRegisterClient message:

type MsgRegisterClient struct {
    CodeHash []bytes
    ClientType string
}

This will register a client type in a global registry, that is "pre-seeded" with native modules:

// pseudocode
clientTypeGlobalRegistry["07-tendermint"] = *TendermintModule
clientTypeGlobalRegistry["09-solomachine"] = *SolomachineModule

How it works

The flow for creating client would be as follows:

{
  "client_type": "07-tendermint",
  "client_state": "0x...",
  "consensus_state": "0x..." 
}

this msg is received by core.keeper.Keeper, which then inspects the client type by checking it's registry:

// pseudocode
func (k Keeper) CreateClient(goCtx context.Context, msg *clienttypes.MsgCreateClient) (*clienttypes.MsgCreateClientResponse, error) {
    ctx := sdk.UnwrapSDKContext(goCtx)

    lightClientModule := keeper.registeredClientTypes[msg.clientType]
    // this call will do the necessary validations on the states - for tendermint, deserializing
    // Any<ibc.tendermint.[...]>, for wasm simply passing the bytes through to the contract to validate itself
    lightClientModule.CreateClient(clientType, clientState, consensusState)

    return &clienttypes.MsgCreateClientResponse{}, nil
}

If the client type points to an existing handler, the handler would handle the msg however it likes. for 07-tendermint, the tendermint module would deserialize to ibc.lightclients.tendermint.v1.ClientState/ConsensusState, and 08-wasm would pass the bytes through to the contract directly.

08-wasm specific flow

• upload code, with hash 0xabcd

• register client type with said code:

  {
    "client_type": "foobar",
    "code_hash": "0xabcd"
  }

  this will be sent to the 08-wasm module, which will then register the client_type "foobar" under the global client type registry module, pointing back to the 08-wasm module with the code hash.

• create a client with the new type:

  {
    "client_type": "foobar",
    "client_state": "0x...", 
    "consensus_state": "0x..." 
  }

  the keeper will look up the handler for the client type "foobar", and pass the message to it. the looked up module is (08-wasm, "0xabcd"), which instructs the wasm module to pass the client & consensus state to the contract (as arbitrary bytes). this will require the following change to 08-wasm/types/contract_api.go#InstantiateMessage:

  type InstantiateMessage struct {
      // no longer a type implementing Client/ConsensusState, instead just arbitary bytes that will be verified by the called contract
      ClientState    []byte `json:"client_state"`
      ConsensusState []byte `json:"consensus_state"`
  }

  the called contract will then do it's thing for instantiate (as specified in Delegate store operation of ClientState and ConsensusState in Initialize to contract #3956)

ibc.lightclients.wasm.v1.{ClientState,ConsensusState,ClientMessage} will all be removed.

Upgrading contracts

When upgrading a contract (#3956), the existing mapping of clientType => codeHash will be updated to point to the new codeHash.

Backwards Compatability

Since this proposal breaks backwards compatibility with existing relayers by changing the existing MsgCreateClient message, we propose deprecating ibc.core.client.v1.MsgCreateClient and create a new msg as follows:

message MsgCreateClient {
  option (cosmos.msg.v1.signer) = "signer";

  option (gogoproto.goproto_getters) = false;

  // add this field
  string client_type = 1;

  // these are just bytes, that are decoded by the module being routed to
  bytes client_state = 2;
  bytes consensus_state = 3;

  // signer address
  string signer = 4;
}

This would allow for keeping the same interface for existing native light clients (using ibc.core.client.v1.MsgCreateClient), but without support for 08-wasm clients - instead, introduce the above message as ibc.core.client.v2.MsgCreateClient that supports both native and non-native light clients via the routing system described above - the v1 messages could easily be routed to the v2 handler internally, and the v1 messages could be eventually deprecated.

The following other changes would be required to client.v1, to be added as new messages in client.v2:

MsgUpdateClient

message MsgUpdateClient {
  option (cosmos.msg.v1.signer) = "signer";

  option (gogoproto.goproto_getters) = false;

  // client unique identifier
  string client_id = 1;
  // client message to update the light client
  // NOTE: This is now bytes, instead of any - parsing is handled by the light client handler pointed to by `client_id`.
  bytes client_message = 2;
  // signer address
  string signer = 3;
}

Since MsgUpdateClient now encompasses both misbehaviour and regular updating, client_message is now bytes to allow for the target module to do the parsing, instead of enforcing Any - allowing for wasm clients to use alternate encodings depending on the usecase (protobuf oneOf, ethabi, etc). Existing native clients can and will continue to use this field as an Any.

MsgUpgradeClient

message MsgUpgradeClient {
  option (cosmos.msg.v1.signer) = "signer";

  option (gogoproto.goproto_getters) = false;

  // client unique identifier
  string client_id = 1;
  // upgraded client state
  // NOTE: Now bytes, instead of `Any`
  bytes client_state = 2;
  // upgraded consensus state, only contains enough information to serve as a
  // basis of trust in update logic
  // NOTE: Now bytes, instead of `Any`
  bytes consensus_state = 3;
  // proof that old chain committed to new client
  bytes proof_upgrade_client = 4;
  // proof that old chain committed to new consensus state
  bytes proof_upgrade_consensus_state = 5;
  // signer address
  string signer = 6;
}

client_state and consensus_state fields have changed for the same reason as MsgUpdateClient and MsgCreateClient - to allow for the target module to parse the bytes how it wishes.

TLDR

• deprecate the existing v1.MsgCreateClient, v1.MsgUpdateClient, and v1.MsgUpgradeClient interfaces
• add a new v2.MsgCreateClient, which contains a client type field
• add v2.MsgUpdateClient and v2.MsgUpgradeClient, changing their Any state fields to bytes
• add wasm.v1.MsgRegisterClient
• remove the envelope types from wasm.v1

We would also like to note that we are ready and willing to implement this ASAP if this is accepted. Since it is much more difficult to do larger scale structural changes after the release of a feature (08-wasm), we believe that this is the perfect time to make this change.

---

For Admin Use

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

[crodriguezvega]

Thank you for opening the issue and for the detailed writeup, @benluelo. Please give us some time to discuss and we will come back to you as soon as possible.

[colin-axner]

Amazing!!! Thank you @benluelo! It's great to have a good issue outline in addition to a concrete proposal, thank you for spending the time to open up this issue! I had been planning to write up a very similar issue this week so great timing. I appreciate the user perspective you bring to this problem/solution 🙏

I agree it is quite problematic. This is quite the structural design change, so we find it important to due diligence of thinking through all aspects of consideration. You have done a great job thinking about the effects of the proposed change. I will add some color to some other areas worth considering.

In general, we need to account for backwards compatibility, forwards compatibility, and our existing commitments. It's quite the balance to strike. Preventing unnecessary headaches of the future, while also living up to the timelines we have committed to and enabling future design revisions.

I'd like to start by revisiting the issues you face as well as outlining other issues I've seen within this codebase.

Problem

I was thinking about this issue more generically at the 02-client layer last week, here's how I summarized the issue:

In its current state of 02-client it fulfills two purposes:

• it routes client related msgs/logic to the associated client modules
• it acts as a guardian of the client stores by providing the prefixed store to a client module

In order to route to client modules, it acts on the encoded client state structure set in the client prefixed store.
This is problematic for a couple reasons.

1. It requires all information a client module needs to be encoded into the client prefixed store (or set as a global)
2. It overlaps the specified encoding structure with routing, thus if two implementations would like to fulfill the same client spec, they must use different encodings (07-tendermint vs 08-wasm with tendermint contract, Remove dependency on 08-wasm from ibc-go #4779 (comment))

You have noted 4 additional points that are downstream issues of 08-wasm interacting with this structure, in addition to two concrete problems. I'd like to take note of the immediate issues relative to the structural issues, in case we need to compromise and have immediate fix vs long term fixes.

Immediate issues

When relaying, it becomes impossible to know what the type of the light client is since all clients are 08-wasm-N

To clarify, when relaying, you'd like an easy way to know which client type is associated with the code hash in order to unmarshal the embedded data? I can totally understand why this would a pain point.

Since wasm clients are now expected to write their own states in instantiate (#4033), there is a possible discrepancy between the message types (wasm.*, wrapping the actual types) and the stored states.

Can you expand on this point more? As noted above, because 02-client uses the stored state for routing, ibc-go asserts that the stored state must be a wrapped wasm client, otherwise it will get incorrectly routed and things will break. We are also adding an defensive check to assert contracts don't accidentally modify their routing during regular execution (only allow routing changes during migration). Are you referring to non-client state types or is there something I am missing? Thanks

Structural issues

1. The current implementation of 08-wasm light clients leaks implementation details into it's ibc interface

Can you expand on this? What is being referred to as "it's ibc interface"? Is this referring to the proto types?

2. The ClientState envelope leaks information specific to the contract (code_hash), since the wasm module doesn't store any sort of mapping between client id and contract address.

This is in reference to the immediate issue of understanding the client type based on code hash? I guess for the secondary point you made here, it is also affected by the fact that the routing mechanism is the client state interface. In this case, the routing mechanism for wasm (wasm client state) is being forced to include unnecessary info like latest height.

08-wasm clients are always of type 08-wasm, which makes routing very difficult.

Yes I agree. The clientID/type become very vague for instantiated wasm clients

ConsensusState and ClientMessage have been whittled down to just being thin envelopes around the data passed to the contract, serving no purpose other than routing the message to the 08-wasm module.

Yes this is unfortunate.

Desired properties

In the next section, I will define additional problems I have seen or foresee, but I find it easier to discuss defining desired properties for 02-client.

1. 02-client is a gateway router only.
2. Light client modules have full control over their client prefixed store.
3. Light client modules can access additional information without breaking the client state/consensus state interfaces or using globals.
4. Light client modules have a well defined interface they must fulfill to interact with 02-client and other actors.
5. Routing to light client modules is isolated from the encoding structures set in the client store.

Optional:

1. Light client modules are accessible without going through 02-client/core ibc

Additional problems

Here's a quick case study of 08-wasm and a client usage being discussed called conditional clients.

08-wasm

1. 08-wasm requires globals.

   • If 08-wasm could include additional structures that don't need to be encoded in the client state, all globals set here could be removed and set in the client module.

2. 08-wasm light client contracts generate a secondary encoding structure breaking existing features (connection handshake, client upgrades)

   • Currently all contracts must create a secondary encoding structure (08-wasm wrapper) in order to fulfill ibc-go routing requirements, separating routing from encoding allows for reuse of a single expected encoding structure.

3. Light client modules are not provided the clientID they are acting on.

   • If light client modules are guardians of their own prefix store, the clientID they act on will be a standard request argument, allowing complex self introspection on client store to be removed from 08-wasm https://github.com/cosmos/ibc-go/blob/feat/wasm-clients/modules/light-clients/08-wasm/types/store.go#L195

These are in addition to the problems y'all have outlined, such as the unnecessary wrapper types 08-wasm generates to fulfill the 02-client routing interface.

Conditional clients

Conditional clients are client types that are dependent another a client to pass verification. That is client X will depend client Y to pass verification. When a proof of packet commitment occurs, client X is passed both proofs for X and Y. It then will need to invoke client Y with proof Y. If success, continue process proof X for client X, otherwise error.

1. Conditional clients require API breaking changes to access additional information
   • If the routing system already existed, conditional clients could embed whatever extra information they need into their light client module (such as direct access to the 02-client keeper)

Long term solution space

In addition to standardizing the routing at 02-client we will we want to reuse the same concepts for the other parts of ibc-go that require pure data transfer. I noted in this ADR that currently ibc-go does routing of information to:

• light clients
• ibc applications

but it does it in two distinct ways, with the approach of routing to a module using a pre-defined route as being the superior mechanism. Routing based on the interface was poor design and I'm sorry it has taken so long to address.

Keeping forwards compatibility in mind, ibc-go will also need to route information based on the framework which is calling it. Currently this is the cosmos SDK, but as we are in the early stages of trying to investigate allowing for other frameworks to call into ibc-go, this design consideration is worth keeping in mind.

Ideally we would have one routing approach to all 3 areas. I don't want to delay improvements based on theoretical purity/implementation across the board, so as long as we feel confident the fix for 02-client could be reused for ibc applications or for the framework driving ibc-go, I would be happy to see an implementation move forward. To do this, it might help to outline as the problem/solution in generic terms not too specific to 02-client.

Proposed solution

Adding some commentary on your proposed solution.

1. Add a field client_type to MsgCreateClient, and make the states arbitrary bytes:

Essentially:

1. Add route to request type + make request args for a specific request type to be more generic

The question that comes into my head is if it is necessary to add the route into all request types. I think the answer is yes off the top of my head, since the proto url for the request type is not destined for a single location but can be reused. Maybe there's a world where each client has its own request types and you can use the proto url, but then you are creating different request types entirely, so to summarize I agree any msg used for routing will require a route field 👍

For the wasm module, add a MsgRegisterClient message:

Got it, so for each code hash, you want to add in a separate entry. This is beneficial because the code hash is the routing information, so then it wouldn't need to be embedded in some other information 08-wasm needs. Nice suggestion 👍 Instead of having a separate msg type, we can add the route information to MsgStoreCode?

Modifying your code to be generalized:

// pseudocode
func (k Keeper) CreateClient(goCtx context.Context, msg *clienttypes.MsgCreateClient) (*clienttypes.MsgCreateClientResponse, error) {
 ctx := sdk.UnwrapSDKContext(goCtx)

    lightClientModule := k.router[msg.route]
    
    // this call will do the necessary validations on the states - for tendermint, deserializing
    // Any<ibc.tendermint.[...]>, for wasm simply passing the bytes through to the contract to validate itself
    lightClientModule.CreateClient(clientType, clientState, consensusState)

    return &clienttypes.MsgCreateClientResponse{}, nil

There is a lot of discussion which could be had on the light client module interface (whether it uses specific args or msg types), but lets wait on that for a second.

I would like to note, it should be possible to maintain the existing routing mechanism for legacy light clients, while enabling an opt in updated approach

When upgrading a contract (#3956), the existing mapping of clientType => codeHash will be updated to point to the new codeHash.

Good point. This should be straight forward

Since this proposal breaks backwards compatibility with existing relayers by changing the existing MsgCreateClient message, we propose deprecating ibc.core.client.v1.MsgCreateClient and create a new msg as follows:

There is probably a way you could squeeze the new changes into the old type, but I agree to make a new type to use the new flow.

This would allow for keeping the same interface for existing native light clients (using ibc.core.client.v1.MsgCreateClient), but without support for 08-wasm clients - instead, introduce the above message as ibc.core.client.v2.MsgCreateClient that supports both native and non-native light clients via the routing system described above - the v1 messages could easily be routed to the v2 handler internally, and the v1 messages could be eventually deprecated.

Amazing yea. We can support both mechanisms, making v2 be opt in. Clients could choose to drop support for v1 once they have fully migrated and relayers have been integrated.

client_message is now bytes to allow for the target module to do the parsing, instead of enforcing Any

I really like this benefit. It was one I didn't consider before but I think it's a really nice flexibility increase!!

We would also like to note that we are ready and willing to implement this ASAP if this is accepted. Since it is much more difficult to do larger scale structural changes after the release of a feature (08-wasm), we believe that this is the perfect time to make this change.

❤️ ❤️

Balancing short/long term solutions

It is my job to ensure we meet our existing commitments while enabling future improvements as soon as possible. With this in mind, I am particularly thinking about v0.1 version of wasm. It's important not to block releases on proposed features in my opinion, as it increases stress for everyone involved and can lead to poor decision making.

I am really happy you opened this issue up, because it helps to have the proposal written down and I like it a lot! I like that there's an additional benefit I did not see. I am not convinced we need to block v0.1 of wasm on this proposed change.

To address your immediate concerns, we can add a query api to the contract (credit to @srdtrk for the idea). This is just a query that can return the client type. We can expose a grpc for this as well.

The secondary consideration for the impact of these changes is with regards to how 08-wasm could handle v1 vs v2. 08-wasm already handles the v1 routing. The primary thing 08-wasm has difficulty changing is the contract api. In the case where we ship v0.1 with v1 routing, the wrapper types would still be in the contract api, but given that the bytes still need to be unmarshaled by the contract in either case, it seems like only an unnecessary structure/additional storage bytes. I agree it'd be nice to fix, but I'm not convinced the benefit is worth delaying the initial release of 08-wasm

I believe in the long run, 08-wasm will need to manage handling older and newer versions of the contract api anyways.

I guess we could optionally modify the contract api to remove the reference to the wrapper clients. The contracts would still need to store the wrapper clients in the short term, but could migrate when the v2 routing is added.

Conclusion

Very much in favor of the direction! It is already in line with what I had in mind. I think there will be a lot of nice benefits to making this switch. However, I don't think we should block a v0.1 08-wasm release on these changes. The immediate benefit doesn't seem to be there for me and with any large change, there's likely a hidden roadblock somewhere. I will be happy to see this work move forward in parallel.

I'd like to get a second to discuss my perspective with the team to get their input. We will also need to agree on how the light client module interface should be structured. You have outlined one proposed version, another would be to create request types for each rather than individual args.

Thanks again!

[colin-axner]

We decided internally that would we move forward with this proposal. We also met with Union to discuss collaboration on the feature. I've opened #5565 as an outline for the initial design. Once these changes are in a semi-stable state, we will open up any smaller issues necessary to help get this feature to the finish line. Once I get a chance, I will also open up issues that remove technical debt by taking advantage of the increased flexibility (removing globals in 08-wasm for example)

[colin-axner]

related issues: #5844, #5845

[colin-axner]

v9 will be releasing the decoupling of routing for light client modules from their encoding. In addition it also removes the self validation of client and consensus states (making the encoding structure primarily used for queries and not verification). Is there any additional concerns we should address @benluelo to close this issue? Otherwise I would recommend we close and open additional improvements as separate issues.

[benluelo]

I think you've covered everything mentioned in this issue & discussion - can't wait for v9, thanks again to everyone who worked on this! 🙌