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

ICS2: Allow Light Client implementations more flexibility in setting/getting of client states and consensus states #569

Closed
colin-axner opened this issue May 3, 2021 · 7 comments
Labels
implementation Tracking an external implementation of the spec. tao Transport, authentication, & ordering layer.

Comments

@colin-axner
Copy link
Contributor

colin-axner commented May 3, 2021

Edit: see comment

Summary

IBC clients as defined by ICS 02 should contain a version to allow backwards compatible changes to proof key formats and proto definitions.

Problem

As we continue to iterate on IBC after 1.0.0 has been released, we have ran into difficulty in making clean backwards compatible changes to proof keys. This is because the first notion of versioning occurs at the second layer of the TAO stack (03 connection). At the first layer we create clients and consensus states, but without versioning information it is impossible to cleanly make changes at the 02-client level in a backwards compatible way.

It seems very strange to me that the first notion of versioning occurs at the second level. It has become clear to me that versions are primarily useful in providing information for usage at the layer above. For example, the current connection versioning is useful for establishing channel types. The current version in a channel is useful for providing information about the application. A client version would be useful in establishing information about proof keys and proto definitions used in connection handshake verification.

Currently, if we want to change our client state or consensus state, it is impossible to do so in a backwards compatible fashion. We create our clients and update them before we establish an IBC connection version. There is no flexibility in using different store keys or different proto definitions.

Proposed Solution

Add a client version into each IBC client which represents the versions of IBC the counterparty currently supports.

In ibc-go we would not store this version inside the ClientState. This is because we need to know the version of a client before we can even get the client as the version may indicate which store key to use. Instead we store the client version under a version key which should not be verified by counterpartys (this way the version key can change without waiting for the next major IBC version). Properly verifying the counterparty in connection handshakes acts as an implicit verification of the counterparty version. The client version is used to ensure the connection handshake can be completed properly, if the counterparty is using an unsupported version, the connection handshake would fail.

Positives

Proof keys and proto definitions for clients can change in a backwards compatible way.

Negatives

Another layer of versioning, may become confusing

Unresolved Questions

  • Does the client version need to be verified
  • How does the counterparty chain indicate it updates its versioning if the version is not stored in the client state

Opening to start discussion as I think this is a useful feature in helping us decrease the amount of times IBC needs to do a synchronous network upgrade. Would love to see someone sketch out this idea in more detail

@cwgoes
Copy link
Contributor

cwgoes commented May 4, 2021

Thank you, the general sense of this discussion makes sense to me. I don't entirely understand what role this version is playing - if the client version is not verified anywhere, does it really play a role in IBC at all (protocol-wise) or is it just a bit of "helpful metadata" e.g. for relayers to determine whether or not they should attempt a handshake between two chains?

Properly verifying the counterparty in connection handshakes acts as an implicit verification of the counterparty version.

This is true, and this is why I'm wondering what role a client version would serve, exactly? Would chains try to support multiple versions of their own light client so that verification can work in the handshake if consensus state fields changed, or something?

@colin-axner
Copy link
Contributor Author

colin-axner commented May 4, 2021

We shall discuss this later today on the core call, but the idea is to have access to a version which specifies information on how the counterparty chain expects to interact with IBC.

This applies both to proof keys and light client proto definitions, but because chainA needs to verify its own representation on chainB, we cannot change proof keys or light client proto definitions without a synchronous upgrade or disabling new connection creation between chains on the old version/new version.

Lets say we add a field to the tendermint proto definition. This changes the encoding value, if the new version has no understanding that the chain it is representing is actually on the old version, then connection handshake can never be established without the counterparty upgrading. If instead we had access to the version the counterparty is using, we could know that the counterparty requires using the old client state proto definition. The same logic can be applied to proof keys (which key do we set the consensus state under, new or old).

Attempting to cover the case of adding fields to the proto definition might introduce too much complexity since we need to generically set the IBC client in 02-client, but require light client specific information to understand what the correct client state is to set. I think the solution would require moving the setting of clients/consensus states into each specific light client.

Having the ability to change client/consensus keys in a backwards compatible fashion still seems like a worthwhile tradeoff for adding a client version

@colin-axner
Copy link
Contributor Author

colin-axner commented May 4, 2021

Another problem that has arisen is supporting changing the store prefix. The store prefix is set in the connection, thus if a counterparty wanted to change its store prefix, it would incapable of doing so without losing all its existing IBC connections.

If we wanted to move the prefix from the connection level to the client level, we run into the issue above about changing proto definitions. If we were able to change the proto definition using a client version, then we could also use a client version to indicate if we should be utilizing the connection prefix or the client prefix.

This all still seems hacky to me and maybe there's a more elegant solution, but I think the issue of backwards compatibility and supporting non IBC breaking upgrade paths is very important. Our technical debt tracker is growing fairly fast and I'm continually seeing proposed solutions require very hacky fixes to support backwards compatibility or require fracturing the connectivity of the IBC network.

While I think there aren't many consequences of requiring chains to upgrade to a new version now (due to the adolescents of IBC), I think this is an unsustainable upgrade option. If we don't add emphasis on supporting backwards compatible protocol changes we will either end up in a world where not all chains can communicate via IBC or we are stuck on a very old version of IBC. I'd like to take advantage of our existing problems to construct a good solution that allows us to do backwards compatibility in an if statement that is easy to deprecate and isolate

Ideally the IBC protocol does essentially rolling upgrades. Version 2 supports version 1 and 2 while version 3 supports version 2, 3. This gives chains a much longer upgrade period than a synchronous upgrade and the technical debt can be manageable with the ability for deprecation

@colin-axner
Copy link
Contributor Author

After some good discussion we decided to go with the following direction:

Instead of introducing a version that core IBC cannot understand, we should push getting/setting of client states and consensus state into the responsibility of each light client implementation. This would allow each light client to introduce their own versioning that is relevant to their own development. Light clients could set/get based on this version. This fix ideally would also address issue outlined in #562 where the IBC protocol makes assumptions on how to determine the timestamp for a client at some height

Once clients can set/get based on their own version, we can begin introducing robust backwards compatible fixes which simply do some sort of semantic translation before setting/getting clients

@colin-axner colin-axner changed the title ICS 02 Client Versioning Allow Light Client implementations more flexibility in setting/getting of client states and consensus states May 4, 2021
@colin-axner
Copy link
Contributor Author

colin-axner commented Jul 22, 2021

I think this issue can be tackled in incremental steps. I think the most useful first step is allowing IBC clients to set their own client state/consensus states

Looks like setting the client state and consensus states per the spec should occur in the IBC light client implementations as indicated by 02-client:

If the provided header was valid, the client MUST also mutate internal state to store now-finalised consensus roots and update any necessary signature authority tracking (e.g. changes to the validator set) for future calls to the validity predicate.

This is also true in the 07 specs

I'm going to move forward with modifying this in the ibc-go implementation. I will just start with the CheckHeaderAndUpdateState interface function we have

Edit: solo machine timeouts should be fixed before closing this issue

@mpoke mpoke changed the title Allow Light Client implementations more flexibility in setting/getting of client states and consensus states ICS2: Allow Light Client implementations more flexibility in setting/getting of client states and consensus states Mar 17, 2022
@mpoke mpoke added tao Transport, authentication, & ordering layer. implementation Tracking an external implementation of the spec. labels Mar 17, 2022
@mpoke
Copy link
Contributor

mpoke commented Mar 21, 2022

@colin-axner Is this issue still relevant?

@colin-axner
Copy link
Contributor Author

Not on the spec side (we are currently implementing these changes on ibc-go - spec is already correct). The solo machine fix can be tracked via #689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Tracking an external implementation of the spec. tao Transport, authentication, & ordering layer.
Projects
None yet
Development

No branches or pull requests

3 participants