Poseidon with logup

[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

Attention: Patch coverage is 98.59155% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.25%. Comparing base (68ec132) to head (2150ceb).

Files	Patch %	Lines
crates/prover/src/constraint_framework/logup.rs	98.59%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #712      +/-   ##
==========================================
+ Coverage   92.80%   93.25%   +0.45%     
==========================================
  Files          89       89              
  Lines       11725    11795      +70     
  Branches    11725    11795      +70     
==========================================
+ Hits        10881    11000     +119     
+ Misses        744      694      -50     
- Partials      100      101       +1     

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

[alonh5]

Reviewed 4 of 4 files at r4.
Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/logup.rs line 30 at r4 (raw file):

    pub prev_mask: E::EF,
    pub is_first: E::F,
}

Please document. Specifically what is queue and BATCH_SIZE.

Code quote:

pub struct LogupAtRow<const BATCH_SIZE: usize, E: EvalAtRow> {
    pub interaction: usize,
    pub queue: [(E::EF, E::EF); BATCH_SIZE],
    pub queue_size: usize,
    pub claimed_sum: SecureField,
    pub prev_mask: E::EF,
    pub is_first: E::F,
}

---

crates/prover/src/constraint_framework/logup.rs line 52 at r4 (raw file):

            values,
            E::EF::zero() + lookup_elements.alpha,
            E::EF::zero() + lookup_elements.z,

Add From<SecureField> to EF?

Suggestion:

            lookup_elements.alpha.into(),
            lookup_elements.z.into(),

---

crates/prover/src/constraint_framework/logup.rs line 57 at r4 (raw file):

    }

    pub fn push_frac(&mut self, eval: &mut E, p: E::EF, q: E::EF) {

Suggestion:

pub fn push_frac(&mut self, eval: &mut E, numerator: E::EF, denom: E::EF) {

---

crates/prover/src/constraint_framework/logup.rs line 96 at r4 (raw file):

            std::array::from_fn(|_| eval.next_interaction_mask(self.interaction, [0, -1]));
        let cur = E::combine_ef(cumulative_mask_values.map(|[cur, _prev]| cur));
        let up = E::combine_ef(cumulative_mask_values.map(|[_cur, prev]| prev));

Suggestion:

        let cur = E::combine_ef(cumulative_mask_values.map(|[cur, _up]| cur));
        let up = E::combine_ef(cumulative_mask_values.map(|[_cur, up]| up));

---

crates/prover/src/examples/poseidon/mod.rs line 118 at r4 (raw file):

        logup: LogupAtRow::new(1, SecureField::zero(), BaseField::zero()),
    };
    counter.eval.next_interaction_mask(2, [0]);

Doesn't one of the columns have an offset -1 also? How does this work?

Code quote:

counter.eval.next_interaction_mask(2, [0]);

---

crates/prover/src/examples/poseidon/mod.rs line 131 at r4 (raw file):

    }

    fn trace_log_degree_bounds(&self) -> TreeVec<ColumnVec<u32>> {

Can you move these two implementations below to InfoEvaluator?

---

crates/prover/src/examples/poseidon/mod.rs line 255 at r4 (raw file):

struct PoseidonEval<E: EvalAtRow> {
    eval: E,
    logup: LogupAtRow<2, E>,

Does LogupAtRow have to be separate from EvalAtRow? Can it be internal?

Code quote:

LogupAtRow

---

crates/prover/src/examples/poseidon/mod.rs line 308 at r4 (raw file):

            // Provide state lookup.
            self.logup
                .push_lookup(&mut self.eval, -E::EF::one(), &state, self.lookup_elements);

Why is this -1 while the initial state's numerator is 1? Wouldn't the -1 be in the callers column?

Code quote:

-E::EF::one()

---

crates/prover/src/examples/poseidon/mod.rs line 362 at r4 (raw file):

                .iter_mut()
                .zip(state)
                .for_each(|(res, si)| res.data[vec_index] = si);

Accident?

Code quote:

si

[alonh5]

Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/logup.rs line 96 at r4 (raw file):

            std::array::from_fn(|_| eval.next_interaction_mask(self.interaction, [0, -1]));
        let cur = E::combine_ef(cumulative_mask_values.map(|[cur, _prev]| cur));
        let up = E::combine_ef(cumulative_mask_values.map(|[_cur, prev]| prev));

Or actually maybe lets use row_prev and col_prev?

[spapinistarkware]

Reviewable status: 1 of 7 files reviewed, 7 unresolved discussions (waiting on @alonh5)

---

crates/prover/src/constraint_framework/logup.rs line 30 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Please document. Specifically what is queue and BATCH_SIZE.

Done.

---

crates/prover/src/constraint_framework/logup.rs line 57 at r4 (raw file):

    }

    pub fn push_frac(&mut self, eval: &mut E, p: E::EF, q: E::EF) {

Done.

---

crates/prover/src/examples/poseidon/mod.rs line 118 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Doesn't one of the columns have an offset -1 also? How does this work?

It's is_first

---

crates/prover/src/examples/poseidon/mod.rs line 131 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you move these two implementations below to InfoEvaluator?

I don't understand. Also, note that in 2 PRs, these will be auto implemented for any framework component.

---

crates/prover/src/examples/poseidon/mod.rs line 255 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Does LogupAtRow have to be separate from EvalAtRow? Can it be internal?

2 different things. EvalAtRow is a trait that we imoplemented with either point, domain, info or assert ops. LogupAtRow is a specific struct that does the constraints of logup. It also is generic over EvalAtRow, like the component constriants.

---

crates/prover/src/examples/poseidon/mod.rs line 308 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why is this -1 while the initial state's numerator is 1? Wouldn't the -1 be in the callers column?

I decided to reverse this, since the callers will be called a lot more times than the callees (which have multiplicity). So, I thought it makes sense to not have negations in the common case. Also, the multiplicity could incorporate the negatives themeselves, so in most cases, no negations at all.

---

crates/prover/src/examples/poseidon/mod.rs line 362 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Accident?

I wanted to distinguish this from state, since it's just a single elements of the state (state_i)

[alonh5]

Reviewed 3 of 6 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/logup.rs line 75 at r7 (raw file):

        }

        // Compute sum_i pi/qi over batch, as a fraction, p/q.

Fix to match name change.

Code quote:

// Compute sum_i pi/qi over batch, as a fraction, p/q.

---

crates/prover/src/constraint_framework/logup.rs line 87 at r7 (raw file):

        self.queue_size = 1;

        // Add a constraint that p / q = diff.

Here also.

Code quote:

// Add a constraint that p / q = diff.

---

crates/prover/src/constraint_framework/logup.rs line 89 at r7 (raw file):

        // Add a constraint that p / q = diff.
        let cur_cumsum = E::combine_ef(std::array::from_fn(|_| {
            eval.next_interaction_mask(1, [0])[0]

Suggestion:

self.interaction

---

crates/prover/src/constraint_framework/logup.rs line 107 at r7 (raw file):

            std::array::from_fn(|_| eval.next_interaction_mask(self.interaction, [0, -1]));
        let cur_cumsum = E::combine_ef(cumsum_mask.map(|[cur_row, _prev_row]| cur_row));
        let prev_row_cumsum = E::combine_ef(cumsum_mask.map(|[_cur_row, prev_row]| prev_row));

I thought row_prev could refer to the previous value in the row (left) and col_prev to the previous value in the column (up).
Alternatively you can say prev_row is the previous row value (up) and prev_col the previous columns value (left).
Now it's a mix of both and is more confusing IMO.

Code quote:

        let cur_cumsum = E::combine_ef(cumsum_mask.map(|[cur_row, _prev_row]| cur_row));
        let prev_row_cumsum = E::combine_ef(cumsum_mask.map(|[_cur_row, prev_row]| prev_row));

---

crates/prover/src/examples/poseidon/mod.rs line 362 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I wanted to distinguish this from state, since it's just a single elements of the state (state_i)

So how about state_i?

[alonh5]

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

[alonh5]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

crates/prover/src/constraint_framework/logup.rs line 87 at r8 (raw file):

        self.queue_size = 1;

        // Add a constraint that p / q = diff.

Suggestion:

num / denom = diff

[alonh5]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

[spapinistarkware]

Merge activity

• Jul 30, 9:05 AM EDT: @spapinistarkware started a stack merge that includes this pull request via Graphite.
• Jul 30, 9:05 AM EDT: @spapinistarkware merged this pull request with Graphite.