Logup generator returns multiple prefix sums

[shaharsamocha7]

Logup generator finalize get a list of indices and returns the corresponding prefix sum

---

This change is 

[shaharsamocha7]

• State machine prover #845
• State machine AIR #841
• Add constraint for Logup claimed cumsum #830
• Logup generator returns multiple prefix sums #829 👈
• Logup - rename claimed_sum to total_sum #825
• Revert logup without is_first #818
• dev

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

Join @shaharsamocha7 and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.96%. Comparing base (75b94f9) to head (ee7ec83).
Report is 2 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #829      +/-   ##
==========================================
+ Coverage   91.95%   91.96%   +0.01%     
==========================================
  Files          90       90              
  Lines       12195    12211      +16     
  Branches    12195    12211      +16     
==========================================
+ Hits        11214    11230      +16     
  Misses        875      875              
  Partials      106      106              

Flag	Coverage Δ
	91.96% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[Alon-Ti]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson and @shaharsamocha7)

---

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

    col_gen.finalize_col();

    let (trace, [total_sum]) = logup_gen.finalize([(1 << log_size) - 1]);

Isn't it a little weird that you have to specify an index that in all "normal" cases translates to a constant index? Maybe there should be a finalize_last, or conversely finalize_at?

[shaharsamocha7]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @Alon-Ti and @andrewmilson)

---

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

Previously, Alon-Ti wrote…

Isn't it a little weird that you have to specify an index that in all "normal" cases translates to a constant index? Maybe there should be a finalize_last, or conversely finalize_at?

you suggest to rename finalize, to finalize_at()?
I think that there are more API functions that I want to rename here
e.g. next_interaction_mask(..) -> next_trace_column(..), WDYT?

[andrewmilson]

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/constraint_framework/logup.rs line 195 at r1 (raw file):

            .collect_vec();
        (trace, returned_prefix_sums)
    }

Agree with Alon.
Realise I already had this in Draft from a couple days ago but forgot to publish.

Suggestion:

    /// Returns the trace and the claimed sum of the last column.
    pub fn finalize_last(self) -> (
        ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,
        [SecureField; N],
    ) {
        let (trace, [cum_sum] = self.finalize_with_sum_at_indices(...);
        (trace, cum_sum)
    }
    
    /// Returns the trace and the prefix sum of the last column at the corresponding `indices`.
    pub fn finalize_at<const N: usize>(
        mut self,
        indices: [usize; N],
    ) -> (
        ColumnVec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>>,
        [SecureField; N],
    ) {
        // Prefix sum the last column.
        let last_col_coords = self.trace.pop().unwrap().columns;
        let coord_prefix_sum = last_col_coords.map(inclusive_prefix_sum);
        let secure_prefix_sum = SecureColumnByCoords {
            columns: coord_prefix_sum,
        };
        let returned_prefix_sums = indices.map(|idx| {
            // Prefix sum column is in bit-reversed circle domain order.
            let fixed_index = bit_reverse_index(
                coset_index_to_circle_domain_index(idx, self.log_size),
                self.log_size,
            );
            secure_prefix_sum.at(fixed_index)
        });
        self.trace.push(secure_prefix_sum);

        let trace = self
            .trace
            .into_iter()
            .flat_map(|eval| {
                eval.columns.map(|c| {
                    CircleEvaluation::new(CanonicCoset::new(self.log_size).circle_domain(), c)
                })
            })
            .collect_vec();
        (trace, returned_prefix_sums)
    }

[shaharsamocha7]

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @Alon-Ti and @andrewmilson)

---

crates/prover/src/constraint_framework/logup.rs line 195 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Agree with Alon.
Realise I already had this in Draft from a couple days ago but forgot to publish.

Do you suggest to rename finalize -> finalize_at?
or adding more functions?

I think this can be done in another pr, WDYT?

[andrewmilson]

with comment

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/constraint_framework/logup.rs line 195 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Do you suggest to rename finalize -> finalize_at?
or adding more functions?

I think this can be done in another pr, WDYT?

Sounds good to me

I'm not so fussed about the names it just seems that

let (trace, [total_sum]) = logup_gen.finalize([(1 << log_size) - 1]);
(trace, total_sum)

will be a very common operation (will it be?) and seems nice to have a specific function for it

[Alon-Ti]

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

---

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

Previously, shaharsamocha7 wrote…

you suggest to rename finalize, to finalize_at()?
I think that there are more API functions that I want to rename here
e.g. next_interaction_mask(..) -> next_trace_column(..), WDYT?

Agreed, I think mask isn't a word that should make it to the user.

[shaharsamocha7]

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

---

crates/prover/src/constraint_framework/logup.rs line 195 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Sounds good to me

I'm not so fussed about the names it just seems that

let (trace, [total_sum]) = logup_gen.finalize([(1 << log_size) - 1]);
(trace, total_sum)

will be a very common operation (will it be?) and seems nice to have a specific function for it

Done.