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

[srdtrk]

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]

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]

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

[srdtrk]

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 cosmos/ibc-go#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]

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

[Farhad-Shabani]

I have given this some thought and contemplated defining a type for the path argument in the verify_(non_)membership API that is (1) more generic, uses byte representation, and even merging it with the prefix into a single argument such as merkle_path: MerklePath. However, after examining the situation, I concluded this approach has serious drawbacks:

1. It leaves light client developers without the option to benefit from the concrete domain type of the Path. This type might be used to match and craft the final path bytes needed for verification purposes (e.g., see how the path is used for implementing verify_(non_)_membership in the Sovereign light client).
2. The ibc-rs core module will have to create the MerklePath or a byte representation to pass to verify_(non_)_membership (For example places like here). This means we have to assume how the path is (1) created and (2) serialized, which can vary across different hosts. As an example, sovereign-ibc use borsh serialization. Therefore, this breaks the core modules' agnostic approach to paths.
3. Exposes breaking changes to users.

Another approach is to simply (2) add a Custom variant to the Path, such as Path::Custom(String), which accepts any passed string as a Path. This makes Path::from_str() infallible, and keeps the passed path for verify_membership rich and meaningful while also allowing to work with custom paths.

I am suggesting to use Custom(String) instead of Custom(Vec<u8>), as I believe switching to bytes should be examined as a separate issue. Currently, all path types in ibc-rs are defined using String. Therefore, a separate work needed to consistently update all path types to use byte representation if that's gonna be the decision.

[srdtrk]

I tried testing #1273 and #1271. I found it quite hard to migrate my code to use PathBytes. Specifically because I couldn't find any constructor for PathBytes and I don't know how to convert between PathBytes, MerklePath, and Path.

Also, assume that I have two PathBytes. And I want to merge them. How would I do this? Overall, I think more freedom with this type would be great. I'll probably create a separate issue about this but would love to hear your thoughts.

[Farhad-Shabani]

Hey @srdtrk, much appreciate the feedback. Didn't expect. Otherwise I would've definitely asked for your input before the merge.

• For the constructor of Vec<u8> wrapper types like PathBytes, we generally utilize From. Didn’t PathBytes::from(...) work for you? Do you need to obtain PathBytes from a specific type?
• Regarding the merge, there's a flatten method designed for this use. Did that not help?
• For converting Path --> PathBytes, the serialize_path() is defined for this purpose, which is part of the Tendermint client implementation. Logically, where you intend to perform verify_(non_)membership, you should have the ClientState object, and be able to call that as well. If that’s not the case, could you give us more details?
• For converting PathBytes --> MerklePath, since PathBytes is a serialized path, you only need to pass the list of paths to the MerklePath's constructor. There is just a point here, if your system uses commitmentPrefix, that should be converted to PathBytes and prepended. Otherwise, the straight path bytes should work. (I do see the documentation for this should be improved.)

Any specific details (if possible with pointers) would be super helpful. If the info above didn’t quite do the trick, let me know. Happy to tweak things!

[Farhad-Shabani]

fyi @srdtrk, a few helper methods will be introduced by PR #1283. Hope that makes your life easier.