starkware-libs / stwo

Apache License 2.0
251 stars 86 forks source link

Support shared preproccessed columns. #869

Closed ilyalesokhin-starkware closed 2 weeks ago

reviewable-StarkWare commented 3 weeks ago

This change is Reviewable

ilyalesokhin-starkware commented 3 weeks ago

crates/prover/src/core/vcs/verifier.rs line 61 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…
Should we use tracing logs here i.e. ```info!(...)``` since we use for similar logs in other spots like [here](https://github.com/starkware-libs/stwo/blob/dev/crates/prover/src/core/prover/mod.rs#L80)

IT was here for debugging, I'll remove that. Thanks.

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 72.88136% with 32 lines in your changes missing coverage. Please review.

Project coverage is 91.73%. Comparing base (8385e54) to head (971c8cf).

Files with missing lines Patch % Lines
...rates/prover/src/constraint_framework/component.rs 74.66% 18 Missing and 1 partial :warning:
crates/prover/src/core/air/components.rs 66.66% 11 Missing and 1 partial :warning:
crates/prover/src/constraint_framework/info.rs 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #869 +/- ## ========================================== - Coverage 91.92% 91.73% -0.19% ========================================== Files 92 92 Lines 12626 12731 +105 Branches 12626 12731 +105 ========================================== + Hits 11606 11679 +73 - Misses 910 939 +29 - Partials 110 113 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/constraint_framework/component.rs line 141 at r3 (raw file):

Previously, shaharsamocha7 wrote…
mask_offset [0] is empty vec before adding the preprocessed columns? maybe we should put a dbg_assert?

yes

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/constraint_framework/component.rs line 71 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…
Done.

done

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/core/air/components.rs line 37 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…
WDYT?

I think it is slightly less efficient, but I don't mind if you think it is better.

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/examples/blake/air.rs line 93 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…
Seems ok to me for now. Was there somewhere you wanted to put it?

maybe in verify_blake? as the verifier is supposed to know the structure of the preproccess trace.

The problem is that I do want the assert here: https://github.com/starkware-industries/stwo/blob/b36d75f09347977062a1eb077db6c11069996980/crates/prover/src/examples/blake/air.rs#L439

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/core/air/components.rs line 77 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…
This isn't performance critical. WDYT? I think offset should be renamed to index because column offset I usually associate with a column's mask offset.

I return component_trace_log_sizes at the end of the map.

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/core/air/components.rs line 68 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…
Optional: could just do one storing Option.

I would have to convert it to a vector of non-options at the end.

ilyalesokhin-starkware commented 2 weeks ago

crates/prover/src/constraint_framework/component.rs line 134 at r8 (raw file):

                        );
                        next_column
                    })

Note that this removes duplicated columns.

Code quote:

                    .entry(*col)
                    .or_insert({
                        assert_matches!(
                            location_allocator.preprocessed_columns_allocation_mode,
                            PreprocessedColumnsAllocationMode::Dynamic,
                        );
                        next_column
                    })