Commitment Scheme evaluation per size

[spapinistarkware]

This change is 

[graphite-app]

Your org has enabled the Graphite merge queue for merging into dev

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

[spapinistarkware]

• parallel wide fib test #511
• Evaluation oods once #496
• Reuse commitment evaluation #495
• Use precomputed twiddles #494
• Add measurement logs #493
• WideFib test with AVX Backend #492
• Implement Backend for AVX512Backend #491
• Make prove() generic in Backend #490
• CommitmentSchemeProver generic in backend #489
• AVX quotients #488
• Clean up fri quotients #486
• Save inverse on quotient numerator #484
• Commitment Scheme evaluation per size #483 👈
• Prepare commitmentschem per size #537
• 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 99.49749% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.37%. Comparing base (1ebc2be) to head (38a3fc8).

Files	Patch %	Lines
src/core/commitment_scheme/quotients.rs	99.22%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #483      +/-   ##
==========================================
+ Coverage   95.35%   95.37%   +0.02%     
==========================================
  Files          68       69       +1     
  Lines        9973    10038      +65     
  Branches     9973    10038      +65     
==========================================
+ Hits         9510     9574      +64     
- Misses        401      402       +1     
  Partials       62       62              

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

[alonh5]

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

a discussion (no related file):
Please separate PR to (1) moving/refactoring/extracting code (2) logical/implementation changes.

---

---

src/core/fri.rs line 425 at r2 (raw file):

        assert_eq!(queries.log_domain_size, self.expected_query_log_domain_size);
        assert_eq!(decommitted_values.len(), self.column_bounds.len());
        println!("decommitted_values: {decommitted_values:#?}");

Forgot to remove?

Code quote:

println!("decommitted_values: {decommitted_values:#?}");

---

src/core/commitment_scheme/prover.rs line 72 at r2 (raw file):

    ) -> CommitmentSchemeProof {
        // Evaluate polynomials on open points.
        let openings = self

Can we stay with the "sample" terminology rather the "opening"? Same for whole PR.

Suggestion:

samples

[spapinistarkware]

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @alonh5)

a discussion (no related file):

Previously, alonh5 (Alon Haramati) wrote…

Please separate PR to (1) moving/refactoring/extracting code (2) logical/implementation changes.

Done.

---

---

src/core/fri.rs line 425 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Forgot to remove?

Done.

---

src/core/commitment_scheme/prover.rs line 72 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can we stay with the "sample" terminology rather the "opening"? Same for whole PR.

Done.

[alonh5]

Reviewed 1 of 9 files at r1, 2 of 4 files at r2, 3 of 3 files at r3, 5 of 5 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @spapinistarkware)

---

src/core/commitment_scheme/prover.rs line 89 at r5 (raw file):

        channel.mix_felts(&proved_values.clone().flatten_cols());

        // Compute oods quotients for boundary constraints on sampled_points.

Suggestion:

the sampled points

---

src/core/commitment_scheme/prover.rs line 92 at r5 (raw file):

        let columns = self.evaluations().flatten();
        let quotients =
            compute_fri_quotients(&columns[..], &samples.flatten(), channel.draw_felt());

Suggestion:

&columns

---

src/core/commitment_scheme/prover.rs line 134 at r5 (raw file):

#[derive(Debug)]
pub struct CommitmentSchemeProof {
    pub sampled_values: TreeVec<ColumnVec<Vec<SecureField>>>,

Why this change?

Code quote:

sampled_values

---

src/core/commitment_scheme/quotients.rs line 35 at r5 (raw file):

/// A batch of column samplings at a point.
pub struct ColumnSampleBatch {

This is technically a row, right? Can we call it that?

Suggestion:

ColumnSampleRow

---

src/core/commitment_scheme/quotients.rs line 42 at r5 (raw file):

}
impl ColumnSampleBatch {
    /// Groups column opening by opening point.

opening -> sample in this file too.

Suggestion:

sample

---

src/core/commitment_scheme/quotients.rs line 45 at r5 (raw file):

    /// # Arguments
    /// opening: For each column, a vector of openings.
    pub fn new(openings: &[&Vec<PointSample>]) -> Vec<Self> {

This is effectively transposing the samples. I think this can be made simpler, consider using a mapping as an intermediate data structure.

---

src/core/commitment_scheme/quotients.rs line 74 at r5 (raw file):

    random_coeff: SecureField,
) -> Vec<SecureEvaluation<B>> {
    izip!(columns, samples)

Suggestion:

zip

---

src/core/commitment_scheme/quotients.rs line 75 at r5 (raw file):

) -> Vec<SecureEvaluation<B>> {
    izip!(columns, samples)
        .group_by(|(c, _)| c.domain.log_size())

From what I see group_by only groups adjacent element with the same defined key. We want to group all columns of the same size regardless of location, right?
Consider using a mapping as an intermediate data structure.
Same below.

Code quote:

.group_by

---

src/core/commitment_scheme/quotients.rs line 79 at r5 (raw file):

        .sorted_by_key(|(log_size, _)| Reverse(*log_size))
        .map(|(log_size, tuples)| {
            let (columns, openings): (Vec<_>, Vec<_>) = multiunzip(tuples);

No need for multiple unzip.

Suggestion:

tuples.unzip()

---

src/core/commitment_scheme/quotients.rs line 160 at r5 (raw file):

            })
            .collect(),
    );

refactor into a for loop for readability?

Code quote:

    let res = SparseCircleEvaluation::new(
        query_domain
            .iter()
            .map(|subdomain| {
                let domain = subdomain.to_circle_domain(&commitment_domain);
                let column_evals = queried_values_per_column
                    .iter_mut()
                    .map(|q| {
                        CircleEvaluation::new(domain, q.take(domain.size()).copied().collect_vec())
                    })
                    .collect_vec();
                // TODO(spapini): bit reverse iterator.
                let values = (0..domain.size())
                    .map(|row| {
                        let domain_point = domain.at(bit_reverse_index(row, log_size));
                        accumulate_row_quotients(
                            &batched_openings,
                            &column_evals.iter().collect_vec(),
                            row,
                            random_coeff,
                            domain_point,
                        )
                    })
                    .collect();
                CircleEvaluation::new(domain, values)
            })
            .collect(),
    );

---

src/core/commitment_scheme/verifier.rs line 69 at r5 (raw file):

                vec![CirclePolyDegreeBound::new(log_size); prove_points.len()]
            })
            .flatten_cols_rev();

This can be removed now, right?

Code quote:

flatten_cols_rev

---

src/core/commitment_scheme/verifier.rs line 83 at r5 (raw file):

        // Get FRI query domains.
        let fri_query_domains = fri_verifier.column_sample_positions(channel);

I think these are actually the opening positions - the positions needed to be opened with respect to the queries. Sorry for the back and forth.

Suggestion:

column_opening_positions

[spapinistarkware]

Reviewed 1 of 3 files at r6.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5)

---

src/core/commitment_scheme/prover.rs line 134 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why this change?

Done.

---

src/core/commitment_scheme/quotients.rs line 42 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

opening -> sample in this file too.

Done.

---

src/core/commitment_scheme/quotients.rs line 45 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is effectively transposing the samples. I think this can be made simpler, consider using a mapping as an intermediate data structure.

Done.

---

src/core/commitment_scheme/quotients.rs line 75 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

From what I see group_by only groups adjacent element with the same defined key. We want to group all columns of the same size regardless of location, right?
Consider using a mapping as an intermediate data structure.
Same below.

Done.

---

src/core/commitment_scheme/verifier.rs line 69 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This can be removed now, right?

Done.

---

src/core/commitment_scheme/verifier.rs line 83 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I think these are actually the opening positions - the positions needed to be opened with respect to the queries. Sorry for the back and forth.

Done.

[alonh5]

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

---

src/core/commitment_scheme/quotients.rs line 45 at r5 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

Nice

---

src/core/commitment_scheme/quotients.rs line 95 at r7 (raw file):

pub fn fri_answers(
    column_log_sizes: Vec<u32>,
    openings: &[Vec<PointSample>],

There are still 12 places in this file this needs to be modified

Suggestion:

samples

---

src/core/commitment_scheme/quotients.rs line 137 at r7 (raw file):

        .collect_vec();

    let mut result = Vec::new();

Suggestion:

evals

---

src/core/commitment_scheme/quotients.rs line 141 at r7 (raw file):

        let domain = subdomain.to_circle_domain(&commitment_domain);
        let mut column_evals = Vec::new();
        for q in queried_values_per_column.iter_mut() {

Suggestion:

for column_values in

[spapinistarkware]

Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

src/core/commitment_scheme/quotients.rs line 95 at r7 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

There are still 12 places in this file this needs to be modified

Done.

---

src/core/commitment_scheme/quotients.rs line 137 at r7 (raw file):

        .collect_vec();

    let mut result = Vec::new();

Done.

---

src/core/commitment_scheme/quotients.rs line 141 at r7 (raw file):

        let domain = subdomain.to_circle_domain(&commitment_domain);
        let mut column_evals = Vec::new();
        for q in queried_values_per_column.iter_mut() {

Done.

[alonh5]

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

[alonh5]

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