Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Structs using PreprocessedColumn Trait #966

Open
wants to merge 1 commit into
base: gali/create_preprocessed_column_trait
Choose a base branch
from

Conversation

Gali-StarkWare
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Gali-StarkWare Gali-StarkWare marked this pull request as ready for review January 8, 2025 08:17
Copy link
Contributor Author

Gali-StarkWare commented Jan 8, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2025

Codecov Report

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

Project coverage is 92.04%. Comparing base (663fec9) to head (c58dc9d).

Files with missing lines Patch % Lines
...r/src/constraint_framework/preprocessed_columns.rs 0.00% 27 Missing ⚠️
Additional details and impacted files
@@                            Coverage Diff                            @@
##           gali/create_preprocessed_column_trait     #966      +/-   ##
=========================================================================
- Coverage                                  92.26%   92.04%   -0.22%     
=========================================================================
  Files                                        105      105              
  Lines                                      14357    14311      -46     
  Branches                                   14357    14311      -46     
=========================================================================
- Hits                                       13246    13173      -73     
- Misses                                      1038     1065      +27     
  Partials                                      73       73              

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

Copy link
Collaborator

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

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.

Copy link
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.

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
Copy link
Collaborator

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

@Gali-StarkWare Gali-StarkWare force-pushed the gali/create_preprocessed_column_trait branch from 1bb5639 to 663fec9 Compare January 9, 2025 13:40
@Gali-StarkWare Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch 2 times, most recently from bc0097b to 6ba7258 Compare January 9, 2025 14:28
Comment on lines 26 to 34
unsafe {
PackedM31::from_simd_unchecked(Simd::from_array(std::array::from_fn(|i| {
if i == 0 {
1
} else {
0
}
})))
}
Copy link

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

Choose a reason for hiding this comment

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

Detected 'unsafe' usage, please audit for secure usage

🎉 Removed in commit c58dc9d 🎉

Choose a reason for hiding this comment

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

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
  1. Review the PackedM31::from_simd_unchecked function to understand why it requires unsafe and what invariants must be upheld for safe usage.
  2. 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.
  3. 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.
  4. 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>.

@Gali-StarkWare Gali-StarkWare force-pushed the gali/preprocessed_columns_structs branch from 6ba7258 to c58dc9d Compare January 9, 2025 14:48
@semgrep-code-starkware-libs
Copy link

Semgrep found 1 unsafe-usage finding:

  • crates/prover/src/constraint_framework/preprocessed_columns.rs

Detected 'unsafe' usage, please audit for secure usage

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