ICS 28: WASM Client

[ParthDesai]

Reopened this PR: #506

cc: @AdityaSripal @hxrts @cwgoes

[cwgoes]

Apologies for taking awhile to review this. Besides the nits, my substantive comment is that it would be better to allow the WASM code to read/write state and specify all the datatypes rather than partially specifying them here, I think.

[cwgoes]

Does it? The chain hosting the client should not be exposed to any correctness risk, only users who use the client

[mkaczanowski]

It doesn't.

I've rephrased this:

This specification depends on the correct instantiation of the `Wasm client` and is decoupled from any specific implementation of the target `blockchain` consensus algorithm.

@cwgoes ^^ does that look good to you?

[mkaczanowski]

@cwgoes bumping the question :)

[cwgoes]

please capitalise WASM consistently (or not, but pick one)

[ethanfrey]

I believe "Wasm" (not WASM or wasm) is the preferred abbreviation

[cwgoes]

Then let's use that consistently :)

[mkaczanowski]

"WASM" has been replaced with "Wasm" across the ICS. I think we can resolve this thread :)

[cwgoes]

why can't this whole type be up to the WASM client code

[ParthDesai]

@cwgoes That was the initial idea.

ClientState, ConsensusState and Misbehaviour interfaces have some functions like GetProofSpec, IsFrozen, Validate which do not give access to store and/or are being called during tx creation on client side. In both cases we do not have access to the WASM VM. But, we still have to return valid answer as per semantics of that function and to do that we need to have some structure instead of opaque blob of byte only understood by WASM code.

[ethanfrey]

This is a good point - if the spec defines data structures that must be validated without any access to storage of the VM, then these types must be understood by the Go code.

However, this is in the spec level and I wonder if "In both cases we do not have access to the WASM VM." is obvious in the spec or an implementation detail we could work to improve.

Could you link the functions that require inspection of these types, so they cannot be defined in Wasm. It might make sense to revisit those definitions with @cwgoes and see if we can make them more flexible.

[cwgoes]

Hmm, that's an excellent point, but I think we may want to alter the structure of IBC or whatever code is involved here so that the VM can be accessed - sounds like we're on the same page @ethanfrey.

[mkaczanowski]

@cwgoes @ethanfrey These are methods without access to storage:

Client State (link):

ClientType() string
GetLatestHeight() Height
Validate() error
GetProofSpecs() []*ics23.ProofSpec
ZeroCustomFields() ClientState

Changes (with the latest IBC updates, some things can be removed, see comments):

interface ClientState {
  codeId: []byte
  data: []byte
  
  frozen: boolean // to be removed (deprecated, in favor of "Status" method, that has access to storage)
  frozen_height: Height // ^^ ditto
  latest_height: Height
  proof_specs: ProofSpec // not listed, but should be here
  type: String // likely to be removed (we'll return "10-wasm" client type instead) - I'll explain in another comment
}

After:

interface ClientState {
  codeId: []byte
  data: []byte
  
  latest_height: Height
  proof_specs: ProofSpec // not listed, but should be here
}

Execution context (who is the caller? is there a WASM VM available?)

• GetProofSpecs() - ibc-go can't see any reference (also checked cosmos-sdk repo)
• GetLatestHeight() - ibc-go all search results point to test files
• Validate() - often called in ValidateBasic here: modules/core/03-connection/types/msgs.go
• ZeroCustomFields - ibc-go seems we can access KV Store here

[ethanfrey]

Thanks for the detailed list of those methods.

ProofSpecs is static over the lifetime of the client and there may be somewhere to include this in code not in state.

LatestHeight obviously requires a query to the KV store. How does the current Go code handle returning this without storage access?

[AdityaSripal]

Thanks, @ParthDesai and @mkaczanowski for the explanations. In principle, I'm ok with breaking any Go APIs that are not expected by counterparty chains or relayers if it will help make WASM and future client code more clean.

ClientType() string: Agreed that you should just be returning 10-wasm here.

Validate() error: Since this is getting used in messages, it has to be stateless. Any additional checks you want to do with state must be done elsewhere. We could rename this to ValidateBasic to emphasize this.

ZeroCustomFields: This is just used for upgrades, we can provide store here if necessary.

---

Interfaces that may be expected by counterparty chains

GetProofSpecs: The proof specs get used in the client Verify functions. This of course doesn't require an external interface, in face we just use the clientstate field directly. However, it also gets used on connection handshake to ensure that the counterparty client is using the correct proof specs: here. Currently, it is not using the interface itself but rather getting the ProofSpecs field directly because we are making an assumption that all counterparty clients for an SDK chain are 07-tendermint clients. However, once WASM client is introduced we need to remove that assumption since we could have a wasm smart contract client tracking the SDK chain (this could be better than 07-tendermint since it may have easier upgrade logic). So once that happens, we will have to rely on the interface itself. We cannot give you access to the store here, since this is getting called by the counterparty chain, which does not contain the store for your light client. You will need to find a way to provide this information in the client state struct itself.

GetLatestHeight() Height: Similar to the ProofSpec case, we check that the latest height of the counterparty client is not greater than our height during connection handshake here. Again we currently use the field directly, but will have to switch to using an interface.

We don't strictly require the interface, seeing as we could just execute separate ValidateSelfClient logic for wasm/go clients, however the wasm client must still have proof specs and latest height directly retrievable from its client state in order to perform all necessary checks on connection handshake.

[AdityaSripal]

LatestHeight obviously requires a query to the KV store. How does the current Go code handle returning this without storage access?

We "cache" the latest height directly into the client state on update: https://github.com/cosmos/ibc-go/blob/2e95805e2d/modules/light-clients/07-tendermint/types/update.go#L251

[mkaczanowski]

Thanks, @AdityaSripal for such a detailed response

however the wasm client must still have proof specs and latest height directly retrievable from its client state in order to perform all necessary checks on connection handshake.

We'll update the ICS with:

interface ClientState {
  codeId: []byte
  data: []byte
  
  latestHeight: Height
  proofSpecs: []ProofSpec
}

ZeroCustomFields: This is just used for upgrades, we can provide a store here if necessary.

Since the method intends to reset some fields in the ClientState, I don't think we need storage access. However, possibly changing the interface to allow returning the error would be helpful. Please see the initial implementation:
https://github.com/ChorusOne/ibc-go/blob/ics_28_wasm_client/modules/light-clients/10-wasm/types/client_state.go#L228

what do you think?

[mkaczanowski]

updated the ClientState interface here

[cwgoes]

ditto, same question, why can't this all be up to the WASM code?

[ParthDesai]

@cwgoes See my previous comment.

[cwgoes]

See my previous response

[mkaczanowski]

These are methods without access to storage:

Consensus State (link):

ClientType() string
GetRoot() Root
GetTimestamp() uint64
ValidateBasic() error

Execution context (who is the caller? is there a WASM VM available?)

• GetRoot() - ibc-go doesn't seem to be called anywhere
• GetTimestamp() - here we don't have VM access
• ValidateBasic() - one place where there is no kv access

[AdityaSripal]

GetRoot() Root: This is only getting used in internal client logic, we could remove it altogether if desired.

GetTimestamp() uint64: We need the timestamp of the consensus state to process packet timeouts. We can give you the client store here. The function you posted is merely a convenience, we could use the local time instead.

ValidateBasic() error: Again, this must be a stateless check. You can do very basic checks (e.g. codeId not empty), or even a no-op, and perform your stateful checks later on in message execution when you have access to the store.

[mkaczanowski]

GetRoot() Root: This is only getting used in internal client logic, we could remove it altogether if desired.

Yes, if GetRoot is removed from the interface, then there is no need for us to require root: MerkleRoot field anymore.

GetTimestamp() uint64: We need the timestamp of the consensus state to process packet timeouts. We can give you the client store here. The function you posted is merely a convenience, we could use the local time instead.

Great, then we can modify the function modules/apps/transfer/client/cli/tx.go and the only place where GetTimeout() is called is 03-connection/keeper/keeper.go and here where we have access to the VM.

To pull the timestamp from the Wasm light client we would have to:

• call the VM, which may fail. The current interface GetTimestamp() uint64 doesn't allow to return an error, so would it be alright to change the signature to GetTimestamp() (uint64, error)?
• ConsensusState doesn't hold codeId that is required to call the WasmVM. So far, it was only stored by ClientState, so here are the possible options:
  • change the signature ConsensusState.GetTimestamp() uint64 -> ConsensusState.GetTimestamp(cs ClientState) uint64 but that looks weird
  • add the codeId field to ConsensusState
  • other options?

[mkaczanowski]

updated ConsensusState here but this assumes that GetRoot() is removed from the current interface.

[cwgoes]

why can't this be up to the WASM code

[ParthDesai]

@cwgoes See my previous comment.

[cwgoes]

See my previous response

[mkaczanowski]

These are methods without access to the storage:

Misbehaviour (link):**

ClientType() string
GetClientID() string
ValidateBasic() error

This seems to be a stale definition:

interface Misbehaviour {
  fromHeight: Height  // doesn't seem to be used
  client_id: string, // missing, used by GetClientID()
  h1: Header 
  h2: Header
}

More correct version:

interface Misbehaviour {
  client_id: string,
  h1: Header 
  h2: Header
}

[ethanfrey]

With the two headers, we should be able to compare the headers for misbehavior without storage access, just code.

[mkaczanowski]

yes, that sounds right

[AdityaSripal]

With the two headers, we should be able to compare the headers for misbehavior without storage access, just code.

Agreed.

[mkaczanowski]

updated Misbehaviour here

[cwgoes]

can we give the WASM code limited state read/write access instead, it would be more flexible

[ethanfrey]

I agree completely. Since you are using wasmvm, it would not be hard to give each contract a sandboxed storage area, like we do in x/wasm. The contract knows what data it needs and can just store it locally.

However, it seems you actually need to write to some particular ibc keys here. Could you not just make a prefix store of clients/{identifier} and pass that to the contract? It could then store client state under the empty key and consensus state under /consensusStates/{header.height}. Meaning full access to the state it manages, but no access to other ibc state

[ParthDesai]

@ethanfrey @cwgoes I did not want WASM code read/write access to storage, because current version of WASM manager does not allow validation of the WASM code thoroughly. So, to err on safe side, I removed storage access to the WASM code.

[ethanfrey]

If wasm can return results that will be written to some specified storage locations, you could also provide the wasm code direct read/write access to only those storage locations.

I would assume wasm code is potentially malicious (even if this code must have been verified by governance to run), but you should be able to define a sandbox where it can work without impacting other modules (just it's own state). It is trusted to approve packets for a subset of clients/connections/channels, so should be able to write any data relative to those without increasing the attack surface at all

[cwgoes]

What do you mean, "does not allow validation of the WASM code thoroughly"? That sounds dangerous. I would be far more worried about compute issues than storage issues, since storage usage is gas-metered and safely sub-store-constrained by the keeper.

[AdityaSripal]

If wasm can return results that will be written to some specified storage locations, you could also provide the wasm code direct read/write access to only those storage locations.

Agreed. The WASM code handlers should just be given the client-id prefixed store. This is already what is given to the client handlers. Core IBC already prevents the ability for one client to read/write outside of its sandbox. So WASM can follow same security model. Just pass the store that the light client receives directly into WASM, it is already sandboxed by core IBC.

But who can define such a wasm code as a light client? This is out of scope for ICS repo maybe, but I feel essential to understand how this integrates in a blockchain, and what security guarantees we need.

Can I upload some wasm as light-client-foo and then make a new client/connection on top of it? Does governance need to approve it? If this code misbehaves what is the worst that can happen? (I assume fake packets in any channels built on top of this client).

We may rollout more conservatively, but I'm of the belief that the spec itself should enable random users uploading wasm light client code. This is a big selling point of WASM clients imo, since you don't need buy in from governance to support your eclectic choice of consensus algorithm.

Since IBC is designed to sandbox any issues, any malicious light client code submitted by user can only affect users of that light client. Though of course, they can do whatever they want to the users of their light client.

Here it will be incumbent on counterparty chains to enforce wasm contract correctness, by saying I will only accept a connection where the client has this smart contract code hash(es). This way we do not allow malicious clients to be submitted against correct chains. By creating the restriction on the destination chain rather than the source chain, we can enable all the flexibility of dynamic light clients while still preventing malicious connections over correctly implemented chains.

[AdityaSripal]

Though we should double check that 02-client does not make any assumptions that something will be in the store in ways that will affect its overall functioning.

It's ok if the client becomes non-functional or downright malicious imo.

What would be bad, is if deleting something (like say the clientstate itself), causes the client handler to panic and shutdown the node. In case, the wasm contract does something like this, we should just return an error and fail th tx whenever client state is needed

[mkaczanowski]

Just pass the store that the light client receives directly into WASM, it is already sandboxed by core IBC.

In the initial implementation we already do that, this is, use the default flow where 02-client/keeper/client.go gets client-id prefixed store from ClientStore(), and passes it on, to the client handle.

@AdityaSripal @cwgoes I am not sure how to phrase it (could use your help here), but here's a try:

    store = getStore("clients/{identifier}")
    isValid, consensusState = codeHandle.validateHeaderAndCreateConsensusState(store, clientState, header, epoch)
    store.set("/consensusStates/{header.height}", consensusState)
    // save the client
    store.set("", clientState)

We may rollout more conservatively, but I'm of the belief that the spec itself should enable random users uploading wasm light client code. This is a big selling point of WASM clients imo, since you don't need buy in from governance to support your eclectic choice of consensus algorithm.

Agreed.

[AdityaSripal]

Ok so what is needed beyond what is already available in the implementation. If anything?

[mkaczanowski]

To let the light-clients update their state, IBC interface would have to change slightly (it's going to be covered by #569).

For example:

CheckHeaderAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, Header) (ClientState, ConsensusState, error)

to smth like this:

CheckHeaderAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, Header) error

because the {client,consensus} states are going to be updated within CheckHeaderAndUpdateState lightclient method

[cwgoes]

ditto, can we give the WASM code limited state read/write access instead, it would be more flexible

[cwgoes]

@ParthDesai Bump on this, any thoughts on my comments? Anything I should clarify?

[ParthDesai]

@ParthDesai Bump on this, any thoughts on my comments? Anything I should clarify?

@cwgoes Replied. Please check and let me know.

[cwgoes]

@ParthDesai Bump on this, any thoughts on my comments? Anything I should clarify?

@cwgoes Replied. Please check and let me know.

Responded!

[mkaczanowski]

Methods that don't have access to storage

Header (link)

ClientType() string
GetHeight() Height
ValidateBasic() error

This definition looks stale. It should be:

interface Header { 
   data: []byte,
   height: Height, // required by GetHeight() 
}

[AdityaSripal]

ditto on ValidateBasic

I'm a bit hesitant to make GetHeight stateful here since it gets used in ValidateBasic checks and without it the ValidateBasic isn't doing much at all. If you could clarify why you need access to storage, and the difficulties with keeping it stateless that would be helpful.

[mkaczanowski]

@AdityaSripal sorry I phrased it wrongly. The storage is not needed here, but I wanted to give some context as to why the height: Height is a separate field (not a part of data), this is during the UpdateClient call the GetHeight() is required to set the consensus state.

That's the only significant use case of GetHeight I found

[mkaczanowski]

updated Header here

[AdityaSripal]

Right but I'm saying ValidateBasic should be doing some check on Height and because that is stateless, there should be a stateless access to Height here as well even if not strictly used directly in code. cc: @colin-axner what do you think?

[colin-axner]

Sorry to interject without reading through all the context, but I think GetHeight() will be removed soonish. In a couple months I'd like to start work on #569 which would move setting of IBC client information to light client implementations. This would mean the consensus state would be set by the light client, which could directly cast to its Header definition as opposed to using the header interface. The header interface would only be really used to pass the header from the user message to 02-client and finally to the IBC client (which would not require GetHeight() usage)

I'm not really sure what this means for the decision being discussed, but whatever is decided should be fine since it'll likely be short lived. It sounds like GetHeight will still be stateless in the short term? The height field would just be removed from data once GetHeight is removed

[AdityaSripal]

Thanks for pinging me on this. I'd like to confirm why storage is needed in these client interfaces from WASM.

From my understanding, this information would be in data but you would need to retrieve the smart contract code from WASM store in order to parse it and return the value. If you can't retrieve code, then you will have to store directly in client state so that go handlers can implement the interfaces. Is that correct? If not, please clarify why WASM client needs storage to implement these interfaces.

In general, I'm open to changing the interfaces for ClientState and ConsensusState since those are mostly created and stored directly on-chain, and the GO apis can change.

I'm less comfortable changing the Header and Misbehaviour interfaces, since they are very client facing and so having some stateless interfaces is useful here.

I also believe you should just be passing the client-prefixed store into the wasm handlers. I don't see any security risk there. It's already an assumption of core IBC that clients may do arbitrary things to their own client-prefixed store.

[AdityaSripal]

Thanks, @ParthDesai and @mkaczanowski for the explanations. In principle, I'm ok with breaking any Go APIs that are not expected by counterparty chains or relayers if it will help make WASM and future client code more clean.

ClientType() string: Agreed that you should just be returning 10-wasm here.

Validate() error: Since this is getting used in messages, it has to be stateless. Any additional checks you want to do with state must be done elsewhere. We could rename this to ValidateBasic to emphasize this.

ZeroCustomFields: This is just used for upgrades, we can provide store here if necessary.

---

Interfaces that may be expected by counterparty chains

GetProofSpecs: The proof specs get used in the client Verify functions. This of course doesn't require an external interface, in face we just use the clientstate field directly. However, it also gets used on connection handshake to ensure that the counterparty client is using the correct proof specs: here. Currently, it is not using the interface itself but rather getting the ProofSpecs field directly because we are making an assumption that all counterparty clients for an SDK chain are 07-tendermint clients. However, once WASM client is introduced we need to remove that assumption since we could have a wasm smart contract client tracking the SDK chain (this could be better than 07-tendermint since it may have easier upgrade logic). So once that happens, we will have to rely on the interface itself. We cannot give you access to the store here, since this is getting called by the counterparty chain, which does not contain the store for your light client. You will need to find a way to provide this information in the client state struct itself.

GetLatestHeight() Height: Similar to the ProofSpec case, we check that the latest height of the counterparty client is not greater than our height during connection handshake here. Again we currently use the field directly, but will have to switch to using an interface.

We don't strictly require the interface, seeing as we could just execute separate ValidateSelfClient logic for wasm/go clients, however the wasm client must still have proof specs and latest height directly retrievable from its client state in order to perform all necessary checks on connection handshake.

[AdityaSripal]

LatestHeight obviously requires a query to the KV store. How does the current Go code handle returning this without storage access?

We "cache" the latest height directly into the client state on update: https://github.com/cosmos/ibc-go/blob/2e95805e2d/modules/light-clients/07-tendermint/types/update.go#L251

[AdityaSripal]

GetRoot() Root: This is only getting used in internal client logic, we could remove it altogether if desired.

GetTimestamp() uint64: We need the timestamp of the consensus state to process packet timeouts. We can give you the client store here. The function you posted is merely a convenience, we could use the local time instead.

ValidateBasic() error: Again, this must be a stateless check. You can do very basic checks (e.g. codeId not empty), or even a no-op, and perform your stateful checks later on in message execution when you have access to the store.

[AdityaSripal]

ditto on ValidateBasic

I'm a bit hesitant to make GetHeight stateful here since it gets used in ValidateBasic checks and without it the ValidateBasic isn't doing much at all. If you could clarify why you need access to storage, and the difficulties with keeping it stateless that would be helpful.

[AdityaSripal]

With the two headers, we should be able to compare the headers for misbehavior without storage access, just code.

Agreed.

[AdityaSripal]

If wasm can return results that will be written to some specified storage locations, you could also provide the wasm code direct read/write access to only those storage locations.

Agreed. The WASM code handlers should just be given the client-id prefixed store. This is already what is given to the client handlers. Core IBC already prevents the ability for one client to read/write outside of its sandbox. So WASM can follow same security model. Just pass the store that the light client receives directly into WASM, it is already sandboxed by core IBC.

But who can define such a wasm code as a light client? This is out of scope for ICS repo maybe, but I feel essential to understand how this integrates in a blockchain, and what security guarantees we need.

Can I upload some wasm as light-client-foo and then make a new client/connection on top of it? Does governance need to approve it? If this code misbehaves what is the worst that can happen? (I assume fake packets in any channels built on top of this client).

We may rollout more conservatively, but I'm of the belief that the spec itself should enable random users uploading wasm light client code. This is a big selling point of WASM clients imo, since you don't need buy in from governance to support your eclectic choice of consensus algorithm.

Since IBC is designed to sandbox any issues, any malicious light client code submitted by user can only affect users of that light client. Though of course, they can do whatever they want to the users of their light client.

Here it will be incumbent on counterparty chains to enforce wasm contract correctness, by saying I will only accept a connection where the client has this smart contract code hash(es). This way we do not allow malicious clients to be submitted against correct chains. By creating the restriction on the destination chain rather than the source chain, we can enable all the flexibility of dynamic light clients while still preventing malicious connections over correctly implemented chains.

[mkaczanowski]

@AdityaSripal, thanks a lot for the feedback!

From my understanding, this information would be in data, but you would need to retrieve the smart contract code from WASM store to parse it and return the value.

Once the Wasm light client binary (aka. data blob or data []byte) is uploaded, the CosmWasm library gives us the codeId we use to reference the code.
So, whenever we need to execute a call, for instance CheckHeaderAndUpdateState, we would have to specify the codeId, say:

// abstract syntax
WasmVM.Execute(codeID, "CheckHeaderAndUpdateState", args...)

And pass all required arguments to the Wasm method itself. Right now, the codeId is only present within the ClientState structure.

If you can't retrieve code, then you will have to store directly in the client state so that go handlers can implement the interfaces. Is that correct?

Yes. If the code can't be retrieved (i.e. client code without VM access, no codeId available). Then we would store directly in the relevant structure like ClientState to satisfy the interface. To parse the data []byte, we need to execute Wasm VM light client method that would "understand the blob of data" and return the Go understandable struct (ie. Height, ConsesnsusState, ClientState, Status etc).

If not, please clarify why WASM client needs storage to implement these interfaces.

As I am reading my comments, I noticed the "storage" term is a bit unclear. Or at least I made it somewhat confusing, sorry. I referred to "needs storage" as "needs access to Wasm code identified by codeId".

So for example, ClientState.ZeroCustomFields:

1. must have access to both the Wasm VM and codeId (present in ClientState struct) to execute light client native method
2. doesn't require KV-store to be present as an argument because the methods consume ClientState/self and return updated ClientState. The Wasm light client code method is idempotent in this case.

Hopefully, that clarifies it a bit.

[mkaczanowski]

@cwgoes @AdityaSripal could you guys take another round on the comments + changes I've made? Thanks!

[AdityaSripal]

Right but I'm saying ValidateBasic should be doing some check on Height and because that is stateless, there should be a stateless access to Height here as well even if not strictly used directly in code. cc: @colin-axner what do you think?

[AdityaSripal]

Ok so what is needed beyond what is already available in the implementation. If anything?

[AdityaSripal]

Is the wasm client code stored in the clientID-prefixed store provided by IBC or it it stored in a KV store owned by WASM?

[mkaczanowski]

the Wasm client code is stored in the KV store owned by 28-wasm module.

Example entry:

ibc/code_id/d6537444f6df8686bdfefc3dc42441cd1d332ac608d27ef5903dd083a537e85f

Source: https://github.com/ChorusOne/ibc-go/blob/ics_28_wasm_client/modules/core/28-wasm/keeper/keeper.go#L61

[AdityaSripal]

The updates look good to me 👍

[cwgoes]

I'm a little confused, do we not want the WASM code to be able to write state itself? I don't see changes to that effect.

[mkaczanowski]

Thanks a lot for the comments! From what I can tell the last major thing to tackle in this ICS, is the question of whether Wasm code should be able to update its own client/consensus state, as pointed out by @cwgoes.

As @colin-axner explained that upcoming changes are to give light clients such flexibility, so I need to update this ICS.

I am not so fluent with pseudo-language, so here is what I came up with:
https://gist.github.com/mkaczanowski/06c43592a66453e13871d419870e66eb

@cwgoes @AdityaSripal can you please tell me if ^^ diff is alright?

[colin-axner]

The diffs align with what I had in mind for the future implementation changes 👍

[cwgoes]

Yes, the store should be passed to the WASM code.

[mkaczanowski]

@cwgoes @colin-axner @AdityaSripal I applied the remaining changes, plus renamed the ICS-28 to ICS-08 to match current ordering. Please let me know if you see anything else that requires changes, thanks!

[mkaczanowski]

@cwgoes @colin-axner @AdityaSripal friendly ping. It would be great if you could go through this ICS one more time :)

[cwgoes]

I'm basically fine with this, just checking - is the choice of having Go Merkle path validation (sub-state proofs) instead of letting WASM code do that intentional? It will constrain what WASM light clients can do, though it will probably be more efficient unless there are precompiles for hash functions or the like.

[mkaczanowski]

@cwgoes at first that's what we did (proof verification on the Go side), but as you said it's quite limiting.

is the choice of having Go Merkle path validation (sub-state proofs) instead of letting WASM code do that intentional

it's not intentional, and thanks for catching this.

Proposed changeset (please review)

Ideally, the verify methods are just a pass-through to WASM code (example VerifyConnectionState method in the initial impl)

So, here is the proposed changeset:
mkaczanowski@5750485

please let me know if ^^ this looks fine to you

[colin-axner]

So, here is the proposed changeset:
mkaczanowski@5750485

These changes make a lot more sense to me 👍 This means the wasm client interface needs to be expanded as well?

[mkaczanowski]

So, here is the proposed changeset:
mkaczanowski@5750485

These changes make a lot more sense to me +1 This means the wasm client interface needs to be expanded as well?

I am looking at this

[AdityaSripal]

Great work!! Small nits to address before this is ready to merge

[mkaczanowski]

Great work!! Small nits to address before this is ready to merge

thanks @AdityaSripal , applied the requested changes

[AdityaSripal]

Thanks @mkaczanowski , pending @cwgoes ack and then will merge

[cwgoes]

ACK from me, thanks for the spec work!

[AdityaSripal]

Note: We will remove GetProofSpecs from the interface as well.

Since ValidateSelfClient in the connection handshake must cast to specific client type(s) to perform all its checks, we can remove GetProofSpecs from the interface and check that each individual client type is doing the proofs correctly.

ref: cosmos/ics23#12 (comment)
cc: @colin-axner