Skip to content
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

Merged
merged 12 commits into from
Jan 2, 2019

Conversation

houjieth
Copy link
Contributor

Resolves #716


Description

Implement tree-hash defined by ssz.

@houjieth
Copy link
Contributor Author

Travis CI failed due to change irrelevant to this PR.

Copy link
Member

@terencechain terencechain left a 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

@@ -23,6 +23,14 @@ type Decodable interface {
}
```

### Hashable
A type is Hashable if it implements `HashSSZ()`.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

HashSSZ() ([32]byte, error)
}

// Hash calculate tree-hash result for input value.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Thanks

Copy link
Contributor Author

@houjieth houjieth left a 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!

@@ -23,6 +23,14 @@ type Decodable interface {
}
```

### Hashable
A type is Hashable if it implements `HashSSZ()`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

HashSSZ() ([32]byte, error)
}

// Hash calculate tree-hash result for input value.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Thanks

Copy link
Member

@terencechain terencechain left a 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

Copy link
Contributor

@rauljordan rauljordan left a 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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@terencechain terencechain merged commit e5d92a5 into prysmaticlabs:master Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants