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

Remove leaf hashing in MerkleRoot function #1653

Merged
merged 4 commits into from
Feb 20, 2019

Conversation

wemeetagain
Copy link
Contributor

Description

Previously, the MerkleRoot function hashed the leaves provided as input.
This is in line with merkle hashing, generally speaking, however the spec's definition of the merkle_root function makes an optimization by assuming that the leaves are pre-hashed, and therefore not hashing the leaves.

This update removes the hashing of the leaf nodes from the MerkleRoot function and updates the MerkleRoot test.

See ethereum/consensus-specs#646 for clarification.

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

I would rather than not remove leaf hashing as we would still have to do it outside the function to generate the merkle root. Ex:

	
	for i, v := range values {	
		hashedValue := Hash(v)	
		values[i] = hashedValue[:]	
	}
       
        root := MerkleRoot(values)

I rather do the hashing in the function than outside the function and have to add boilerplate code whenever we have to calculate the merkle root.

@wemeetagain
Copy link
Contributor Author

@nisdas

The thing is, these leaves are not just hashed directly and not hashed immediately before being put into MerkleRoot. They are currently stored, pre-hashed (I believe one by one over time), in the BeaconState.

To get specific, the MerkleRoot function is currently used only once, here, to hash state.LatestBlockRootHash32S.

state.LatestBlockRootHash32S are already hashed (they are block root hashes), and you would presumably need to gather the actual underlying blocks first if you wanted to keep the leaf hashing logic inside MerkleRoot.

The issue is less about whether or not to keep a MerkleRoot function that hashes the leaves, and more about this specific instance, where you most likely will need a function that returns a merkle root, but does not hash the leaves.

@nisdas
Copy link
Member

nisdas commented Feb 20, 2019

@wemeetagain In that case it makes sense :) . Could you add a comment stating why we are not hashing the leaves in the function and are assuming that the leaves are pre-hashed

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

LGTM , thanks for this !

@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #1653 into master will decrease coverage by 0.64%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1653      +/-   ##
==========================================
- Coverage   71.49%   70.85%   -0.65%     
==========================================
  Files          98       97       -1     
  Lines        7010     6687     -323     
==========================================
- Hits         5012     4738     -274     
+ Misses       1553     1523      -30     
+ Partials      445      426      -19

@nisdas nisdas merged commit e3ba3e1 into prysmaticlabs:master Feb 20, 2019
@wemeetagain wemeetagain deleted the fix-merkle-root branch February 20, 2019 18:30
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.

None yet

3 participants