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

EIP-4881: Merkle tree implementation #11605

Closed
wants to merge 21 commits into from

Conversation

saolyn
Copy link
Contributor

@saolyn saolyn commented Nov 2, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?
Part 2 of the implementation of an optimized deposit tree as specified in eip-4881.
This PR contains the implementation of the MerkleTreeNode from #11597.

Each node type was separated into it's own file for clarity.

  • Inner Node: represents inner nodes with two children.
  • Leaf Node: represents a leaf node holding a deposit.
  • Finalized Node: represents a node that has been finalized.
  • Zero Node: represents an empty node without a deposit.

Additionally this PR adds both the recursive and iterative algorithm for creating the Merkle tree from a given list of deposits.

Which issues(s) does this PR fix?

Part of #11256

Other notes for review
All uint types were changed to uint64 and the MerkleTreeNode interface was modified to include errors.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Left review on the structs. I haven't reviewed the merkle tree snapshot construction.

beacon-chain/cache/depositsnapshot/finalized_node.go Outdated Show resolved Hide resolved
beacon-chain/cache/depositsnapshot/finalized_node.go Outdated Show resolved Hide resolved
beacon-chain/cache/depositsnapshot/inner_node.go Outdated Show resolved Hide resolved
beacon-chain/cache/depositsnapshot/inner_node.go Outdated Show resolved Hide resolved

// GetFinalized returns a list of hashes of all the finalized nodes and the number of deposits.
func (l *Leaf) GetFinalized(result [][32]byte) ([][32]byte, uint64) {
return result, 0
Copy link
Member

Choose a reason for hiding this comment

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

This isn't implemented right? Consider a TODO or panic("not implemented")

Copy link
Contributor Author

@saolyn saolyn Nov 4, 2022

Choose a reason for hiding this comment

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

I cross referenced the EIP spec and it appears to be correct.

class Leaf(MerkleTree):
        ...
        def get_finalized(self, result: List[Hash32]) -> uint:
                return 0

Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why the API for our implementation is different than the spec? Their spec only returns the deposit count

Copy link
Contributor Author

@saolyn saolyn Nov 4, 2022

Choose a reason for hiding this comment

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

In the python implementation the get_finalized from the class Finalized is appending a hash to the list of hashes result via pointers.
I decided to return the result instead of using pointers as I was worried it could be unclear and error prone. Which means each instance of GetFinalized must also return the list.
If this is an issue, I can modify it to use pointers instead.

Comment on lines 21 to 24
// IsFull returns whether there is space left for deposits.
func (z *Zero) IsFull() bool {
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback as leaf node. Add commentary why this always returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, additional commentary added, if you could please check that it is clear.


// GetFinalized returns a list of hashes of all the finalized nodes and the number of deposits.
func (z *Zero) GetFinalized(result [][32]byte) ([][32]byte, uint64) {
return result, 0
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback here too. Is this implemented correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing as the GetFinalized for Leaf, I cross referenced the EIP spec and it appears to be correct.

class Zero(MerkleTree):
        ...
        def get_finalized(self, result: List[Hash32]) -> uint:
                return 0

beacon-chain/cache/depositsnapshot/zero_node.go Outdated Show resolved Hide resolved
Comment on lines 16 to 19
// IsFull returns whether there is space left for deposits.
func (f *Finalized) IsFull() bool {
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback to add commentary about why this is always full.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 26 to 29
// GetFinalized returns a list of hashes of all the finalized nodes and the number of deposits.
func (f *Finalized) GetFinalized(result [][32]byte) ([][32]byte, uint64) {
return append(result, f.hash), f.deposits
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. Same feedback about is this implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, it appears to be correct:

class Finalized(MerkleTree)
        ...
         def get_finalized(self, result: List[Hash32]) -> uint:
                result.append(self.hash)
                return self.deposit_count

@saolyn
Copy link
Contributor Author

saolyn commented Jan 26, 2023

Closing as this has been replaced by #11720.

@saolyn saolyn closed this Jan 26, 2023
@saolyn saolyn deleted the eip4881-interface-implementation branch August 28, 2024 18:27
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.

2 participants