Add eq evals constraints

[andrewmilson]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.35%. Comparing base (d1e6267) to head (614b466).
Report is 1 commits behind head on dev.

Files	Patch %	Lines
crates/prover/src/core/backend/simd/column.rs	94.11%	1 Missing ⚠️
crates/prover/src/core/utils.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #741      +/-   ##
==========================================
+ Coverage   92.30%   92.35%   +0.04%     
==========================================
  Files          91       91              
  Lines       12417    12450      +33     
  Branches    12417    12450      +33     
==========================================
+ Hits        11462    11498      +36     
+ Misses        849      846       -3     
  Partials      106      106              

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

[spapinistarkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/constraint_framework/constant_columns.rs line 17 at r1 (raw file):

/// Generates a column with a single one at every `2^log_step` positions, and zero elsewhere.
// TODO(andrew): Optimize. Is a quotients of two coset_vanishing (use succinct rep for verifier).

Not 100% sure what's better for the verifier.

Suggestion:

// TODO(andrew): Consider optimizing. Is a quotients of two coset_vanishing (use succinct rep for verifier).

---

crates/prover/src/constraint_framework/constant_columns.rs line 37 at r1 (raw file):

/// [`CircleDomain`]: crate::core::poly::circle::CircleDomain
/// [`Coset`]: crate::core::circle::Coset
fn coset_index_to_circle_domain_index(coset_index: usize, log_domain_size: u32) -> usize {

Looks very similar to offset_bit_reversed_circle_domain_index(). Can we unite?

---

crates/prover/src/core/backend/simd/column.rs line 118 at r1 (raw file):

        }

        let length = self.length;

Can we use collect() here as well?

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 249 at r1 (raw file):

    ) -> ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>> {
        let eq_evals = SimdBackend::gen_eq_evals(eval_point, SecureField::one()).into_evals();
        let eq_evals_coordinate_columns = eq_evals.into_secure_column().columns;

Might it be better to just keep eq_evals as a secure colum to begin with/

[andrewmilson]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/constant_columns.rs line 17 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Not 100% sure what's better for the verifier.

Done.

---

crates/prover/src/constraint_framework/constant_columns.rs line 37 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Looks very similar to offset_bit_reversed_circle_domain_index(). Can we unite?

United with coset_order_to_circle_domain_order_index

---

crates/prover/src/core/backend/simd/column.rs line 118 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Can we use collect() here as well?

Don't think it would be appropriate.

Could do self.into_iter<Item=SecureField> and then collect into SecureColumn but I don't think that's very efficient given the way PackedSecureField is stored. Can't do self.into_iter<Item=PackedSecureField> because length information could be lost

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 249 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Might it be better to just keep eq_evals as a secure colum to begin with/

Yes. Would require a few changes so added as a TODO for now

[andrewmilson]

• Add eval order and interpolate to SecureEvaluation #794
• Add MLE eval constraints #742
• Add eq evals constraints #741 👈
• Add benchmarks for GKR lookups #673
• Implement LogupOps for SIMD backend #641
• Implement GrandProductOps for SIMD backend #640
• Implement GkrOps for SIMD backend #630
• Implement MleOps for SIMD backend #629
• Add GKR implementation of Logup lookups #623
• Add GKR implementation of Grand Product lookups #620
• Implement GkrOps for CPU backend #621
• Add batch GKR prover and verifier #581
• Rename CPU* structs to Cpu* to conform to Rust's naming convention #617
• Remove AVX backend #616
• Convert wide_fibbonacci AVX backend example to SIMD backend #615
• Update CI to test Wasm simd128, avx2 and aarch64 neon #614
• Fix CirclePointIndex bug in debug mode for 32-bit archs #613
• Change opt-level in benchmarks script #612
• Add benchmarks for SIMD m31 #611
• Add benchmarks for SIMD merkle #610
• dev

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

Join @andrewmilson and the rest of your teammates on Graphite

[spapinistarkware]

Reviewed 4 of 7 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @andrewmilson)

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 74 at r3 (raw file):

        mask_offsets[1..].copy_from_slice(&variable_step_offsets);

        let mask_items = eval.next_extension_interaction_mask(TRACE, mask_offsets);

We should be able to do this with just next row offsets. Talk to me.

[spapinistarkware]

Reviewed 1 of 7 files at r2.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @andrewmilson)

[andrewmilson]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 74 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

We should be able to do this with just next row offsets. Talk to me.

Done.

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 13 at r4 (raw file):

#[educe(Debug, Clone, Copy)]
pub struct EqEvalsMask<E: EvalAtRow, const N_VARIABLES: usize> {
    pub curr: E::EF,

Document fields.

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 57 at r4 (raw file):

        // Check the initial value on half_coset0 and final value on half_coset1.
        // Combining these constraints is safe because `is_first` and `is_second` are never
        // non-zero at the same time on the trace.

Do we really need 2 selectors for each constraint?
We can probably have 1 selector with 2 offsets [0,1] or [0,-1], right?
Do you think it's better this way?

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 12 at r4 (raw file):

#[derive(Educe)]
#[educe(Debug, Clone, Copy)]
pub struct EqEvalsMask<E: EvalAtRow, const N_VARIABLES: usize> {

Suggestion:

MulitlinearEqConstraints

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 12 at r4 (raw file):

#[derive(Educe)]
#[educe(Debug, Clone, Copy)]
pub struct EqEvalsMask<E: EvalAtRow, const N_VARIABLES: usize> {

/// Evaluates EqEvals constraints on a column.
/// Given a column c(P) defined on a circle domain D,
/// and an mle evalaution point r0, r1, ...
/// evaluates constraints that guarantee:
///   c(D[b0,b1,...]) = eq((b0,b1,...), (r0,r1,...))

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 17 at r4 (raw file):

    pub is_first: E::F,
    pub is_second: E::F,
    pub is_half_coset_step_by_log_size: [Option<CircleDomainStepMask<E>>; N_VARIABLES],

Isn't it prettier to just shift by 1?

Suggestion:

CircleDomainStepMask<E>

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 21 at r4 (raw file):

impl<E: EvalAtRow, const N_VARIABLES: usize> EqEvalsMask<E, N_VARIABLES> {
    pub fn draw<const EQ_EVALS_TRACE: usize, const SELECTOR_TRACE: usize>(eval: &mut E) -> Self {

I think this is just new, not draw.

Also, not need to seperate this frunction from eval, right? They should always be called togther AFAIU.
Just make it a single function. I don't think we even need an auxilliary struct in this case.

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 57 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Do we really need 2 selectors for each constraint?
We can probably have 1 selector with 2 offsets [0,1] or [0,-1], right?
Do you think it's better this way?

Nvm, I see you already do this

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 80 at r4 (raw file):

#[derive(Educe)]
#[educe(Debug, Clone, Copy)]
pub struct CircleDomainStepMask<E: EvalAtRow> {

Document what this does, and what constant column it needs.

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 21 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I think this is just new, not draw.

Also, not need to seperate this frunction from eval, right? They should always be called togther AFAIU.
Just make it a single function. I don't think we even need an auxilliary struct in this case.

You do need to externalize curr though. But no need for the entire struct.
I suggest calling the function somthing like eval_eq_constraints and letting it return the value of the eq column, to be used externally. This also makes sure you didn't forget to call the eval, and that you are using the same EvalAtRow, and dont call it twice, etc...

[spapinistarkware]

Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 21 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

You do need to externalize curr though. But no need for the entire struct.
I suggest calling the function somthing like eval_eq_constraints and letting it return the value of the eq column, to be used externally. This also makes sure you didn't forget to call the eval, and that you are using the same EvalAtRow, and dont call it twice, etc...

Note: If you had instantiated this before ever having EvalAtRow, so that you can hold preprocessed data, this would justify a struct.

[andrewmilson]

Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 12 at r4 (raw file):

#[derive(Educe)]
#[educe(Debug, Clone, Copy)]
pub struct EqEvalsMask<E: EvalAtRow, const N_VARIABLES: usize> {

Changed to function

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 12 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

/// Evaluates EqEvals constraints on a column.
/// Given a column c(P) defined on a circle domain D,
/// and an mle evalaution point r0, r1, ...
/// evaluates constraints that guarantee:
///   c(D[b0,b1,...]) = eq((b0,b1,...), (r0,r1,...))

Done.

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 13 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Document fields.

Removed struct.

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 17 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Isn't it prettier to just shift by 1?

Removed.

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 21 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Note: If you had instantiated this before ever having EvalAtRow, so that you can hold preprocessed data, this would justify a struct.

Thanks

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 80 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Document what this does, and what constant column it needs.

Removed.

[spapinistarkware]

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

---

crates/prover/src/examples/xor/eq_eval_constraints.rs line 17 at r5 (raw file):

///
/// See <https://eprint.iacr.org/2023/1284.pdf> (Section 5.1).
pub fn eval_eq_constraints<

Very cool