search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
179 stars 73 forks source link

07-tendermint's verify membership doesn't allow custom paths #1255

Open srdtrk opened 2 weeks ago

srdtrk commented 2 weeks ago

Bug Summary

Whether or not this is a bug needs discussion.

While using the 07-tendermint client from a cosmwasm context to verify membership. I noticed that unless the path is one of the paths defined in here, verify membership fails due to this. This makes implementing features such as icq difficult.

rnbguy commented 2 weeks ago

Definitely need some discussion. For now, we just follow ibc spec and ibc-go paths.

Do you think, it makes sense to make it a parameter to client state or part of the concrete light client implementation - possibly keeping it consistent using a Rust trait?

rnbguy commented 2 weeks ago

I am also curious of an example - where this is really necessary. Is this something needed in rollkit-ibc?

srdtrk commented 2 weeks ago

ICS-24 is meant to highlight what state the IBC protocol needs to store. And which of these states need to be provable. It is not meant to be an exhaustive list of all paths that 07-tendermint can be used to prove. In practice, users prove non-ibc state with 07-tendermint such as user balance, gov proposals and etc. (I give some concrete examples below)

Moreover, I'm not sure if any of the stated suggestions solve the issue completely. IBC-Go will be switching to using bytes instead of strings as path since some provable stores in CosmosSDK already cannot be parsed into valid strings, see https://github.com/cosmos/ibc-go/issues/6496.

I initially wanted this feature because I'm working on a CosmWasm POC implementation of ibc-lite and I was playing around with changing some paths. I can easily work around this though.

Otherwise, this is necessary for key-value interchain queries (spec, implementation). There are other chains that use kv-icq, for example neutron.

rnbguy commented 2 weeks ago

Thanks for contexts. Makes sense. This helps a lot. We would be happy to continue the discussion to support this properly.