-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix VerifyMembership proof #11
Conversation
Signed-off-by: Dongri Jin <dongri.jin@speee.jp>
Signed-off-by: Dongri Jin <dongri.jin@speee.jp>
Signed-off-by: Dongri Jin <dongri.jin@speee.jp>
heightBig.Or(revisionNumberBig, revisionHeightBig) | ||
hashHeight := sha256.Sum256(heightBig.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.
There are two mistakes.
- Look at the spec again.
height
is concatenated to the other stuffs before hashing. - The byte length of
heightBig.Bytes()
can be less than 16.- For example, if
revisionNumber == 0
, it will be 8. - You should use
FillBytes
instead ofBytes
.
- For example, if
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 can use encoding/binary
to do this easilier.
https://pkg.go.dev/encoding/binary#example-ByteOrder-Put
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.
Thanks
Change to use encoding/binary
28f2d67
Signed-off-by: Dongri Jin <dongri.jin@speee.jp>
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.
@dongrie Check the spec of Solidity's ABI encoding.
https://docs.soliditylang.org/en/latest/abi-spec.html
binary.LittleEndian.PutUint64(heightBuf[:8], revisionHeight) | ||
binary.LittleEndian.PutUint64(heightBuf[8:], revisionNumber) |
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.
Please use BigEndian
and put revisionNumber
at first and revisionHeight
at last.
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.
Fixed
331c660
Signed-off-by: Dongri Jin <dongri.jin@speee.jp>
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.
@dongrie LGTM
Fix verifyMembership Proof
https://github.com/hyperledger-labs/yui-ibc-solidity/blob/b3dc7b23bc849cb2bb0381d79016c97b93d5ccb5/tests/foundry/src/helpers/ModifiedMockClient.sol