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

Simple mixed merkle tree #525

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Mar 22, 2024

This change is Reviewable

@spapinistarkware spapinistarkware mentioned this pull request Mar 22, 2024
Copy link
Contributor Author

spapinistarkware commented Mar 22, 2024

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 97.89474% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 95.47%. Comparing base (da3de7d) to head (5db48f0).

Files Patch % Lines
src/commitment_scheme/verifier.rs 94.89% 1 Missing and 4 partials ⚠️
src/commitment_scheme/prover.rs 97.36% 1 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                     @@
##           spapini/03-20-avx_blake     #525      +/-   ##
===========================================================
+ Coverage                    95.40%   95.47%   +0.07%     
===========================================================
  Files                           65       68       +3     
  Lines                         9678     9963     +285     
  Branches                      9678     9963     +285     
===========================================================
+ Hits                          9233     9512     +279     
- Misses                         387      389       +2     
- Partials                        58       62       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)


src/commitment_scheme/prover.rs line 10 at r2 (raw file):

pub struct MerkleProver<B: MerkleOps<H>, H: MerkleHasher> {
    pub layers: Vec<Col<B, H::Hash>>,

Document (also the order)

Code quote:

    pub layers: Vec<Col<B, H::Hash>>,

src/commitment_scheme/prover.rs line 13 at r2 (raw file):

}
impl<B: MerkleOps<H>, H: MerkleHasher> MerkleProver<B, H> {
    /// Commits to columns.

Document: Each column leaves is in the height of the length of the column

Code quote:

/// Commits to columns.

src/commitment_scheme/prover.rs line 51 at r2 (raw file):

                if layer.len() > 1 {
                    witness.push(layer.at(query ^ 1));
                }

Remove and replace with
for layer in &self.layers[:-1]

Code quote:

                if layer.len() > 1 {
                    witness.push(layer.at(query ^ 1));
                }

src/commitment_scheme/verifier.rs line 16 at r2 (raw file):

impl<H: MerkleHasher> MerkleTreeVerifier<H> {
    /// Verifies the decommitment of the columns.
    /// Queries are given as indices to the largest column.

We are assuming that this is enough

Code quote:

    /// Queries are given as indices to the largest column.

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from b8b4d6b to 9f67f18 Compare March 24, 2024 15:07
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from 7b36b0e to 74d0ad5 Compare March 24, 2024 15:07
@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from 9f67f18 to 884140b Compare March 25, 2024 05:19
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from 74d0ad5 to c1ab684 Compare March 25, 2024 05:20
Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


src/commitment_scheme/prover.rs line 10 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Document (also the order)

Done.


src/commitment_scheme/prover.rs line 13 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Document: Each column leaves is in the height of the length of the column

Done.


src/commitment_scheme/prover.rs line 51 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Remove and replace with
for layer in &self.layers[:-1]

Done.


src/commitment_scheme/verifier.rs line 16 at r2 (raw file):

Previously, shaharsamocha7 wrote…

We are assuming that this is enough

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from c1ab684 to 027ceda Compare March 25, 2024 06:02
@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from 884140b to f1c6f1c Compare March 25, 2024 09:21
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from 027ceda to d3176d8 Compare March 25, 2024 09:21
@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from f1c6f1c to ec5d9d8 Compare March 25, 2024 09:54
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from d3176d8 to 16a10c7 Compare March 25, 2024 09:54
Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)


src/commitment_scheme/ops.rs line 9 at r4 (raw file):

    /// The node may or may not need to hash 2 hashes from the previous layer - depending if it is a
    /// leaf or not.
    /// In addition, the node may have extra column values that need to be hashed.

How did ohad describe that as well?

Code quote:

    /// Hashes a single Merkle node.
    /// The node may or may not need to hash 2 hashes from the previous layer - depending if it is a
    /// leaf or not.
    /// In addition, the node may have extra column values that need to be hashed.

src/commitment_scheme/prover.rs line 14 at r3 (raw file):

    /// The last layer is the root.
    /// Layer n holds the 2^n hashes of the previous layer if exists, and all the columns of size
    /// 2^n.

How did Ohad described that?

Code quote:

    /// Layer n holds the 2^n hashes of the previous layer if exists, and all the columns of size
    /// 2^n.

src/commitment_scheme/verifier.rs line 21 at r3 (raw file):

    ///
    /// * `queries` - A vector of indices representing the queries to the largest column. This is
    ///   assumed to be sufficient for our use case, though it could be extended to support queries

for bit reversed commitments

Code quote:

for our use case

src/commitment_scheme/verifier.rs line 79 at r4 (raw file):

}

struct MerkleVerifier<H: MerkleHasher> {

Document

Code quote:

struct MerkleVerifier<H: MerkleHasher> {

src/commitment_scheme/verifier.rs line 82 at r4 (raw file):

    witness: std::vec::IntoIter<<H as MerkleHasher>::Hash>,
    column_values: Peekable<std::vec::IntoIter<(u32, Vec<BaseField>)>>,
    layer_column_values: Vec<std::vec::IntoIter<BaseField>>,

document all

Code quote:

    witness: std::vec::IntoIter<<H as MerkleHasher>::Hash>,
    column_values: Peekable<std::vec::IntoIter<(u32, Vec<BaseField>)>>,
    layer_column_values: Vec<std::vec::IntoIter<BaseField>>,

src/commitment_scheme/verifier.rs line 162 at r4 (raw file):

        children_hashes: ChildrenHashesAtQuery<H>,
    ) -> Result<H::Hash, MerkleVerificationError> {
        let hashes_part = children_hashes

Read witness from decommitment if not already known.

Code quote:

let hashes_part = children_hashes

@spapinistarkware spapinistarkware force-pushed the spapini/03-20-avx_blake branch from ec5d9d8 to da3de7d Compare March 25, 2024 11:46
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from 16a10c7 to 5db48f0 Compare March 25, 2024 12:00
Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


src/commitment_scheme/ops.rs line 9 at r4 (raw file):

Previously, shaharsamocha7 wrote…

How did ohad describe that as well?

Done.


src/commitment_scheme/prover.rs line 14 at r3 (raw file):

Previously, shaharsamocha7 wrote…

How did Ohad described that?

Done.


src/commitment_scheme/verifier.rs line 79 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Document

Done.


src/commitment_scheme/verifier.rs line 82 at r4 (raw file):

Previously, shaharsamocha7 wrote…

document all

Done.


src/commitment_scheme/verifier.rs line 162 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Read witness from decommitment if not already known.

Done.

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)


src/commitment_scheme/ops.rs line 8 at r5 (raw file):

/// "[]" denotes optional values.
/// The largest Merkle layer has no left and right child hashes. The rest of the layers have
/// children hases

Suggestion:

/// The largest Merkle layer has no left and right child hashes. The rest of the layers have
/// children hashes

src/commitment_scheme/verifier.rs line 79 at r5 (raw file):

}

/// A helper struct for verifying a [Decommitment].

Rename to MerkleDecommitment

Suggestion:

/// A helper struct for verifying a [MerkleDecommitment].

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_mixed_merkle_tree branch from 5db48f0 to 43bb6bf Compare March 25, 2024 14:40
@spapinistarkware spapinistarkware changed the base branch from spapini/03-20-avx_blake to dev March 25, 2024 14:40
Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


src/commitment_scheme/ops.rs line 8 at r5 (raw file):

/// "[]" denotes optional values.
/// The largest Merkle layer has no left and right child hashes. The rest of the layers have
/// children hases

Done.

Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


src/commitment_scheme/verifier.rs line 79 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Rename to MerkleDecommitment

Done.

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

@spapinistarkware spapinistarkware merged commit 1ebc2be into dev Mar 26, 2024
11 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.

3 participants