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

Reduce zero-padding and cache zero-hashes in MerkleTree #2415

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Apr 1, 2024

This PR is the "plain" Merkle tree counterpart to https://github.com/AleoHQ/snarkVM/pull/2407.

While some of the console/collections/merkle_tree benchmark results are mixed, any regressions are small; however, I also ran a custom benchmark locally, and that one shows a clear improvement in extreme scenarios:

creation of a BHP MerkleTree with DEPTH = 23 and 2^22 + 1 random leaves

before:

alloc count: 38,230,906
heap use:    1.59 GiB
max heap:    2.10 GiB
time:        51.88s

after:

alloc count: 31,932,664 (-16.5%)
heap use:    1.47 GiB (-7.5%)
max heap:    2.00 GiB (-4.8%)
time:        42.61s (-17.9%)

Signed-off-by: ljedrz <ljedrz@gmail.com>
@howardwu
Copy link
Contributor

howardwu commented Apr 3, 2024

@vicsn @mdelle1 can each of you independently review this design change for correctness/completeness and soundness?

@ljedrz
Copy link
Collaborator Author

ljedrz commented Apr 4, 2024

Note: I recommend that we finish https://github.com/AleoHQ/snarkVM/pull/2407, as some of the review comments there are applicable to this PR too.

@ljedrz ljedrz marked this pull request as draft April 5, 2024 09:53
@ljedrz
Copy link
Collaborator Author

ljedrz commented Apr 5, 2024

Converting to a draft until https://github.com/AleoHQ/snarkVM/pull/2407 is reviewed, as they have many common features, and there are already review comments in that one that I've locally applied to this PR.

Signed-off-by: ljedrz <ljedrz@gmail.com>
@ljedrz ljedrz marked this pull request as ready for review April 5, 2024 13:08
Copy link
Contributor

@vicsn vicsn left a comment

Choose a reason for hiding this comment

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

Apart from one nit (adding the ensure), LGTM. This comment applies here too.

I was also told equivalence tests have been run on the binary merkle tree just like they were for the k-ary.

@ljedrz
Copy link
Collaborator Author

ljedrz commented Apr 8, 2024

@vicsn PR updated; let me know if you agree with the second change, as it would also apply to the k-ary Merkle tree PR 😁. Also, the equivalence tests still pass.

@ljedrz ljedrz requested a review from vicsn April 8, 2024 15:12
@howardwu howardwu merged commit 933f12f into AleoNet:mainnet-staging Apr 12, 2024
80 checks passed
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.

4 participants