-
Notifications
You must be signed in to change notification settings - Fork 410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ICS24: Restructure provable packet keys #1155
Merged
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
41a8caa
imp: note that commitments must be lexographically ordered to maintai…
colin-axner 6e51b71
restructure ibc keys
AdityaSripal a91410d
add value instead of redundant provable store column
AdityaSripal 499818e
chore: remove myself as codeowner (#1156)
colin-axner 47fea20
Fix variable name (#1162)
chipshort 526fe11
Merge branch 'main' of github.com:cosmos/ibc into aditya/provable-keys
AdityaSripal ffb4ad3
Merge branch 'feat/v2-spec' of github.com:cosmos/ibc into aditya/prov…
AdityaSripal 1144d76
fix link and put new provable keys into 04-channel spec
AdityaSripal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
restructure ibc keys
- Loading branch information
commit 6e51b71f83b3e60c119bd760f61bc232c5fc478a
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 mistakenThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the one concern I had was that it could be easy to mess up in the future