Create ComponentTraceWriter trait.

[alonh5]

This change is 

[alonh5]

• Add wide fib lookup step constraint. #646
• Extract constraint code into functions. #644
• Use SecureField in interaction. #642
• Modify mask_points for interaction. #639
• Modify trace_log_degree_bounds for interaction. #638
• Add wide fib lookup constraint. #637
• Add interaction to proof. #636
• Split prove function. #634
• Create ComponentTraceWriter trait. #633 👈
• Add interaction_element_ids to Component trait. #631
• Remove wide fibonacci boundary constraint. #582 : 1 other dependent PR (#589 )
• Separate cpu and avx wide fib. #573 : 1 other dependent PR (#574 )
• Get lookup columns. #572
• Configure wide fib. #562
• dev

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

Join @alonh5 and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 92.24%. Comparing base (2c8b6e5) to head (4395873).

Files	Patch %	Lines
crates/prover/src/core/air/air_ext.rs	0.00%	18 Missing ⚠️
crates/prover/src/core/prover/mod.rs	0.00%	7 Missing ⚠️
crates/prover/src/core/mod.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #633      +/-   ##
==========================================
- Coverage   92.52%   92.24%   -0.29%     
==========================================
  Files          69       69              
  Lines        9006     9034      +28     
  Branches     9006     9034      +28     
==========================================
  Hits         8333     8333              
- Misses        601      629      +28     
  Partials       72       72              

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

[shaharsamocha7]

Reviewed 1 of 8 files at r1, 3 of 7 files at r2, all commit messages.
Reviewable status: 4 of 9 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @andrewmilson)

---

crates/prover/src/core/air/mod.rs line 57 at r2 (raw file):

pub trait ComponentTraceWriter<B: Backend> {
    fn write_interaction_trace(

Should this function be autogenerated per component?

Code quote:

fn write_interaction_trace(

---

crates/prover/src/examples/wide_fibonacci/component.rs line 104 at r2 (raw file):

        elements: &InteractionElements,
    ) -> ColumnVec<CircleEvaluation<CPUBackend, BaseField, BitReversedOrder>> {
        let domain = trace[0].domain;

I think that we should have a naming convention here.
trace -> refers to regular trace
interaction_trace -> refers to the interaction..

This convention assumes that we have only one interaction trace.

Suggestion:

let interaction_trace_domain = trace[0].domain;

---

crates/prover/src/examples/wide_fibonacci/component.rs line 105 at r2 (raw file):

    ) -> ColumnVec<CircleEvaluation<CPUBackend, BaseField, BitReversedOrder>> {
        let domain = trace[0].domain;
        let input_trace = trace.iter().map(|eval| &eval.values).collect_vec();

WDYT?

Suggestion:

  let trace_values = trace.iter().map(|eval| &eval.values).collect_vec();

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

}

pub fn write_lookup_column(

This function here is very specfic to fib lookup.
Do we want to generalize it eventually?

Code quote:

pub fn write_lookup_column(

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

}

pub fn write_lookup_column(

Also, document

Code quote:

pub fn write_lookup_column(

[alonh5]

Reviewable status: 4 of 9 files reviewed, 5 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/air/mod.rs line 57 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Should this function be autogenerated per component?

Yes, I think so.

---

crates/prover/src/examples/wide_fibonacci/component.rs line 104 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I think that we should have a naming convention here.
trace -> refers to regular trace
interaction_trace -> refers to the interaction..

This convention assumes that we have only one interaction trace.

Done.

---

crates/prover/src/examples/wide_fibonacci/component.rs line 105 at r2 (raw file):

Previously, shaharsamocha7 wrote…

WDYT?

Done.

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

Previously, shaharsamocha7 wrote…

This function here is very specfic to fib lookup.
Do we want to generalize it eventually?

We could generalize it to get the locations and lengths of the values that should be combined. Using it might make the auto generation easier.

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Also, document

Done.

[shaharsamocha7]

Reviewed 2 of 7 files at r2, 2 of 3 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @andrewmilson)

---

crates/prover/src/core/mod.rs line 68 at r4 (raw file):

    type Output = BaseField;

    fn index(&self, index: &str) -> &Self::Output {

Do we need to return an error here in case that the element is not found?

Code quote:

fn index(&self, index: &str) -> &Self::Output {

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

We could generalize it to get the locations and lengths of the values that should be combined. Using it might make the auto generation easier.

It's not just the locations and length, generally we could get a list of expressions of values of the trace.
If we want to use that function and not autogenerate a different one for each component, it should have other interface.

Anyway it's ok for now.

[alonh5]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/mod.rs line 68 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Do we need to return an error here in case that the element is not found?

The way it is used, only if there's a bug an element can be not found. In that case according to what we decided it shouldn't return an error.

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 25 at r2 (raw file):

Previously, shaharsamocha7 wrote…

It's not just the locations and length, generally we could get a list of expressions of values of the trace.
If we want to use that function and not autogenerate a different one for each component, it should have other interface.

Anyway it's ok for now.

Oh I see. Okay, let's discuss it further offline, for the future.

[shaharsamocha7]

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

---

crates/prover/src/core/mod.rs line 68 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

The way it is used, only if there's a bug an element can be not found. In that case according to what we decided it shouldn't return an error.

I'm saying it because the air infra can potentially have a mismatch in the random elements naming.
Can you explain why it shouldn't return an error?

[andrewmilson]

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

---

crates/prover/src/core/mod.rs line 69 at r4 (raw file):

    fn index(&self, index: &str) -> &Self::Output {
        &self.0.iter().find(|(id, _)| id == index).unwrap().1

WDYT about using BTreeMap? https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#impl-Index%3C%26Q%3E-for-BTreeMap%3CK,+V,+A%3E

Suggestion:

self.0[index]

---

crates/prover/src/core/air/air_ext.rs line 112 at r4 (raw file):

                .iter()
                .map(|component| {
                    let n_columns = component.trace_log_degree_bounds().len();

Should we add n_columns on Component?

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 29 at r4 (raw file):

/// the shifted secure combination of the last two elements in each row.
pub fn write_lookup_column(
    input_trace: &[&Vec<BaseField>],

Suggestion:

&[&[BaseField]]

[alonh5]

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/mod.rs line 68 at r4 (raw file):

Previously, shaharsamocha7 wrote…

I'm saying it because the air infra can potentially have a mismatch in the random elements naming.
Can you explain why it shouldn't return an error?

It should. Added a TODO.

---

crates/prover/src/core/mod.rs line 69 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

WDYT about using BTreeMap? https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#impl-Index%3C%26Q%3E-for-BTreeMap%3CK,+V,+A%3E

Done.

---

crates/prover/src/core/air/air_ext.rs line 112 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Should we add n_columns on Component?

Could be appropriate. Probably in a separate PR. @shaharsamocha7

---

crates/prover/src/examples/wide_fibonacci/trace_gen.rs line 29 at r4 (raw file):

/// the shifted secure combination of the last two elements in each row.
pub fn write_lookup_column(
    input_trace: &[&Vec<BaseField>],

Done.

[andrewmilson]

Reviewed 4 of 6 files at r4, 1 of 1 files at r5, 4 of 5 files at r6, all commit messages.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)

[shaharsamocha7]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @andrewmilson)

---

crates/prover/src/core/air/air_ext.rs line 112 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Could be appropriate. Probably in a separate PR. @shaharsamocha7

n_columns is derived from component.trace_log_degree_bounds()
Do you think that we need extra interface?

[shaharsamocha7]

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

---

crates/prover/src/core/air/air_ext.rs line 111 at r7 (raw file):

                .map(|component| {
                    let n_columns = component.trace_log_degree_bounds().len();
                    let trace_columns = trace_iter.take(n_columns).collect_vec();

This logic takes each component at a time?
I'm not sure I understand this logic.

Code quote:

                    let n_columns = component.trace_log_degree_bounds().len();
                    let trace_columns = trace_iter.take(n_columns).collect_vec();

---

crates/prover/src/core/air/mod.rs line 64 at r7 (raw file):

}

pub trait ComponentProver<B: Backend>: Component + ComponentTraceWriter<B> {

Please add a TODO here to rethink about this because I think it can be changed.
I think that we'll have a struct that impls the ComponentTraceWriter trait.
But maybe it will generate another struct that impl the Component trait.

I'm not sure that we can impl both traits for the same struct.

Code quote:

pub trait ComponentProver<B: Backend>: Component + ComponentTraceWriter<B> {

[alonh5]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/air/air_ext.rs line 112 at r4 (raw file):

Previously, shaharsamocha7 wrote…

n_columns is derived from component.trace_log_degree_bounds()
Do you think that we need extra interface?

Not necessarily. I'm fine with either.

---

crates/prover/src/core/air/air_ext.rs line 111 at r7 (raw file):

Previously, shaharsamocha7 wrote…

This logic takes each component at a time?
I'm not sure I understand this logic.

for each component it takes the columns that are part of it, and passes them into write_interaction_trace(). It's similar to the logic in component_traces().

---

crates/prover/src/core/air/mod.rs line 64 at r7 (raw file):

Previously, shaharsamocha7 wrote…

Please add a TODO here to rethink about this because I think it can be changed.
I think that we'll have a struct that impls the ComponentTraceWriter trait.
But maybe it will generate another struct that impl the Component trait.

I'm not sure that we can impl both traits for the same struct.

Added the TODO. Why can't we?

[shaharsamocha7]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @andrewmilson)

---

crates/prover/src/core/air/air_ext.rs line 111 at r7 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

for each component it takes the columns that are part of it, and passes them into write_interaction_trace(). It's similar to the logic in component_traces().

Does it mean that the interaction columns are not included in the n_columns?
I wonder if we should have a separate function to fetch the component trace.

---

crates/prover/src/core/air/mod.rs line 64 at r7 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Added the TODO. Why can't we?

because trace_writer will need to write the trace (and doesn't necessarily knows how many rows it will have eventually)
We do need to know n_rows for each component, so maybe we'll have that TraceWriter generates the corresponding component.

[alonh5]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/air/air_ext.rs line 111 at r7 (raw file):

Previously, shaharsamocha7 wrote…

Does it mean that the interaction columns are not included in the n_columns?
I wonder if we should have a separate function to fetch the component trace.

I didn't really do it in the right order because it was easier for me this way, but TAL at 638 - there the interaction columns are included in trace_log_degree_bounds().

Do you mean to extract this into a function and use here and in component_traces()? It's just 2 lines and they aren't exactly the same, so I'm not sure if it'll be helpful.

---

crates/prover/src/core/air/mod.rs line 64 at r7 (raw file):

Previously, shaharsamocha7 wrote…

because trace_writer will need to write the trace (and doesn't necessarily knows how many rows it will have eventually)
We do need to know n_rows for each component, so maybe we'll have that TraceWriter generates the corresponding component.

Okay, let's discuss it offline.

[shaharsamocha7]

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

[alonh5]

Merge activity

• May 29, 3:41 AM EDT: @alonh5 started a stack merge that includes this pull request via Graphite.
• May 29, 3:41 AM EDT: @alonh5 merged this pull request with Graphite.