-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement SSZ Tree Hash Algorithm #1211
Conversation
Travis CI failed due to change irrelevant to this PR. |
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.
all looks good, just 2 minor comments
shared/ssz/README.md
Outdated
@@ -23,6 +23,14 @@ type Decodable interface { | |||
} | |||
``` | |||
|
|||
### Hashable | |||
A type is Hashable if it implements `HashSSZ()`. |
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.
Can we rename HashSSZ
to TreeHashSSZ
to be more explicit? since there's still a more general hashing function that could be used
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.
Sounds good!
shared/ssz/hash.go
Outdated
HashSSZ() ([32]byte, error) | ||
} | ||
|
||
// Hash calculate tree-hash result for input value. |
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.
s/calculate/calculates and I think TreeHash is a better name here
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.
Sure. Thanks
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 for the comments!
shared/ssz/README.md
Outdated
@@ -23,6 +23,14 @@ type Decodable interface { | |||
} | |||
``` | |||
|
|||
### Hashable | |||
A type is Hashable if it implements `HashSSZ()`. |
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.
Sounds good!
shared/ssz/hash.go
Outdated
HashSSZ() ([32]byte, error) | ||
} | ||
|
||
// Hash calculate tree-hash result for input value. |
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.
Sure. Thanks
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.
lgtm, let's get 1 more approval before merge
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.
Great tests, we can include these as part of the yaml suite later on
return hasher, nil | ||
} | ||
|
||
func merkleHash(list [][]byte) ([]byte, error) { |
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.
Hey @houjieth can we have some more detailed comments of the internal logic at the top of this function for easier readability?
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.
Sure. Just added some comments.
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.
can you also add a link of the official spec?
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.
Yes. Just added the link.
Resolves #716
Description
Implement tree-hash defined by ssz.