Blake xor table

[spapinistarkware]

This change is 

[spapinistarkware]

• Blake statement #781
• Blake air #773
• Blake scheduler #770
• Blake round #767
• Optimized lookup combine #765
• Blake xor table #764 👈
• Framework component #761
• Poseidon with logup #712
• Logup trace generation #711
• is_first constant column #710
• Poseidon plain proof #708
• ProverCommitmentScheme::commit_on_evals() #707
• Eval framework info #715
• Eval framework asserts #714
• Eval framework point and domain #713
• Eval framework #706
• Eval framework preperation #705
• Generalize AirExt::mask_points() #704
• Wrap mask with TreeVec #702
• Generalize component_trace() #701
• dev

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

Join @spapinistarkware and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.21%. Comparing base (a2b687c) to head (89854bb).
Report is 2 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #764   +/-   ##
=======================================
  Coverage   93.20%   93.21%           
=======================================
  Files          90       90           
  Lines       11897    11905    +8     
  Branches    11897    11905    +8     
=======================================
+ Hits        11089    11097    +8     
  Misses        707      707           
  Partials      101      101           

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

[shaharsamocha7]

Reviewed 3 of 6 files at r1, 1 of 2 files at r2, 1 of 3 files at r3.
Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/logup.rs line 126 at r2 (raw file):

        shifted_secure_combination(values, EF::from(self.alpha), EF::from(self.z))
    }
    pub fn dummy() -> Self {

Can it be declared with #[cfg(test)]
It is a bit scary that we can use trivial interaction

Code quote:

pub fn dummy() -> Self {

---

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

//! The constant columns correspond only to the smaller table of the lower `ELEM_BITS - EXPAND_BITS`
//! xors: (a_l, b_l, a_l^b_l).
//! The restr of the lookups are computed based on these constant columns.

typo

Suggestion:

The rest of the lookups are computed based on these constant columns.

---

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

    /// 2^EXPAND_BITS columns.
    pub mults: Vec<BaseColumn>,
}

Can you elaborate on what each mult actually counts here?

Code quote:

pub struct XorAccumulator<const ELEM_BITS: u32, const EXPAND_BITS: u32> {
    /// 2^EXPAND_BITS columns.
    pub mults: Vec<BaseColumn>,
}

---

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

    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        let [is_first] = eval.next_interaction_mask(2, [0]);
        let blake_eval = XorTableEval::<'_, _, ELEM_BITS, EXPAND_BITS> {

Fix

Suggestion:

     let xor_eval = XorTableEval::<'_, _, ELEM_BITS, EXPAND_BITS> {

[shaharsamocha7]

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

---

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

{
    pub fn eval(mut self) -> E {
        let [a] = self.eval.next_interaction_mask(2, [0]);

This refers to the constant column, right?
Can you give a,b,c more indicative names?
Also, how would it work if you have more constant columns that are not relevant for xor?

Code quote:

        let [a] = self.eval.next_interaction_mask(2, [0]);

---

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

}

pub fn generate_trace<const ELEM_BITS: u32, const EXPAND_BITS: u32>(

Can you move this and get_interaction_trace in another file?

Code quote:

pub fn generate_trace<const ELEM_BITS: u32, const EXPAND_BITS: u32>(

[shaharsamocha7]

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

---

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

#[test]
fn test_xor_table() {

Rename to test_xor_trace

Do we want another test that actually prove?

Code quote:

#[test]
fn test_xor_table() {

[shaharsamocha7]

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

---

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

Previously, shaharsamocha7 wrote…

This refers to the constant column, right?
Can you give a,b,c more indicative names?
Also, how would it work if you have more constant columns that are not relevant for xor?

I this I got confused, but still why it get next_interaction_mask(2,...)

[spapinistarkware]

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

---

crates/prover/src/constraint_framework/logup.rs line 126 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Can it be declared with #[cfg(test)]
It is a bit scary that we can use trivial interaction

Done.

---

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

Previously, shaharsamocha7 wrote…

Can you elaborate on what each mult actually counts here?

Done.

---

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

Previously, shaharsamocha7 wrote…

Fix

Done.

---

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

Previously, shaharsamocha7 wrote…

I this I got confused, but still why it get next_interaction_mask(2,...)

Done.

---

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

Previously, shaharsamocha7 wrote…

Can you move this and get_interaction_trace in another file?

Done.

---

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

Previously, shaharsamocha7 wrote…

Rename to test_xor_trace

Do we want another test that actually prove?

1. Don't think it just tests the generation. It also tests the constraints.
2. No need. That test wouldn't be testing anything regarding XOR, it would be testing the prover.

[shaharsamocha7]

Reviewed 1 of 3 files at r3, 5 of 6 files at r4, all commit messages.
Reviewable status: 8 of 9 files reviewed, 12 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 17 at r4 (raw file):

    pub fn eval(mut self) -> E {
        // al, bl are the constant columns for the inputs: All pairs of elements in [0,
        // 2^LIMB_BITS).

Do we actually need both al, bl in the constant tree?
This is the same sequential column.

Code quote:

        // al, bl are the constant columns for the inputs: All pairs of elements in [0,
        // 2^LIMB_BITS).

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 41 at r4 (raw file):

                self.logup.push_lookup(
                    &mut self.eval,
                    (-multiplicity).into(),

should we explain why this comes with a minus sign?

Code quote:

(-multiplicity).into(),

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 49 at r4 (raw file):

        self.logup.finalize(&mut self.eval);

        self.eval

remove empty line

Suggestion:

        self.logup.finalize(&mut self.eval);
        self.eval

---

crates/prover/src/examples/blake/xor_table/gen.rs line 21 at r4 (raw file):

pub struct XorTableLookupData<const ELEM_BITS: u32, const EXPAND_BITS: u32> {
    pub xor_accum: XorAccumulator<ELEM_BITS, EXPAND_BITS>,
}

Consider to rename to MultiplicityAccumulator
We can use the exact same thing for RangeCheckUnit and all the other mult things, no?

Code quote:

pub struct XorTableLookupData<const ELEM_BITS: u32, const EXPAND_BITS: u32> {
    pub xor_accum: XorAccumulator<ELEM_BITS, EXPAND_BITS>,
}

---

crates/prover/src/examples/blake/xor_table/gen.rs line 47 at r4 (raw file):

/// Returns the interaction trace, the constant trace, and the claimed sum.
#[allow(clippy::type_complexity)]
pub fn gen_interaction_trace<const ELEM_BITS: u32, const EXPAND_BITS: u32>(

To match with generate_trace

Suggestion:

pub fn generate_interaction_trace<const ELEM_BITS: u32, const EXPAND_BITS: u32>(

---

crates/prover/src/examples/blake/xor_table/gen.rs line 52 at r4 (raw file):

) -> (
    ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,
    ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,

Why interaction trace returns also the constant trace?

Code quote:

    ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,
    ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,

---

crates/prover/src/examples/blake/xor_table/gen.rs line 77 at r4 (raw file):

        let bh1 = i1 as u32 & ((1 << EXPAND_BITS) - 1);

        // Each column has 2^(2*LIMB_BITS) rows, in packs of LOG_LANES.

Packed in N_LANES

Code quote:

in packs of LOG_LANES.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 107 at r4 (raw file):

    }

    // If there is an odd number of columns, handle the last one.

Is that correct?

Suggestion:

    // If there is an odd number of lookup expressions, handle the last one.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 149 at r4 (raw file):

        })
        .collect();
    let mut constant_trace = [a_col, b_col, c_col]

Can you move to another function?

Code quote:

    // Generate the constant columns. In reality, these should be generated before the proof
    // even began.
    let a_col: BaseColumn = (0..(1 << (column_bits::<ELEM_BITS, EXPAND_BITS>())))
        .map(|i| BaseField::from_u32_unchecked((i >> limb_bits) as u32))
        .collect();
    let b_col: BaseColumn = (0..(1 << (column_bits::<ELEM_BITS, EXPAND_BITS>())))
        .map(|i| BaseField::from_u32_unchecked((i & ((1 << limb_bits) - 1)) as u32))
        .collect();
    let c_col: BaseColumn = (0..(1 << (column_bits::<ELEM_BITS, EXPAND_BITS>())))
        .map(|i| {
            BaseField::from_u32_unchecked(((i >> limb_bits) ^ (i & ((1 << limb_bits) - 1))) as u32)
        })
        .collect();
    let mut constant_trace = [a_col, b_col, c_col]

---

crates/prover/src/examples/blake/xor_table/mod.rs line 36 at r4 (raw file):

/// Accumulator that keeps track of the number of times each input has been used.
pub struct XorAccumulator<const ELEM_BITS: u32, const EXPAND_BITS: u32> {
    /// 2^(2*EXPAND_BITS) multiplicity columns. Index (al, bl) fo column (ah, bh) is the number of

of column

Code quote:

fo column

---

crates/prover/src/examples/blake/xor_table/mod.rs line 52 at r4 (raw file):

}
impl<const ELEM_BITS: u32, const EXPAND_BITS: u32> XorAccumulator<ELEM_BITS, EXPAND_BITS> {
    pub fn add_input(&mut self, a: u32x16, b: u32x16) {

The indexing of the mult column is not clear.
Consider give better names, or add a test that explains it

Code quote:

 pub fn add_input(&mut self, a: u32x16, b: u32x16) {

---

crates/prover/src/examples/blake/xor_table/mod.rs line 89 at r4 (raw file):

        };
        xor_eval.eval()
    }

IMO this should be in the constraint file,
and rename it to component.rs

Code quote:

/// Component that evaluates the xor table.
pub struct XorTableComponent<const ELEM_BITS: u32, const EXPAND_BITS: u32> {
    pub lookup_elements: LookupElements,
    pub claimed_sum: SecureField,
}
impl<const ELEM_BITS: u32, const EXPAND_BITS: u32> FrameworkComponent
    for XorTableComponent<ELEM_BITS, EXPAND_BITS>
{
    fn log_size(&self) -> u32 {
        column_bits::<ELEM_BITS, EXPAND_BITS>()
    }
    fn max_constraint_log_degree_bound(&self) -> u32 {
        column_bits::<ELEM_BITS, EXPAND_BITS>() + 1
    }
    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        let [is_first] = eval.next_interaction_mask(2, [0]);
        let xor_eval = XorTableEval::<'_, _, ELEM_BITS, EXPAND_BITS> {
            eval,
            lookup_elements: &self.lookup_elements,
            logup: LogupAtRow::new(1, self.claimed_sum, is_first),
        };
        xor_eval.eval()
    }

[spapinistarkware]

Reviewable status: 6 of 9 files reviewed, 9 unresolved discussions (waiting on @shaharsamocha7)

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 17 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Do we actually need both al, bl in the constant tree?
This is the same sequential column.

They are spaced differentlly. One is
0000111122223333
The other is
0123012301230123

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 49 at r4 (raw file):

Previously, shaharsamocha7 wrote…

remove empty line

Done.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 21 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Consider to rename to MultiplicityAccumulator
We can use the exact same thing for RangeCheckUnit and all the other mult things, no?

Premature abstraction. When we add it, we can change the name.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 47 at r4 (raw file):

Previously, shaharsamocha7 wrote…

To match with generate_trace

Done.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 77 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Packed in N_LANES

Done.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 149 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Can you move to another function?

Done.

---

crates/prover/src/examples/blake/xor_table/mod.rs line 36 at r4 (raw file):

Previously, shaharsamocha7 wrote…

of column

Done.

---

crates/prover/src/examples/blake/xor_table/mod.rs line 52 at r4 (raw file):

Previously, shaharsamocha7 wrote…

The indexing of the mult column is not clear.
Consider give better names, or add a test that explains it

Done.

---

crates/prover/src/examples/blake/xor_table/mod.rs line 89 at r4 (raw file):

Previously, shaharsamocha7 wrote…

IMO this should be in the constraint file,
and rename it to component.rs

Noted. I'd rather keep this here, since in other components we might have a long list of actual constraints, and that file will get overloaded. It is possible to add a component.rs file just for the component, don't think it's necessary.

[shaharsamocha7]

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

---

crates/prover/src/examples/blake/xor_table/constraints.rs line 17 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

They are spaced differentlly. One is
0000111122223333
The other is
0123012301230123

I see.
We need a way to have a view on what each constant column is.
It is not clear from here that this is the structure of al, bl.

---

crates/prover/src/examples/blake/xor_table/gen.rs line 21 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Premature abstraction. When we add it, we can change the name.

ok, but I do think we should use the same code for rc_unit and maybe all the "multiplicity & constant" components

---

crates/prover/src/examples/blake/xor_table/mod.rs line 89 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Noted. I'd rather keep this here, since in other components we might have a long list of actual constraints, and that file will get overloaded. It is possible to add a component.rs file just for the component, don't think it's necessary.

I see your point, but let's discuss that so all components have the same structure.

---

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

//! 2^(ELEM_BITS - EXPAND_BITS).
//! The constant columns correspond only to the smaller table of the lower `ELEM_BITS - EXPAND_BITS`
//! xors: (a_l, b_l, a_l^b_l).

What are their sizes?
2 ^ 2* (ELEM_BITS - EXPAND_BITS)?

Code quote:

//! The constant columns correspond only to the smaller table of the lower `ELEM_BITS - EXPAND_BITS`
//! xors: (a_l, b_l, a_l^b_l).

[spapinistarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

---

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

Previously, shaharsamocha7 wrote…

What are their sizes?
2 ^ 2* (ELEM_BITS - EXPAND_BITS)?

Well, the entire component is of the same length, so yes. I wrote it about the mul columns a line above