ICS24: Restructure provable packet keys #1155
Conversation
| | -------------- | ------------------------------------------------- | ---------- | ------------------------------------ | | ||
| | provableStore | {channelIdentifier}|0x1|{bigEndianUint64Sequence} | bytes | [ICS 4](../ics-004-packet-semantics) | | ||
| | provableStore | {channelIdentifier}|0x2|{bigEndianUint64Sequence} | bytes | [ICS 4](../ics-004-packet-semantics) | | ||
| | provableStore | {channelIdentifier}|0x3|{bigEndianUint64Sequence} | bytes | [ICS 4](../ics-004-packet-semantics) | |
There was a problem hiding this comment.
I think you need a length prefix, or it would be vulnerable to ambiguous encoding.
Also, can't we extend the IBC client API with 3 methods with explicit parameters, so that it can verify the commitment in different ways? The IBC client API don't really need a general method to verify all possible blockchain commitments. Also, not all blockchains need to store commitments in a flattened Merkle store. They may be stored in more efficient or more secure ways, e.g. nested Merkle trees or ZK storage proofs.
For backward compatibility, we can just wrap the legacy IBC clients, and use the encoding scheme verified here to verify the IBC commitments.
There was a problem hiding this comment.
Hmm this is a good point though I don't think there actually is an ambiguous encoding. Since we are creating the key out of 3 components. The channelId, the byte identifying which value type we're storing, and the big endian sequence.
The last two components have a standardized length. So it is only the channel ID that has a variable length.
Thus, i don't think it is possible to construct two channelID, value type, sequence tuples that would yield the same key. Please correct me if I'm mistaken
There was a problem hiding this comment.
Oh right, it is not ambiguous because u64 is fixed size. Though I think it is tricky to reason about ambiguity depending on whether the subsequent fields are fixed size. It would also become ambiguous in case if we generalize sequence in the future to allow variable-length nonce bytes.
There was a problem hiding this comment.
Indeed, the one concern I had was that it could be easy to mess up in the future
These keys are now less verbose and also keys being proved on the same channel will be closer together in the tree since we key first on channelID. This will improve efficiency when we implement batching