Add PreprocessedColumn Structs #966

Gali-StarkWare · 2025-01-08T08:17:26Z

No description provided.

reviewable-StarkWare · 2025-01-08T08:17:36Z

This change is

Gali-StarkWare · 2025-01-08T08:17:51Z

Remove PreProcessedColumn Enum #968
Use PreProcessedColumn Structs and String Instead of Enum #967
Add PreprocessedColumn Structs #966 👈 (View in Graphite)
dev

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

codecov-commenter · 2025-01-08T08:19:01Z

Codecov Report

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

Project coverage is 92.13%. Comparing base (31e8dbc) to head (01a4251).

Files with missing lines	Patch %	Lines
...r/src/constraint_framework/preprocessed_columns.rs	0.00%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #966      +/-   ##
==========================================
- Coverage   92.26%   92.13%   -0.14%     
==========================================
  Files         105      105              
  Lines       14293    14314      +21     
  Branches    14293    14314      +21     
==========================================
  Hits        13188    13188              
- Misses       1032     1053      +21     
  Partials       73       73

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

ohad-starkware

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

crates/prover/src/constraint_framework/preprocessed_columns.rs line 169 at r1 (raw file):

// TODO(Gali): Add documentation.
#[derive(Debug)]
pub struct Plonk {

can you not include this?

crates/prover/src/constraint_framework/preprocessed_columns.rs line 201 at r1 (raw file):

/// A column with `1` at every `2^log_step` positions, `0` elsewhere, shifted by offset.
#[derive(Debug)]
pub struct IsStepWithOffset {

same, if theyre not used, add a todo to implement them
I suspect no one will do those todos for a while, or ever.

Gali-StarkWare

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

crates/prover/src/constraint_framework/preprocessed_columns.rs line 201 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

same, if theyre not used, add a todo to implement them
I suspect no one will do those todos for a while, or ever.

They are used in the examples (I guess this is the reason they exist in the first place), that's why I asked you if this is the right place for them, or maybe now is the time to move their declaration to be in the examples themselves.

ohad-starkware · 2025-01-09T08:37:25Z

crates/prover/src/constraint_framework/preprocessed_columns.rs line 201 at r1 (raw file):

Previously, Gali-StarkWare wrote…

They are used in the examples (I guess this is the reason they exist in the first place), that's why I asked you if this is the right place for them, or maybe now is the time to move their declaration to be in the examples themselves.

Right, I wrote a suggestion in #965 to rid of this

semgrep-code-starkware-libs · 2025-01-09T14:29:18Z

crates/prover/src/examples/preprocessed_columns.rs

+            unsafe {
+                PackedM31::from_simd_unchecked(Simd::from_array(std::array::from_fn(|i| {
+                    if i == 0 {
+                        1
+                    } else {
+                        0
+                    }
+                })))
+            }


~~Detected 'unsafe' usage, please audit for secure usage~~

🎉 Removed in commit c58dc9d 🎉

Semgrep Assistant suggests the following fix: Ensure safe usage of PackedM31::from_simd_unchecked by verifying input validity or using a safe alternative.

View step-by-step instructions

Review the PackedM31::from_simd_unchecked function to understand why it requires unsafe and what invariants must be upheld for safe usage.

Check if there is a safe alternative to from_simd_unchecked that can be used instead. If such a function exists, replace PackedM31::from_simd_unchecked with the safe alternative.

If no safe alternative exists, ensure that the input to from_simd_unchecked is always valid and does not violate any invariants required by the function. This may involve adding checks or assertions before the unsafe block to guarantee safety.

Document the reason for using unsafe and the guarantees that are being upheld in a comment above the unsafe block. This will help future developers understand why unsafe is necessary and how it is being used safely.

This code change should be a good starting point:

Suggested change

unsafe {

PackedM31::from_simd_unchecked(Simd::from_array(std::array::from_fn(|i| {

if i == 0 {

1

} else {

0

}

})))

}

// Ensure that the input to `from_simd_unchecked` is valid.

// In this case, we are creating a SIMD vector with a specific pattern

// that is known to be safe for the `PackedM31` type.

// The use of `unsafe` is justified because we are controlling the input

// and ensuring it adheres to the expected invariants.

let simd_array = Simd::from_array(std::array::from_fn(|i| {

if i == 0 {

1

} else {

0

}

}));

PackedM31::from_simd(simd_array)

_{Leave feedback with a 👍 / 👎. Save a memory with /remember <your custom instructions>.}

semgrep-code-starkware-libs · 2025-01-09T14:49:44Z

Semgrep found 1 unsafe-usage finding:

crates/prover/src/constraint_framework/preprocessed_columns.rs
- L126-134 - Triage

Detected 'unsafe' usage, please audit for secure usage

Gali-StarkWare

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

crates/prover/src/constraint_framework/preprocessed_columns.rs line 169 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

can you not include this?

Done.

crates/prover/src/constraint_framework/preprocessed_columns.rs line 201 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

Right, I wrote a suggestion in #965 to rid of this

Done.

ohad-starkware

Reviewed 1 of 6 files at r3.
Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

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

use crate::core::vcs::blake2_merkle::{Blake2sMerkleChannel, Blake2sMerkleHasher};

pub fn gen_is_first_columns(

why is this here? you're not using it
also, this should be inlined, it doesn't save code really.

Gali-StarkWare

Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

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

Previously, ohad-starkware (Ohad) wrote…

why is this here? you're not using it
also, this should be inlined, it doesn't save code really.

I will be using it instead of gen_preprocessed_columns. I will inline it.

ohad-starkware

Reviewed 2 of 9 files at r2, 2 of 6 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 6 of 10 files reviewed, 1 unresolved discussion (waiting on @Gali-StarkWare and @shaharsamocha7)

shaharsamocha7

Reviewed 1 of 9 files at r2, 1 of 6 files at r3, 1 of 1 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare)

crates/prover/src/examples/plonk/preprocessed_columns.rs line 14 at r6 (raw file):

        format!("preprocessed_plonk_{}", self.wire)
    }
}

I'm not sure this worth it's own file
You can add it to the regular plonk file and please document rather than add TODO

Code quote:

// TODO(Gali): Add documentation.
#[derive(Debug)]
pub struct Plonk {
    pub wire: usize,
}
impl Plonk {
    pub const fn new(wire: usize) -> Self {
        Self { wire }
    }

    pub fn id(&self) -> String {
        format!("preprocessed_plonk_{}", self.wire)
    }
}

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

#[allow(dead_code)]
#[derive(Debug)]
pub struct XorTable {

Consider to move the XorTable generate_constant_trace to here

Code quote:

pub struct XorTable {

Gali-StarkWare

Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

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

Previously, shaharsamocha7 wrote…

Consider to move the XorTable generate_constant_trace to here

Done.

crates/prover/src/examples/plonk/preprocessed_columns.rs line 14 at r6 (raw file):

Previously, shaharsamocha7 wrote…

I'm not sure this worth it's own file
You can add it to the regular plonk file and please document rather than add TODO

Done.

ohad-starkware

Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

Gali-StarkWare requested review from ohad-starkware and shaharsamocha7 January 8, 2025 08:17

Gali-StarkWare marked this pull request as ready for review January 8, 2025 08:17

Gali-StarkWare mentioned this pull request Jan 8, 2025

Add PreProcessedColumn Trait #965

Closed

This was referenced Jan 8, 2025

Use PreProcessedColumn Structs and String Instead of Enum #967

Open

Remove PreProcessedColumn Enum #968

Open

ohad-starkware requested changes Jan 8, 2025

View reviewed changes

Gali-StarkWare requested a review from ohad-starkware January 8, 2025 16:18

Gali-StarkWare commented Jan 8, 2025

View reviewed changes

Gali-StarkWare force-pushed the gali/create_preprocessed_column_trait branch from 1bb5639 to 663fec9 Compare January 9, 2025 13:40

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch 2 times, most recently from bc0097b to 6ba7258 Compare January 9, 2025 14:28

semgrep-code-starkware-libs bot reviewed Jan 9, 2025

View reviewed changes

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from 6ba7258 to c58dc9d Compare January 9, 2025 14:48

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from c58dc9d to 3428754 Compare January 12, 2025 08:53

Gali-StarkWare force-pushed the gali/create_preprocessed_column_trait branch from 663fec9 to c3740fe Compare January 12, 2025 08:55

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from 3428754 to 52a4b0a Compare January 12, 2025 08:55

Gali-StarkWare commented Jan 12, 2025

View reviewed changes

ohad-starkware requested changes Jan 12, 2025

View reviewed changes

Gali-StarkWare requested a review from ohad-starkware January 12, 2025 10:00

Gali-StarkWare commented Jan 12, 2025

View reviewed changes

Gali-StarkWare force-pushed the gali/create_preprocessed_column_trait branch from c3740fe to c7e7607 Compare January 12, 2025 10:06

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from 52a4b0a to ddb66cd Compare January 12, 2025 10:06

ohad-starkware approved these changes Jan 12, 2025

View reviewed changes

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from ddb66cd to 9def2ec Compare January 13, 2025 08:49

Gali-StarkWare changed the base branch from gali/create_preprocessed_column_trait to dev January 13, 2025 08:49

Gali-StarkWare changed the title ~~Add Structs using PreprocessedColumn Trait~~ Add PreprocessedColumn Structs Jan 13, 2025

shaharsamocha7 requested changes Jan 13, 2025

View reviewed changes

Add Structs using PreprocessedColumn Trait

01a4251

Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from 9def2ec to 01a4251 Compare January 13, 2025 13:43

Gali-StarkWare requested review from ohad-starkware and shaharsamocha7 January 13, 2025 13:43

Gali-StarkWare commented Jan 13, 2025

View reviewed changes

ohad-starkware reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PreprocessedColumn Structs #966

Add PreprocessedColumn Structs #966

Gali-StarkWare commented Jan 8, 2025

reviewable-StarkWare commented Jan 8, 2025

Gali-StarkWare commented Jan 8, 2025 •

edited

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

ohad-starkware left a comment

Gali-StarkWare left a comment

ohad-starkware commented Jan 9, 2025

semgrep-code-starkware-libs bot Jan 9, 2025 •

edited

Loading

semgrep-code-starkware-libs bot Jan 9, 2025

semgrep-code-starkware-libs bot commented Jan 9, 2025

Gali-StarkWare left a comment

ohad-starkware left a comment

Gali-StarkWare left a comment

ohad-starkware left a comment

shaharsamocha7 left a comment

Gali-StarkWare left a comment

ohad-starkware left a comment

-            unsafe {
-                PackedM31::from_simd_unchecked(Simd::from_array(std::array::from_fn(|i| {
-                    if i == 0 {
-                    } else {
-                    }
-                })))
-            }
+            // Ensure that the input to `from_simd_unchecked` is valid.
+            // In this case, we are creating a SIMD vector with a specific pattern
+            // that is known to be safe for the `PackedM31` type.
+            // The use of `unsafe` is justified because we are controlling the input
+            // and ensuring it adheres to the expected invariants.
+            let simd_array = Simd::from_array(std::array::from_fn(|i| {
+                if i == 0 {
+                } else {
+                }
+            }));
+            PackedM31::from_simd(simd_array)

Add PreprocessedColumn Structs #966

Are you sure you want to change the base?

Add PreprocessedColumn Structs #966

Conversation

Gali-StarkWare commented Jan 8, 2025

reviewable-StarkWare commented Jan 8, 2025

Gali-StarkWare commented Jan 8, 2025 • edited Loading

codecov-commenter commented Jan 8, 2025 • edited Loading

Codecov Report

ohad-starkware left a comment

Choose a reason for hiding this comment

Gali-StarkWare left a comment

Choose a reason for hiding this comment

ohad-starkware commented Jan 9, 2025

semgrep-code-starkware-libs bot Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

semgrep-code-starkware-libs bot Jan 9, 2025

Choose a reason for hiding this comment

semgrep-code-starkware-libs bot commented Jan 9, 2025

Gali-StarkWare left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

Gali-StarkWare left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

shaharsamocha7 left a comment

Choose a reason for hiding this comment

Gali-StarkWare left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

Gali-StarkWare commented Jan 8, 2025 •

edited

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

semgrep-code-starkware-libs bot Jan 9, 2025 •

edited

Loading