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: Spec implementation #11720

Merged
merged 44 commits into from
Jan 27, 2023
Merged

Conversation

saolyn
Copy link
Contributor

@saolyn saolyn commented Dec 3, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Rewrite of the Go implementation for an optimized deposit tree as specified in eip-4881.

Which issues(s) does this PR fix?

Part of #11256

Other notes for review

}, nil
}

func (d *DepositTree) finalize(eth1data eth.Eth1Data, executionBlockHeight uint64) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prestonvanloon which Eth1Data should I be using here?

Copy link
Member

Choose a reason for hiding this comment

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

That one seems correct. What other eth1data is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used eth "github.com/prysmaticlabs/prysm/v3/proto/eth/v1" since it made more sense to me but there is also eth "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1" so wanted to double check.

Copy link
Member

Choose a reason for hiding this comment

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

Ah -- Those are essentially the same thing. We had to duplicate it for some compatibility reasons with ssz-gen tooling

@saolyn saolyn mentioned this pull request Dec 12, 2022
@saolyn saolyn marked this pull request as ready for review December 19, 2022 13:24
@saolyn saolyn requested a review from a team as a code owner December 19, 2022 13:24
depositCount: leftSubtree,
hash: finalized[0],
}
node.right = fromSnapshotParts(finalized[1:], deposits-leftSubtree, level-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

check if you can do deposits-leftSubtree

}

// Finalize marks deposits of the Merkle tree as finalized.
func (n *InnerNode) Finalize(depositsToFinalize uint64, depth uint64) MerkleTreeNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Checks on depth here to prevent underflow

// PushLeaf adds a new leaf node at the next available zero node.
func (n *InnerNode) PushLeaf(leaf [32]byte, depth uint64) (MerkleTreeNode, error) {
if !n.left.IsFull() {
left, err := n.left.PushLeaf(leaf, depth-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

checks on depth here


// GetRoot returns the root of the Merkle tree.
func (z *ZeroNode) GetRoot() [32]byte {
if z.depth == DepositContractDepth {
Copy link
Contributor

Choose a reason for hiding this comment

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

checks on depth

//nolint:unused
func (d *DepositTree) pushLeaf(leaf [32]byte) error {
var err error
d.mixInLength++
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add after we push? or do so inside of push leaf? worried that if it fails, we would have still modified the tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like we need it for PushLeaf so I do not see a problem moving it below the error handling. We will most likely need to test it to be sure.

@saolyn saolyn force-pushed the eip4881-spec-implementation branch from 769055e to 6e2cb33 Compare January 19, 2023 23:17
@prylabs-bulldozer prylabs-bulldozer bot merged commit 9529c73 into develop Jan 27, 2023
@delete-merged-branch delete-merged-branch bot deleted the eip4881-spec-implementation branch January 27, 2023 17:35
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