-
Notifications
You must be signed in to change notification settings - Fork 17
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
Integrate Range Check 128 Builtin #357
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
let (mut trace, mut lookup_data, mut sub_components_inputs) = unsafe { | ||
( | ||
ComponentTrace::<N_TRACE_COLUMNS>::uninitialized(log_size), | ||
LookupData::uninitialized(log_n_packed_rows), | ||
SubComponentInputs::uninitialized(log_size), | ||
) | ||
}; |
There was a problem hiding this comment.
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 e66d428 🚀
99d5681
to
9620c4e
Compare
9620c4e
to
8dabf74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @semgrep-code-starkware-libs[bot] from a discussion.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @shaharsamocha7)
bc3afc7
to
4d9ea3d
Compare
c1d308c
to
575e11c
Compare
575e11c
to
e66d428
Compare
8d22c27
to
e9b8976
Compare
755b942
to
5411574
Compare
e9b8976
to
4a4ba6b
Compare
4a4ba6b
to
a418f08
Compare
5411574
to
125e8d7
Compare
a418f08
to
b1541f3
Compare
125e8d7
to
f6a0bb9
Compare
b1541f3
to
601abbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, 6 of 7 files at r7, all commit messages.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 38 at r7 (raw file):
pub struct BuiltinsClaimGenerator { range_check_128_builtin_trace_generator: Option<range_check_builtin_bits_128::ClaimGenerator>,
consider to rename
Suggestion:
range_check_128
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
.map(|range_check_bits_128| { range_check_builtin_bits_128::ClaimGenerator::new( (range_check_bits_128.stop_ptr - range_check_bits_128.begin_addr).ilog2(),
Can you change this to checked_ilog2()?
Code quote:
(range_check_bits_128.stop_ptr - range_check_bits_128.begin_addr).ilog2(),
601abbc
to
659c03c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 38 at r7 (raw file):
Previously, shaharsamocha7 wrote…
consider to rename
IMO it should stay the same for consistency with the other components (we can change all of them in a different PR if you want)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
Previously, shaharsamocha7 wrote…
Can you change this to checked_ilog2()?
Done.
f6a0bb9
to
80c32dd
Compare
659c03c
to
8d77476
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
Previously, Gali-StarkWare wrote…
Done.
ty but the error message is incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
Previously, shaharsamocha7 wrote…
ty but the error message is incorrect
checked_ilog2() returns None in case of a 0 input (and Some(log_size rounded down) else) so it's not a check for receiving a power of 2 from air-infra. Is this better?
8d77476
to
8c25544
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 3 of 8 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
Previously, Gali-StarkWare wrote…
checked_ilog2() returns None in case of a 0 input (and Some(log_size rounded down) else) so it's not a check for receiving a power of 2 from air-infra. Is this better?
I don't understand, we want to make sure that this is a power of 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 54 at r9 (raw file):
(range_check_bits_128.stop_ptr - range_check_bits_128.begin_addr).ilog2(), range_check_bits_128.begin_addr as u32, )
consider to move this to a util function in a later pr.
Code quote:
assert!(
(range_check_bits_128.stop_ptr - range_check_bits_128.begin_addr)
.is_power_of_two(),
"range_check_bits_128 segment length is not a power of two"
);
range_check_builtin_bits_128::ClaimGenerator::new(
(range_check_bits_128.stop_ptr - range_check_bits_128.begin_addr).ilog2(),
range_check_bits_128.begin_addr as u32,
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-f and @andrewmilson)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 47 at r7 (raw file):
Previously, shaharsamocha7 wrote…
I don't understand, we want to make sure that this is a power of 2.
I know, now we do. Using checked_ilog2() wasn't enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)
stwo_cairo_prover/crates/prover/src/cairo_air/builtins_air.rs
line 54 at r9 (raw file):
Previously, shaharsamocha7 wrote…
consider to move this to a util function in a later pr.
Will do
8c25544
to
fee7b1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alon-f and @andrewmilson)
This change is