Enable_using_secure_run_in_proof_mode #2113
Conversation
|
|
Benchmark Results for unmodified programs 🚀
|
YairVaknin-starkware
force-pushed
the
yairv/enable_using_secure_run_in_proof_mode
branch
from
4c7661b to
9ad4783
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 1032 at r1 (raw file):
stop_ptr.ok_or_else(|| RunnerError::NoStopPointer(Box::new(builtin.name())))?; builtin_segment_info.push((index, stop_ptr)); }
Suggestion:
if proof_mode {
// In proof‐mode there are builtin runners for all builtins, but only the ones that are in the program are finalized (`stop_ptr` is set). Only collect those.
if let Some(stop_ptr) = stop_ptr {
builtin_segment_info.push((index, stop_ptr));
}
} else {
// In non‐proof-mode all builtin runners are from the program and thus should be finalized (`stop_ptr` is set).
let stop_ptr =
stop_ptr.ok_or_else(|| RunnerError::NoStopPointer(Box::new(builtin.name())))?;
builtin_segment_info.push((index, stop_ptr));
}vm/src/vm/runners/cairo_runner.rs line 1032 at r1 (raw file):
stop_ptr.ok_or_else(|| RunnerError::NoStopPointer(Box::new(builtin.name())))?; builtin_segment_info.push((index, stop_ptr)); }
suggestion
Suggestion:
match stop_ptr {
Some(stop_ptr) => {
// comment ...
builtin_segment_info.push((index, stop_ptr));
}
None => {
if !proof_mode {
// comment...
RunnerError::NoStopPointer(Box::new(builtin.name()))
}
}
}
YairVaknin-starkware
force-pushed
the
yairv/enable_using_secure_run_in_proof_mode
branch
2 times, most recently
from
20953cc to
e5ad90b
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)
vm/src/vm/runners/cairo_runner.rs line 1032 at r1 (raw file):
Previously, yuvalsw wrote…
suggestion
Refactored so we only check if proof_mode once.
vm/src/vm/runners/cairo_runner.rs line 1032 at r1 (raw file):
stop_ptr.ok_or_else(|| RunnerError::NoStopPointer(Box::new(builtin.name())))?; builtin_segment_info.push((index, stop_ptr)); }
Done.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2113 +/- ##
======================================================
Coverage 96.63% 96.63%
======================================================
Files 104 104
Lines 43864 43900 +36
======================================================
+ Hits 42388 42424 +36
Misses 1476 1476 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
YairVaknin-starkware
force-pushed
the
yairv/enable_using_secure_run_in_proof_mode
branch
from
e5ad90b to
622a92b
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 1023 at r2 (raw file):
// Closure to process each builtin's segment info based on whether we are in proof mode or not. let mut process: ProcessBuiltinSegmentClosure<'_> = if proof_mode {
While saving the if inside the loop, it complicates the code, also adding other indirections - so I am not sure it's more efficient (saving the if is pretty negligible). Unless I missed some other reason to use a closure here, I prefer the previous way.
Though I will not block on it, if you choose to leave it this way, please explain why.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @YairVaknin-starkware)
YairVaknin-starkware
force-pushed
the
yairv/enable_using_secure_run_in_proof_mode
branch
from
622a92b to
1043f0a
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)
vm/src/vm/runners/cairo_runner.rs line 1023 at r2 (raw file):
Previously, yuvalsw wrote…
While saving the
ifinside the loop, it complicates the code, also adding other indirections - so I am not sure it's more efficient (saving the if is pretty negligible). Unless I missed some other reason to use a closure here, I prefer the previous way.
Though I will not block on it, if you choose to leave it this way, please explain why.
It's not there in terms of efficiency. They should be pretty similar in that regard (most efficient would just be duplication of the loop, but still really negligible for our use case). I Just like this style better and I think this avoids polluting the loop with a logic that could be decided earlier on in the run (it's pretty standard practice to have different methods do different things based on some branch logic, just here there's no need to define them outside the func scope and the logic is pretty "thin").
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 1023 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
It's not there in terms of efficiency. They should be pretty similar in that regard (most efficient would just be duplication of the loop, but still really negligible for our use case). I Just like this style better and I think this avoids polluting the loop with a logic that could be decided earlier on in the run (it's pretty standard practice to have different methods do different things based on some branch logic, just here there's no need to define them outside the func scope and the logic is pretty "thin").
I disagree - the if way is much more readable and straight forward IMO. Anyway as said, not blocking on this if you insist.
vm/src/vm/runners/cairo_runner.rs line 1016 at r4 (raw file):
// Returns a map from builtin base's segment index to stop_ptr offset // Aka the builtin's segment number and its maximum offset
Suggestion:
/// Returns a map from builtin base's segment index to stop_ptr offset
/// Aka the builtin's segment number and its maximum offsetvm/src/vm/runners/cairo_runner.rs line 1017 at r4 (raw file):
// Returns a map from builtin base's segment index to stop_ptr offset // Aka the builtin's segment number and its maximum offset pub fn get_builtin_segments_info(&self) -> Result<Vec<(usize, usize)>, RunnerError> {
Any reason not to use HashMap?
If there is, please just adjust the doc saying it's a vector mapping these values, not "a map".
Code quote:
<Vec<(usize, usize)>vm/src/vm/runners/cairo_runner.rs line 3938 at r4 (raw file):
#[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn get_builtin_segments_info_non_proof_mode() {
consider uniting the test using parametrrization
vm/src/vm/runners/cairo_runner.rs line 3952 at r4 (raw file):
let mut hint_executor = BuiltinHintProcessor::new_empty(); let runner = cairo_run(program_data, &cairo_run_config, &mut hint_executor).unwrap(); assert_eq!(runner.get_builtin_segments_info(), Ok(vec![(2, 6)]));
how only a single segment?
Which builtin is it?
Why is it index 2?
- Same questions for the other test
- Please reply to those questions in comments.
YairVaknin-starkware
force-pushed
the
yairv/enable_using_secure_run_in_proof_mode
branch
from
1043f0a to
763901a
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JulianGCalderon and @yuvalsw)
vm/src/vm/runners/cairo_runner.rs line 1017 at r4 (raw file):
Previously, yuvalsw wrote…
Any reason not to use HashMap?
If there is, please just adjust the doc saying it's a vector mapping these values, not "a map".
No need for random access here, and later used as a tuple in a few places. I don't see a reason to change it.
vm/src/vm/runners/cairo_runner.rs line 3938 at r4 (raw file):
Previously, yuvalsw wrote…
consider uniting the test using parametrrization
they don't have the needed dep. prefer not to add it or create a custom macro for this case.
vm/src/vm/runners/cairo_runner.rs line 3952 at r4 (raw file):
Previously, yuvalsw wrote…
how only a single segment?
Which builtin is it?
Why is it index 2?
- Same questions for the other test
- Please reply to those questions in comments.
It's just range check. idx 2 for non proof mode, idx 4 for proof mode since the layout contains some before it, but not used.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JulianGCalderon and @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 3952 at r4 (raw file):
Previously, YairVaknin-starkware wrote…
It's just range check. idx 2 for non proof mode, idx 4 for proof mode since the layout contains some before it, but not used.
ok, but as said - "Please reply to those questions in comments "
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is