Skip to content

Remove Preprocessed Columns#411

Closed
Gali-StarkWare wants to merge 1 commit intomainfrom
gali/remove_pp_columns
Closed

Remove Preprocessed Columns#411
Gali-StarkWare wants to merge 1 commit intomainfrom
gali/remove_pp_columns

Conversation

@Gali-StarkWare
Copy link
Copy Markdown
Contributor

@Gali-StarkWare Gali-StarkWare commented Mar 23, 2026

Note

Medium Risk
Swaps locally hardcoded preprocessed-column lists for PreProcessedTrace::canonical_small(), which affects proof configuration sizing and the set/order of column IDs used during verification. If the upstream canonical list changes, proofs/configs could mismatch at runtime.

Overview
Removes the in-crate hardcoded CANONICAL_SMALL_PREPROCESSED_COLUMNS list (and its test) and keeps only MAX_SEQUENCE_LOG_SIZE.

Updates verifier configuration and statement logic (privacy.rs, test.rs, statement.rs) to derive the preprocessed column count and PreProcessedColumnIds directly from stwo_cairo_common::preprocessed_columns::preprocessed_trace::PreProcessedTrace::canonical_small() instead of the deleted local constant.

Written by Cursor Bugbot for commit 1f44a38. This will update automatically on new commits. Configure here.

@reviewable-StarkWare
Copy link
Copy Markdown
Collaborator

This change is Reviewable

Copy link
Copy Markdown
Contributor Author

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

@Gali-StarkWare Gali-StarkWare force-pushed the gali/remove_pp_columns branch from e6e81be to 1f44a38 Compare March 23, 2026 10:30
@Gali-StarkWare Gali-StarkWare marked this pull request as ready for review March 23, 2026 10:30
@Gali-StarkWare Gali-StarkWare self-assigned this Mar 23, 2026
Copy link
Copy Markdown
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyalesokhin-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare).


crates/cairo_air/src/test.rs line 79 at r1 (raw file):

    let proof_config = ProofConfig::from_components(
        &components,
        PreProcessedTrace::canonical_small().columns.len(),

I'd rather not have this in multiple places in the code, not that we want to evantually make it configurable.

Code quote:

PreProcessedTrace::canonical_small()

Copy link
Copy Markdown
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gali-StarkWare made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare and ilyalesokhin-starkware).


crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I'd rather not have this in multiple places in the code, not that we want to evantually make it configurable.

Would you rather I re-export it in preprocessed_column?

Copy link
Copy Markdown
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyalesokhin-starkware made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare).


crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Would you rather I re-export it in preprocessed_column?

You would still call it twice, right? What is the benefit?

Copy link
Copy Markdown
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gali-StarkWare made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare and ilyalesokhin-starkware).


crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

You would still call it twice, right? What is the benefit?

Yes. Right now we call it twice as well, we just call the constant..

@ilyalesokhin-starkware
Copy link
Copy Markdown
Contributor

crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Yes. Right now we call it twice as well, we just call the constant..

a const is a const, you don't call it.

Copy link
Copy Markdown
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gilbens-starkware

@Gali-StarkWare made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on ilyalesokhin-starkware).

Copy link
Copy Markdown
Collaborator

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gilbens-starkware made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on ilyalesokhin-starkware).


crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

a const is a const, you don't call it.

Ilya what what is your suggestion, wrapping it in a function?

@ilyalesokhin-starkware
Copy link
Copy Markdown
Contributor

crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Ilya what what is your suggestion, wrapping it in a function?

I thought we could compute PreProcessedTrace::canonical_small() once here and then pass it to CairoVerifierConfig.

Copy link
Copy Markdown
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyalesokhin-starkware reviewed 1 file.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on Gali-StarkWare).

@ilyalesokhin-starkware
Copy link
Copy Markdown
Contributor

crates/cairo_air/src/test.rs line 79 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I thought we could compute PreProcessedTrace::canonical_small() once here and then pass it to CairoVerifierConfig.

Maybe its better to have PreProcessedTraceVariant in the config:
https://reviewable.io/reviews/starkware-libs/stwo-circuits/429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants