search

cosmos / ics23

Building generic merkle proof format for IBC
Apache License 2.0
110 stars 66 forks source link

`commitment_proof` for non-Cosmos chain #80

Open livelybug opened 2 years ago

livelybug commented 2 years ago

Hello,

We are working on the Substrate chain support of ibc-rs, which utilizes https://github.com/confio/ics23/tree/master/rust.

The CommitmentProof has 4 variants Exist, Nonexist, Batch, Compressed, none of them matches the structure of the corresponding proof in Substrate.

We use a temporary work-around compose_ibc_merkle_proof to insert the Substrate's proof into the CommitmentProof currently.

Is it possible to create a new variant in CommitmentProof for Substrate?

Thank you

notbdu commented 2 years ago

Ethan talks about the issues generalizing parity/trie (also an issue with eth trie) to the ics23 proofing format. https://github.com/confio/ics23/issues/65#issuecomment-1031851018

OTOH, if we continue to use ics23 within 02-client I don't think it makes sense to add a separate variant just for parity/trie. It's still a merkle tree, albeit a hyper optimized one that doesn't generalize well.

Perhaps it would be possible to generate deterministic commitment roots via an extended proof spec?

ethanfrey commented 2 years ago

I don't think it makes sense to add a separate variant just for parity/trie

This is the point, ics23 was supposed to be generic enough that any Merkle trie/tree implemented could create such proofs. I didn't count on chains making other decisions to prevent this confirmation (I was thinking the chain designers wanted to be on Cosmos... not strapping this onto someone who wants to bind all chains to their one hub).

We definitely do not want to add one new variant for each trie/tree we encounter.

I think we may need (and be ready for) wasm code to do ics23 verification so we can easily add new formats.

@crodriguezvega @AdityaSripal I would love to hear your input here... some changes are needed to support substrate... we cannot make them compatible with the current ics23 protobuf format unless we add a custom variant just for them (and then wait for all counterparty chains to update to that)