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

Blake round #767

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Blake round #767

merged 1 commit into from
Aug 5, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Jul 29, 2024

This change is Reviewable

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 5af79fa to db98a63 Compare July 30, 2024 11:11
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from f505a0b to 393fc1f Compare July 30, 2024 11:11
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.34%. Comparing base (c4f3336) to head (c845ef5).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #767   +/-   ##
=======================================
  Coverage   93.34%   93.34%           
=======================================
  Files          89       89           
  Lines       11957    11957           
  Branches    11957    11957           
=======================================
  Hits        11161    11161           
  Misses        695      695           
  Partials      101      101           

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

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from db98a63 to c28ed04 Compare July 30, 2024 12:56
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 393fc1f to 3a7de20 Compare July 30, 2024 12:56
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from c28ed04 to 786d728 Compare July 30, 2024 12:59
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 3a7de20 to 7cf2207 Compare July 30, 2024 12:59
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 786d728 to 17e294d Compare July 30, 2024 13:12
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 7cf2207 to 1deb28b Compare July 30, 2024 13:12
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 17e294d to 87a136c Compare July 30, 2024 13:32
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 1deb28b to 1ba9ec1 Compare July 30, 2024 13:32
@spapinistarkware spapinistarkware mentioned this pull request Jul 30, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 87a136c to a27e12e Compare August 1, 2024 07:08
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 1ba9ec1 to 34c742f Compare August 1, 2024 07:08
@spapinistarkware spapinistarkware mentioned this pull request Aug 1, 2024
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 1 of 4 files at r1, 1 of 3 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 12 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/examples/blake/mod.rs line 1 at r2 (raw file):

#![allow(unused)]

Add module documentation
You had something that pointed to the paper.

Code quote:

#![allow(unused)]

crates/prover/src/examples/blake/mod.rs line 48 at r2 (raw file):

    fn draw(channel: &mut Blake2sChannel) -> Self {
        Self {
            xor12: LookupElements::draw(channel, 3),

make constant

Code quote:

3

crates/prover/src/examples/blake/mod.rs line 64 at r2 (raw file):

            4 => &self.xor4,
            _ => panic!("Invalid w"),
        }

Consider to store xor\d in array and have here a general implementation
Same for the accums

Code quote:

        match w {
            12 => &self.xor12,
            9 => &self.xor9,
            8 => &self.xor8,
            7 => &self.xor7,
            4 => &self.xor4,
            _ => panic!("Invalid w"),
        }

crates/prover/src/examples/blake/mod.rs line 69 at r2 (raw file):

#[derive(Clone, Copy, Debug)]
struct Fu32<F>

Isn't that specific for M31?

Code quote:

struct Fu32<F>

crates/prover/src/examples/blake/round/constraints.rs line 10 at r2 (raw file):

use crate::examples::blake::Fu32;

const I16: BaseField = BaseField::from_u32_unchecked(1 << 15);

Took me some time to understand Inv16.. (rename or document)

Code quote:

const I16: BaseField

crates/prover/src/examples/blake/round/constraints.rs line 33 at r2 (raw file):

        self.g(7, v.get_many_mut([3, 4, 9, 14]).unwrap(), m[14], m[15]);

        self.logup.push_lookup(

Comment:
// Yield

Code quote:

        self.logup.push_lookup(

crates/prover/src/examples/blake/round/constraints.rs line 53 at r2 (raw file):

        Fu32 { l, h }
    }
    fn g(&mut self, _round: u32, v: [&mut Fu32<E::F>; 4], m0: Fu32<E::F>, m1: Fu32<E::F>) {

Shouldn't you use it?

Code quote:

_round: u32

crates/prover/src/examples/blake/round/constraints.rs line 95 at r2 (raw file):

            carry_l
                * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 0)))
                * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 1))),

Consider to extract to is_triplet function

Code quote:

            carry_l
                * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 0)))
                * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 1))),

crates/prover/src/examples/blake/round/constraints.rs line 116 at r2 (raw file):

    }

    /// Checks that a, b are in range, and computes their xor rotate.

Suggestion:

    /// Checks that a, b are in range, and computes their xor rotate right by `r` bits.

crates/prover/src/examples/blake/round/constraints.rs line 142 at r2 (raw file):

        let (bhl, bhh) = self.split_unchecked(b.h, 8);

        // These also guarantee that all elements are in range.

This comment should be in the xor doc.

Code quote:

// These also guarantee that all elements are in range.

crates/prover/src/examples/blake/round/constraints.rs line 154 at r2 (raw file):

    }

    /// Checks that a,b in in [0,2^w) and computes their xor.

Suggestion:

/// Checks that a,b are in [0,2^w) and computes their xor.

crates/prover/src/examples/blake/round/gen.rs line 24 at r2 (raw file):

pub struct BlakeLookupData {
    xor_lookups: Vec<(u32, [BaseColumn; 3])>,

document everywhere

Code quote:

xor_lookups: Vec<(u32, [BaseColumn; 3])>,

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from a27e12e to d3fe674 Compare August 5, 2024 06:17
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch 2 times, most recently from b4400c3 to 676d9fa Compare August 5, 2024 07:07
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from d3fe674 to fe41cc2 Compare August 5, 2024 07:09
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 9 files reviewed, 9 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/examples/blake/mod.rs line 1 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Add module documentation
You had something that pointed to the paper.

Which paper?


crates/prover/src/examples/blake/mod.rs line 48 at r2 (raw file):

Previously, shaharsamocha7 wrote…

make constant

Did better


crates/prover/src/examples/blake/mod.rs line 64 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Consider to store xor\d in array and have here a general implementation
Same for the accums

What would I have in the unused slots?


crates/prover/src/examples/blake/mod.rs line 69 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Isn't that specific for M31?

Yes, but F could be packed.


crates/prover/src/examples/blake/round/constraints.rs line 53 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Shouldn't you use it?

Nope, but I removed it. It is only used to select the message, but I'm already passing the selecetd messages to the function.


crates/prover/src/examples/blake/round/constraints.rs line 95 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Consider to extract to is_triplet function

Noted. It is only used here, in these few lines in this relatively short fucntion. Way too early IMO to extract a function. It would also make the function harder to read (anyone would definitely dive into is_triplet). But I did extract a constant.


crates/prover/src/examples/blake/round/constraints.rs line 116 at r2 (raw file):

    }

    /// Checks that a, b are in range, and computes their xor rotate.

Done.


crates/prover/src/examples/blake/round/constraints.rs line 154 at r2 (raw file):

    }

    /// Checks that a,b in in [0,2^w) and computes their xor.

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from fe41cc2 to 4351076 Compare August 5, 2024 08:45
@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 676d9fa to dc4b524 Compare August 5, 2024 09: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: 0 of 9 files reviewed, 9 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/examples/blake/round/gen.rs line 24 at r2 (raw file):

Previously, shaharsamocha7 wrote…

document everywhere

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch 2 times, most recently from e0c64fb to 050ea88 Compare August 5, 2024 11:01
@spapinistarkware spapinistarkware changed the base branch from spapini/07-28-optimized_lookup_combine to dev August 5, 2024 11:01
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 1 of 9 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 2 of 9 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/constraint_framework/logup.rs line 147 at r5 (raw file):

            - EF::from(self.z)
    }
    #[cfg(test)]

can you re-add here?

Code quote:

#[cfg(test)]

crates/prover/src/examples/blake/mod.rs line 1 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Which paper?

I'm not sure, but we had a page that explained the blake function flow. (wikipedia maybe?)
I think that a reference to this could be good.


crates/prover/src/examples/blake/mod.rs line 64 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

What would I have in the unused slots?

Panic if key is not in dict?
I meant dict not an array

@shaharsamocha7
Copy link
Collaborator

crates/prover/src/examples/blake/mod.rs line 1 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I'm not sure, but we had a page that explained the blake function flow. (wikipedia maybe?)
I think that a reference to this could be good.

maybe here
https://en.wikipedia.org/wiki/BLAKE_(hash_function)

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 9 files at r3, 1 of 2 files at r4.
Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/examples/blake/round/gen.rs line 24 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

I don't understand what the xor_lookups means
Specifically it is hard for me to understand what xor() func does.


crates/prover/src/examples/blake/round/gen.rs line 67 at r5 (raw file):

}
impl<'a> TraceGeneratorRow<'a> {
    fn append_felt(&mut self, val: u32x16) {

Do we want to assert/debug_assert here that those are in renge [0,P)?

Code quote:

fn append_felt(&mut self, val: u32x16) {

crates/prover/src/examples/blake/round/gen.rs line 132 at r5 (raw file):

    }

    /// Splits a felt at r.

Consider to write why you only append the high part.

Code quote:

    /// Splits a felt at r.

crates/prover/src/examples/blake/round/gen.rs line 150 at r5 (raw file):

        let (bhl, bhh) = self.split(b >> 16, r);

        let _xorll = self.xor(r, all, bll);

Remove the return value or

add an assert that it is correct.
assert_eq(cr, build(xor_ll, xor_lh, ..)

Code quote:

let _xorll = self.xor(r, all, bll);

crates/prover/src/examples/blake/round/gen.rs line 171 at r5 (raw file):

        let _xorlh = self.xor(8, alh, blh);
        let _xorhl = self.xor(8, ahl, bhl);
        let _xorhh = self.xor(8, ahh, bhh);

same

Code quote:

        let _xorll = self.xor(8, all, bll);
        let _xorlh = self.xor(8, alh, blh);
        let _xorhl = self.xor(8, ahl, bhl);
        let _xorhh = self.xor(8, ahh, bhh);

crates/prover/src/examples/blake/round/gen.rs line 176 at r5 (raw file):

    }

    /// Checks that a, b are in [0, 2^w) and computes their xor.

Can you state the upper bound on w?
It doesn't work for every xor as you append c as felt

Code quote:

    /// Checks that a, b are in [0, 2^w) and computes their xor.

crates/prover/src/examples/blake/round/gen.rs line 273 at r5 (raw file):

        let p = round_lookup_elements.combine(
            &lookup_data
                .round_lookups

why don't you batch those in chunks of two?

Code quote:

            &lookup_data
                .round_lookups

crates/prover/src/examples/blake/round/mod.rs line 21 at r5 (raw file):

    };
    component.evaluate(InfoEvaluator::default())
}

Don't we have a generic impl?

Code quote:

pub fn blake_round_info() -> InfoEvaluator {
    let component = BlakeRoundComponent {
        log_size: 1,
        xor_lookup_elements: BlakeXorElements::dummy(),
        round_lookup_elements: RoundElements::dummy(),
        claimed_sum: SecureField::zero(),
    };
    component.evaluate(InfoEvaluator::default())
}

@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 050ea88 to fd67e73 Compare August 5, 2024 14:23
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 9 files reviewed, 10 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/constraint_framework/logup.rs line 147 at r5 (raw file):

Previously, shaharsamocha7 wrote…

can you re-add here?

I used it for the info(), which is trace generation to get the number of columns.


crates/prover/src/examples/blake/mod.rs line 1 at r2 (raw file):

Previously, shaharsamocha7 wrote…

maybe here
https://en.wikipedia.org/wiki/BLAKE_(hash_function)

Done.


crates/prover/src/examples/blake/mod.rs line 64 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Panic if key is not in dict?
I meant dict not an array

I think match is more straight forward than a dict, and the compiler will have an easier time optimizing it out.


crates/prover/src/examples/blake/round/gen.rs line 24 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I don't understand what the xor_lookups means
Specifically it is hard for me to understand what xor() func does.

Done.


crates/prover/src/examples/blake/round/gen.rs line 67 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Do we want to assert/debug_assert here that those are in renge [0,P)?

Nah...


crates/prover/src/examples/blake/round/gen.rs line 132 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Consider to write why you only append the high part.

I added a comment above the struct that point tot he constraints, so that you can look at both.
I think writing here everythin about the constraints is duplication.


crates/prover/src/examples/blake/round/gen.rs line 150 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Remove the return value or

add an assert that it is correct.
assert_eq(cr, build(xor_ll, xor_lh, ..)

Done.


crates/prover/src/examples/blake/round/gen.rs line 176 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Can you state the upper bound on w?
It doesn't work for every xor as you append c as felt

Done.


crates/prover/src/examples/blake/round/gen.rs line 273 at r5 (raw file):

Previously, shaharsamocha7 wrote…

why don't you batch those in chunks of two?

It's the remainder. A single lookup.
All the previous ones (even number) were already batched.


crates/prover/src/examples/blake/round/mod.rs line 21 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Don't we have a generic impl?

Nope. We need to instantiate the component somehow, and that is not something provided by FrameworkComponent.

@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from fd67e73 to 952d01c Compare August 5, 2024 14:58
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 1 of 9 files at r3, 1 of 2 files at r6, 1 of 2 files at r7.
Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/constraint_framework/logup.rs line 147 at r5 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I used it for the info(), which is trace generation to get the number of columns.

Let's recall to change poseidon accordingly, and also think of the correct way to retreive N_COLUMNS
Can merge it for now


crates/prover/src/examples/blake/round/gen.rs line 28 at r6 (raw file):

    xor_lookups: Vec<(u32, [BaseColumn; 3])>,
    /// A column of round lookup values (v_in, v_out, m).
    round_lookups: [BaseColumn; 16 * 3 * 2],

We are going to have BlakeLookupData for the scheduler, no?

Suggestion:

pub struct BlakeRoundLookupData {
    /// A vector of (w, [a_col, b_col, c_col]) for each xor lookup.
    /// w is the xor width. c_col is the xor col of a_col and b_col.
    xor_lookups: Vec<(u32, [BaseColumn; 3])>,
    /// A column of round lookup values (v_in, v_out, m).
    round_lookup: [BaseColumn; 16 * 3 * 2],

crates/prover/src/examples/blake/round/mod.rs line 88 at r6 (raw file):

        let xor_lookup_elements = BlakeXorElements::dummy();
        let round_lookup_elements = RoundElements::dummy();

Can you use non dummy elements here?

Code quote:

        let xor_lookup_elements = BlakeXorElements::dummy();
        let round_lookup_elements = RoundElements::dummy();

@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 952d01c to 4ffac8d Compare August 5, 2024 15:03
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: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/examples/blake/round/gen.rs line 28 at r6 (raw file):

Previously, shaharsamocha7 wrote…

We are going to have BlakeLookupData for the scheduler, no?

Done.


crates/prover/src/examples/blake/round/mod.rs line 88 at r6 (raw file):

Previously, shaharsamocha7 wrote…

Can you use non dummy elements here?

I think it's ok for tests.

@spapinistarkware spapinistarkware force-pushed the spapini/07-29-blake_round branch from 4ffac8d to c845ef5 Compare August 5, 2024 15:16
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 1 of 2 files at r5, 1 of 2 files at r6, 1 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware merged commit aa53616 into dev Aug 5, 2024
13 of 14 checks passed
@spapinistarkware spapinistarkware mentioned this pull request Aug 7, 2024
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