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

MerkleVerifier #3

Merged
merged 1 commit into from
Jul 7, 2024
Merged

MerkleVerifier #3

merged 1 commit into from
Jul 7, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Jun 9, 2024

This change is Reviewable

Copy link
Contributor Author

spapinistarkware commented Jun 9, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spapinistarkware and the rest of your teammates on Graphite Graphite

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from c51fb12 to 3552bc8 Compare June 10, 2024 09:59
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 7 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)


stwo_cairo_verifier/src/vcs/hasher.cairo line 5 at r1 (raw file):

use core::poseidon::poseidon_hash_span;
use stwo_cairo_verifier::BaseField;

This file was just copied + I added another test at the end

@spapinistarkware spapinistarkware mentioned this pull request Jun 16, 2024
Merged
Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware 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 7 files at r1, all commit messages.
Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


stwo_cairo_verifier/src/vcs/verifier.cairo line 41 at r1 (raw file):

        queries_per_log_size: Felt252Dict<Nullable<Span<usize>>>,
        queried_values: Array<Span<BaseField>>,
        decommitment: MerkleDecommitment<H>,

docunent

Code quote:

        self: @MerkleVerifier<H>,
        queries_per_log_size: Felt252Dict<Nullable<Span<usize>>>,
        queried_values: Array<Span<BaseField>>,
        decommitment: MerkleDecommitment<H>,

@spapinistarkware spapinistarkware mentioned this pull request Jun 16, 2024
Copy link
Contributor

@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 5 of 7 files at r1, all commit messages.
Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @spapinistarkware)


stwo_cairo_verifier/src/vcs/hasher.cairo line 56 at r1 (raw file):

            word = word * M31_IN_HASH_SHIFT + column_values.pop_front().unwrap().inner.into();
            word = word * M31_IN_HASH_SHIFT + column_values.pop_front().unwrap().inner.into();
            hash_array.append(word);

You added this line. Is that on purpose?

Code quote:

hash_array.append(word);

stwo_cairo_verifier/src/vcs/hasher.cairo line 72 at r1 (raw file):

        assert_eq!(
            PoseidonMerkleHasher::hash_node(Option::None, array![m31(0), m31(1)]),
            2552053700073128806553921687214114320458351061521275103654266875084493044716

Why the result was changed?

Code quote:

            PoseidonMerkleHasher::hash_node(Option::None, array![m31(0), m31(1)]),
            2552053700073128806553921687214114320458351061521275103654266875084493044716

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/utils.cairo line 11 at r1 (raw file):

#[generate_trait]
pub impl DictImpl<T, +Felt252DictValue<T>, +PanicDestruct<T>> of DictTrait<T> {
    fn replace(ref self: Felt252Dict<T>, key: felt252, def: T) -> T {

why def?

Code quote:

 def: T

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from 3552bc8 to ffa3b7b Compare June 18, 2024 07:17
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: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


stwo_cairo_verifier/src/utils.cairo line 11 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

why def?

Done.


stwo_cairo_verifier/src/vcs/hasher.cairo line 56 at r1 (raw file):

Previously, shaharsamocha7 wrote…

You added this line. Is that on purpose?

Yes. It was a bug before. I'm packing the words, then adding to hash_array, then hashing hash_array.


stwo_cairo_verifier/src/vcs/hasher.cairo line 72 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Why the result was changed?

Because it had a bug before.


stwo_cairo_verifier/src/vcs/verifier.cairo line 41 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

docunent

Done.

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 97 at r2 (raw file):

            let mut layer_cols = cols_by_size
                .replace(layer_log_size.into(), Default::default())
                .deref_or(array![]);

What is this?
is this the same as unwrap_or?

Code quote:

 .deref_or

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 102 at r2 (raw file):

            let mut layer_queried_values = array![];

            while !layer_cols.is_empty() {

Can you use while let?

Code quote:

 while !layer_cols.is_empty()

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 245 at r2 (raw file):

fn fetch_prev_node_hash<H, +Copy<H>, +Drop<H>>(
    ref prev_layer_hashes: Array<(u32, H)>, ref hash_witness: Array<H>, expected_query: u32
) -> Option<H> {

document,

  • In what sense is this a 'prev' node hash?

Code quote:

fn fetch_prev_node_hash<H, +Copy<H>, +Drop<H>>(
    ref prev_layer_hashes: Array<(u32, H)>, ref hash_witness: Array<H>, expected_query: u32
) -> Option<H> {

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 127 at r2 (raw file):

                    Option::None
                } else {
                    // If the left child was not computed, read it from the witness.

Isn't this done inside fetch_prev_node_hash? why is this comment there?

Code quote:

   // If the left child was not computed, read it from the witness.

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 154 at r2 (raw file):

                    let mut i = 0;
                    while i != n_columns_in_layer {
                        let col_queried = layer_queried_values[i];

?

Suggestion:

queried_column 

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 240 at r2 (raw file):

            Option::Some(column_query), Option::Some(prev_query)
        ) => { Option::Some(min(*column_query, prev_query)) },
    }

Don't you query everything in the same place with OODS why do we have this complicated logic?

Code quote:

    match (layer_query_head, prev_query_head) {
        (Option::None, Option::None) => { Option::None },
        (Option::Some(column_query), Option::None) => { Option::Some(*column_query) },
        (Option::None, Option::Some(prev_query)) => { Option::Some(prev_query) },
        (
            Option::Some(column_query), Option::Some(prev_query)
        ) => { Option::Some(min(*column_query, prev_query)) },
    }

Copy link
Contributor

@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: 5 of 7 files reviewed, 10 unresolved discussions (waiting on @ilyalesokhin-starkware and @spapinistarkware)


stwo_cairo_verifier/src/vcs/hasher.cairo line 48 at r2 (raw file):

        while !column_values.is_empty() {
            let mut word = 0;
            word = word * M31_IN_HASH_SHIFT + column_values.pop_front().unwrap().inner.into();

Consider changing to a for loop of 8 times
Document that 2**31 **8 < Felt252, no?

Code quote:

word = word * M31_IN_HASH_SHIFT + column_values.pop_front().unwrap().inner.into();

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from a86e564 to ffa3b7b Compare June 20, 2024 11:04
@spapinistarkware spapinistarkware mentioned this pull request Jun 20, 2024
Merged
Copy link
Contributor

@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:

Reviewable status: 5 of 7 files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware and @spapinistarkware)

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from ffa3b7b to e830f42 Compare June 20, 2024 11:36
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: 5 of 7 files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


stwo_cairo_verifier/src/vcs/verifier.cairo line 97 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

What is this?
is this the same as unwrap_or?

It's unwrap, but deref() on the positive branch.


stwo_cairo_verifier/src/vcs/verifier.cairo line 102 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Can you use while let?

Nice!


stwo_cairo_verifier/src/vcs/verifier.cairo line 127 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Isn't this done inside fetch_prev_node_hash? why is this comment there?

Done.


stwo_cairo_verifier/src/vcs/verifier.cairo line 154 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

?

Done.


stwo_cairo_verifier/src/vcs/verifier.cairo line 240 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Don't you query everything in the same place with OODS why do we have this complicated logic?

Unfortunately, no. Because each layer has a circle poly commitment.
I need to open f(P) and f(-P).
However, the mixed degree fri, required my parent layer to be opened at 2P.
This means my sybling must be P+(1,0).

Come to think about it, maybe I could always keep P and -P together in the same leaf. This pairing is preserved under P -> 2P...
I'll see if it work in the main repo, but for now, that's how the algo works.


stwo_cairo_verifier/src/vcs/verifier.cairo line 245 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

document,

  • In what sense is this a 'prev' node hash?

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from e830f42 to 2e9c03b Compare July 7, 2024 06:49
@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/hasher.cairo line 3 at r3 (raw file):

use core::array::ArrayTrait;
use core::option::OptionTrait;
// use core::poseidon::poseidon_hash_span;

?

Code quote:

// use core::poseidon::poseidon_hash_span;

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/hasher.cairo line 8 at r3 (raw file):

use core::poseidon::{hades_permutation, HashState};

// Modified version of poseidon_hash_span that doesn't require builtin gas costs

how does the recursion work without the builtin gas costs?

Code quote:

 require builtin gas costs

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from 2e9c03b to 32e9a35 Compare July 7, 2024 07:05
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: 3 of 7 files reviewed, 11 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


stwo_cairo_verifier/src/vcs/hasher.cairo line 3 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

?

Done.


stwo_cairo_verifier/src/vcs/hasher.cairo line 8 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

how does the recursion work without the builtin gas costs?

Oops, this is leftover from, debugging, sorry. Reverted

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware 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 7 files at r1, 1 of 2 files at r2, 1 of 3 files at r3.
Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


stwo_cairo_verifier/src/vcs/verifier.cairo line 242 at r3 (raw file):

/// Fetches the hash of the next node from the previous layer in the Merkle tree.
/// Either from the computed values or from the witness.

Suggestion:

/// The hash Either from the computed values or from the witness.

stwo_cairo_verifier/src/vcs/verifier.cairo line 270 at r3 (raw file):

// use core::poseidon::poseidon_hash_span;

?

Code quote:

// use core::poseidon::poseidon_hash_span

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 242 at r3 (raw file):

/// Fetches the hash of the next node from the previous layer in the Merkle tree.
/// Either from the computed values or from the witness.

sorry, ignore.

@ilyalesokhin-starkware
Copy link
Collaborator

stwo_cairo_verifier/src/vcs/verifier.cairo line 242 at r3 (raw file):

/// Fetches the hash of the next node from the previous layer in the Merkle tree.
/// Either from the computed values or from the witness.

Suggestion:

/// Fetches the hash of the next node from the previous layer in the Merkle tree.
/// The hash is fetched either from the computed values or from the witness.

@spapinistarkware spapinistarkware force-pushed the spapini/05-09-merkleverifier branch from 32e9a35 to eb3e201 Compare July 7, 2024 07:16
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: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


stwo_cairo_verifier/src/vcs/verifier.cairo line 242 at r3 (raw file):

/// Fetches the hash of the next node from the previous layer in the Merkle tree.
/// Either from the computed values or from the witness.

Done.

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware 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 1 of 1 files at r5.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

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.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @shaharsamocha7)

@spapinistarkware spapinistarkware merged commit d0e10c2 into main Jul 7, 2024
4 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