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

Return an empty list of node hashes when the compact merkle tree size is a power of 2 #273

Merged
merged 2 commits into from
Dec 21, 2016

Conversation

rolandshoemaker
Copy link
Contributor

Dipping my toes into this code base with (what seems to me) a rather simple issue, feel free to tell me I've misunderstood the problem!

This change reuses the existing isPerfectTree method to check if the tree size is a power of two in CompactMerkleTree.Hashes and returns [][]byte{} if so. Also updates tests to reflect this new behavior.

Fixes #200.

@jsha
Copy link
Contributor

jsha commented Dec 16, 2016

Would nil be more idiomatic?

@AlCutter
Copy link
Member

Welcome :)
Looks like you could do with a rebase onto master to fix the Travis breakage…

@@ -229,6 +229,9 @@ func (c CompactMerkleTree) Size() int64 {

// Hashes returns a copy of the set of node hashes that comprise the compact representation of the tree.
func (c CompactMerkleTree) Hashes() [][]byte {
if isPerfectTree(c.size) {
return [][]byte{}
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to the suggestion of returning nil here instead.

Copy link
Contributor

@phad phad 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 fixing this. I'm happy for this to be merged as-is (with the minor return nil change).

-- background --

An alternative (but riskier) approach would be to have AddLeafHash clear the CompactMerkleTree.nodes slice when the leaf addition takes the tree to a perfect size. It would be a considerably more invasive change than the one I did in d1ee609 and I didn't do that at the time because I didn't want to risk introducing any kind of corruption into the compact merkle tree.

If AddLeafHash did work that way, it'd then be possible for checkUnusedNodesInvariant() to complain about CompactMerkleTree.nodes being non-empty on perfect tree sizes.

@phad phad merged commit 2c9402e into google:master Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants