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

Allowing for unopinionated encoding structures of client and consensus states #5845

Open
3 tasks
colin-axner opened this issue Feb 15, 2024 · 1 comment
Open
3 tasks
Labels
02-client needs discussion Issues that need discussion before they can be worked on

Comments

@colin-axner
Copy link
Contributor

Context

For more in-depth context read 02-client refactor ADR

Pre-IBC launch

02-client had direct references to each light client, it stored/grabbed client/consensus states on their behalf. The direct references to each light client was problematic as adding a light client was not modular.

IBC launch

By IBC launch, 02-client had been refactored adding client and consensus state interfaces. The interfaces were acted upon by grabbing the client and consensus states from the store, unpacking into an any and using the client and consensus state interface functions to perform light client functionality.

Improvements enabled:

  • light clients could be developed in isolation from core IBC

v7

02-client was refactored once more to move setting of client and consensus states to the light client implementations. This removed many assumptions on how light clients might work as generalizing this is near impossible. All IBC light client implementations thus far have functioned in very different ways.

Improvements enabled:

  • generalized verify membership and non membership queries
  • clear interfaces for light client development + a guide
  • ability to pass in alternative client msg types (enabled on going work)
  • solomachine timeout support enabled

decoupling encoding/routing

The crux of the issues we have faced in light client development thus far is the deep coupling of how the values in the client store are encoded and how the light client modules are routed to. A poc was done to show we could decouple these two concerns. This removes the root architectural issue and gives light clients much more freedom.

Structurally speaking, the 02-client module is only strictly necessary for core IBC having a router to the light client modules based on its assigned identifier. A side benefit is that the 02-client module can also be used as a single gateway to and from the light client modules. This allows it to be an easy blocking point if it wishes to disable a light client module. It also creates a single unified rpc for external usage rather than external apps/relayers needing to comprehend where to find a light client module based on its client identifier.

Improvements enabled:

allowing unopinionated encoding structures

The poc mentioned above was implemented with the existing API's available and continued the assumption that light clients have types which fulfill the client and consensus states which can be decoded using the codec given to the 02-client router.
This is fine for the existing light client implementations in ibc-go as their types can easily be registered on the codec. It is problematic however in order to enable wasm contracts to skip using the wasmtypes.ClientState in order to wrap their own custom types.
There appear to be two benefits for removing this extra wrapper type:

  • alignment of encoding types across implementations (tendermint client implemented in contract vs native 07-tendermint implementation)
  • easier external interaction (less wasm specific logic is required when interacting with light clients written as contracts)

There are some difficulties that arise when trying to fully decouple encoding from routing at the 08-wasm level (a secondary router after 02-client).
Removing the ability for 02-client to decode client and consensus states requires:

  1. removing the interfaces entirely (deleting non-strictly required function such as ExportMetadata, ZeroCustomFields or moving essential functions to the light client module)
  2. migrating genesis types to take in []byte instead of google.protobuf.Any
  3. adding a query interface to each of the light client modules (the grpcs on 02-client must be capable of delegating to the respective light client modules)
  4. the initialize interface function must be capable of taking in additional information required for 08-wasm (codehash or human readable sub client type)
  5. adding v2 msg types for client creation

Issue 1

Following the merge of #5086, the following interface functions will remain on the ClientState:

type ClientState interface {
	proto.Message

	ClientType() string
	Validate() error

    // ExportMetadata must export metadata stored within the clientStore for genesis export
	ExportMetadata(clientStore storetypes.KVStore) []GenesisMetadata

	// ZeroCustomFields zeroes out any client customizable fields in client state
	// Ledger enforced fields are maintained while all custom fields are zero values
	// Used to verify upgrades
	ZeroCustomFields() ClientState
}

ExportMetadata and ZeroCustomFields are in the process of being removed.
It should be possible to remove Validate() and ClientType() should remain and only be used by the v1 MsgCreateClient.

Issue 2

Currently the 02-client keeper handles importing and exporting state on behalf of the light clients. It asserts that the client and consensus states are of type google.protobuf.Any, but this notion needs to be removed in order to allow any client state type to be written into state.

The GenesisState would need to be replaced to take in []byte for all keys to be set in the 02-client store. It would also need to export these key/values as []byte pairs as it should not try to inspect the existing values.

This is quite a bit of work.

Issue 3

In addition to importing/exporting state, the 02-client also acts a server responding to queries. If the 02-client is no longer capable of unmarshaling client store state, it must be capable of delegating to the light client implementations.

This requires us to add a light client module querier interface for all gRPC's in the 02-client gRPC. The 02-client module would pass through to these light clients.

As it is undesirable for 08-wasm to add more interfaces to the contract_api.go, 08-wasm would likely fulfill this querier by wrapping the bytes in its substore in a 08-wasm type (for readability) or returning the bytes directly (as desired by users).

The queries that need to be implemented:

 // ClientState queries an IBC light client.
  rpc ClientState(QueryClientStateRequest) returns (QueryClientStateResponse) {
    option (google.api.http).get = "/ibc/core/client/v1/client_states/{client_id}";
  }

  // ClientStates queries all the IBC light clients of a chain.
  rpc ClientStates(QueryClientStatesRequest) returns (QueryClientStatesResponse) {
    option (google.api.http).get = "/ibc/core/client/v1/client_states";
  }

  // ConsensusState queries a consensus state associated with a client state at
  // a given height.
  rpc ConsensusState(QueryConsensusStateRequest) returns (QueryConsensusStateResponse) {
    option (google.api.http).get = "/ibc/core/client/v1/consensus_states/"
                                   "{client_id}/revision/{revision_number}/"
                                   "height/{revision_height}";
  }

  // ConsensusStates queries all the consensus state associated with a given
  // client.
  rpc ConsensusStates(QueryConsensusStatesRequest) returns (QueryConsensusStatesResponse) {
    option (google.api.http).get = "/ibc/core/client/v1/consensus_states/{client_id}";
  }

  // ConsensusStateHeights queries the height of every consensus states associated with a given client.
  // TODO: this may not be required
  rpc ConsensusStateHeights(QueryConsensusStateHeightsRequest) returns (QueryConsensusStateHeightsResponse) {
    option (google.api.http).get = "/ibc/core/client/v1/consensus_states/{client_id}/heights";
  }

Issue 4

I explored this problem in-depth here, please refer to that issue for more information.

Issue 5

Assuming the above issues have been handled, then we will need a new message type for MsgCreateClient in order to take in []byte for the client and consensus states rather than a google.protobuf.Any.

The current proto definition of MsgCreateClient:

// MsgCreateClient defines a message to create an IBC client
message MsgCreateClient {
  option (cosmos.msg.v1.signer) = "signer";

  option (gogoproto.goproto_getters) = false;

  // light client state
  google.protobuf.Any client_state = 1;
  // consensus state associated with the client that corresponds to a given
  // height.
  google.protobuf.Any consensus_state = 2;
  // signer address
  string signer = 3;
}

This would likely need to be updated to look something like (depending on the solution to issue #4):

// MsgCreateClient defines a message to create an IBC client
message MsgCreateClient {
  option (cosmos.msg.v1.signer) = "signer";

  option (gogoproto.goproto_getters) = false;

  string client_type     = 1;
  bytes  client_state    = 2;
  bytes  consensus_state = 3;
  string signer          = 4;
}

This msg type will be required for any light client usage which does not set its client and consensus states as a google.protobuf.Any.

Additional notes

If the connection handshake is reworked to allow the underlying chain to have a counterparty which represents it not using a google.protobuf.Any type, then we would need new fields or types for MsgConnectionOpenTry and MsgConnectionOpenAck (for client and consensus state proof validation during the connection handshake).


Improvements enabled:

  • 08-wasm secondary routing structure removed allowing for compatibility with native client implemenations (connection handshake, upgrades were broken previously)
  • no opinionated encoding of client and consensus states (02-client acts as a pure passthrough router)

Conclusion

The 02-client sub-module has been difficult to define and understand. It has required many revisions, each with a whole hearted attempt to make it easier to work with. The decoupling of encoding and routing is a good architectural fix that will ease a lot of the pressure it has caused. The poc for the 02-client router also has shown that fixing this architectural issue is not too difficult and has minimal disruption to our users.

I am less enticed by the proposal to drop opinionated encoding structure used to encode client and consensus state types. I understand that this proposal introduces a benefit that also comes with a clear consequence. I do believe that at the moment it is useful that the 02-client module is able to decode the values set in its store by light clients.
I recognize that working with google.protobuf.Any's is problematic, especially externally, and that it would likely ease interaction. In addition, I think there is clear isolation between light client implementations and having to have some shared format for client and consensus states doesn't seem beneficial (outside of querying).

Given that there are several large changes required to achieve this goal, I would recommend holding off on this refactoring until there is clear agreement that dropping the loosely opinionated structuring of client and consensus states is the right choice and that it can be done in a minimally disruptive manner. It would be beneficial to continue to outline concrete improvements in the IBC workflow that would benefit from such a large structural change.

My main concerns with dropping the ability for self introspection is around queries. Does ibc-go return blobs of bytes for the client state and consensus state which it requires external users to understand? Is it possible to ask light client modules to take in a blob of bytes and return some human readable structure which could be returned back? I would feel more comfortable, if the narrative around this is solved, as I would like to avoid any major disruptions to existing relayers.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@Farhad-Shabani
Copy link
Member

In regard to this issue, the following context has been brought up internally with the ibc-go team. Copying here for our future reference:

Concerning the ibc-starknet project, Cairo's contract environment already offers a unique serialization method well-suited for their STARK proof generation. We're currently working on implementing ICS-02 (essentially a client router component). Use of the Any type for the client_state and consensus_state fields under the MsgCreateClient forces Protobuf decoding. This requires to add an extra layer of deserialization in Cairo, which is inefficient. Therefore, a MsgCreateClient with a client_type field and []byte type for client/consensus_state appears be offering greater compatibility across different hosting chains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client needs discussion Issues that need discussion before they can be worked on
Projects
None yet
Development

No branches or pull requests

2 participants