Integrate Merkle Tree implementation from Plonky3 - #140
Merged
Conversation
|
Coverage (base → head): |
frozenspider
force-pushed
the
feature/p3-merkle-tree
branch
from
August 23, 2025 12:32
46e6e84 to
3df3514
Compare
frozenspider
marked this pull request as ready for review
August 25, 2025 09:30
cupicmarko
reviewed
Aug 25, 2025
cupicmarko
left a comment
Contributor
There was a problem hiding this comment.
Looks good, I would just like to in depth through transcript, because there might be some mixing of endianness, so it's not going to be completely portable.
Contributor
Author
|
@cupicmarko Good catch! My |
NiDimi
approved these changes
Aug 26, 2025
NiDimi
left a comment
Contributor
There was a problem hiding this comment.
Looks good, but there is a small typo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworked Merkle Tree approach used in Zinc.
Instead of a makeshift implementation, rely on the one designed by https://github.com/Plonky3/Plonky3 - just like https://github.com/tcoratger/whir-p3 does.
This particular MT is designed to commit to matrices, each matrix row becoming an MT leaf that is opened together. This is very convenient for Zinc since we need to open entire columns. For that, we transpose a matrix before committing.
Most changes in this PR are trivial due to required implementation of
Copytrait forInteger. Main changes are:Plonky3's MT implementation to commit to entire u-hat matrixMtHashfor ease of changeverify_testingwhere the result ofverify_columnwas silently ignoredencode_rowsand matrix transposal is made usingMaybeUninitand a minimal amount of unsafe code, yielding ~10% speedup of acommitoperation.Benchmarked running time changes of
zip_benches, single threaded:Zip/EncodeRows: (about the same)Zip/MerkleRoot: +25% (since we're no longer optimizing for committing to individual leaves)Zip/Commit: -58%Zip/Open: -50%Zip/Verify: -55%Multi-threaded +
asm:Zip/EncodeRows: (about the same)Zip/MerkleRoot: (about the same)Zip/Commit: -95%Zip/Open: -40%Zip/Verify: (fails both before and after, see Zip/Verify benchmark doesn't work withparallelfeature flag on #141)