Fix_no_trace_padding_flow_in_proof_mode #1909
Conversation
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
d7fd54d to
1af457b
Compare
|
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
1af457b to
21171ad
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1909 +/- ##
=======================================
Coverage 96.35% 96.35%
=======================================
Files 102 102
Lines 41005 41095 +90
=======================================
+ Hits 39511 39599 +88
- Misses 1494 1496 +2 ☔ View full report in Codecov by Sentry. |
|
Benchmark Results for unmodified programs 🚀
|
YairVaknin-starkware
requested review from
igaray,
Oppen,
fmoletta,
juanbono,
pefontana and
gabrielbosio
as code owners
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
6 times, most recently
from
3036bc4 to
6cd85de
Compare
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/runners/builtin_runner/mod.rs line 202 at r2 (raw file):
Some(0) => Ok(0), Some(ratio) => { if !vm.disable_trace_padding {
please explain (doc) why it isn't relevant with disable_trace_padding
vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):
// Ensure the last step is not a power of two - true for this specific program, not always. assert!(!last_step_true.is_power_of_two());
if you know the exact number, maybe best to assert it's that number. (but do comment that the important thing here is to assert it did not round to a power of 2).
Same above - you can check the exact power of 2 you expect.
vm/src/vm/runners/builtin_runner/mod.rs line 929 at r2 (raw file):
for (builtin_runner_false, builtin_runner_true) in builtin_runners_false .iter() .zip(builtin_runners_true.iter())
consider using zip_eq
This isn't checking everything if they are not of the same length.
vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):
let allocated_instances = if vm.disable_trace_padding { div_ceil(numerator, ratio as usize)
Please add doc saying why it's ok that it's not accurate (not used, right?) and why it's related to no padding.
vm/src/vm/runners/builtin_runner/mod.rs line 228 at r1 (raw file):
}; Ok(allocated_instances)
suggestion
Suggestion:
let ratio_den = self.ratio_den().or(1);
let numerator = vm.current_step * ratio_den as usize;
let allocated_instances = if vm.disable_trace_padding {
div_ceil(numerator, ratio as usize)
} else {
safe_div_usize(numerator, ratio as usize)
.map_err(|_| MemoryError::ErrorCalculatingMemoryUnits)?
};
Ok(allocated_instances)
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
6cd85de to
0e23543
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):
Previously, yuvalsw wrote…
Please add doc saying why it's ok that it's not accurate (not used, right?) and why it's related to no padding.
Done
vm/src/vm/runners/builtin_runner/mod.rs line 228 at r1 (raw file):
Previously, yuvalsw wrote…
suggestion
Done
vm/src/vm/runners/builtin_runner/mod.rs line 202 at r2 (raw file):
Previously, yuvalsw wrote…
please explain (doc) why it isn't relevant with disable_trace_padding
Done
vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):
Previously, yuvalsw wrote…
if you know the exact number, maybe best to assert it's that number. (but do comment that the important thing here is to assert it did not round to a power of 2).
Same above - you can check the exact power of 2 you expect.
I think I prefer it the way it is now. How strongly do you feel about changing it?
vm/src/vm/runners/builtin_runner/mod.rs line 929 at r2 (raw file):
Previously, yuvalsw wrote…
consider using zip_eq
This isn't checking everything if they are not of the same length.
They don't import the needed crate in cargo.toml, and would like to avoid adding to it.
So just added an assertion they are of equal lengths.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):
Previously, YairVaknin-starkware wrote…
Done
also "saying why it's ok that it's not accurate" please
vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
I think I prefer it the way it is now. How strongly do you feel about changing it?
Not super strong. Just generally - tests should have less code and be more specific so that you can be more certain their code itself is good (so they test what you want).
vm/src/vm/runners/builtin_runner/mod.rs line 205 at r3 (raw file):
// if we disable trace padding, we don't need to check the min_step, // since we won't have the guarantee of number of steps being padded to // accommodate each builtin size.
please move to above the if (as it refers to what doesn't happen in the if...).
Code quote:
// if we disable trace padding, we don't need to check the min_step,
// since we won't have the guarantee of number of steps being padded to
// accommodate each builtin size.
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
0e23543 to
29a302a
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):
Previously, yuvalsw wrote…
also "saying why it's ok that it's not accurate" please
Done.
vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):
Previously, yuvalsw wrote…
Not super strong. Just generally - tests should have less code and be more specific so that you can be more certain their code itself is good (so they test what you want).
I agree. And here this is the thing we want to test.
|
Please provide more context in the description of the PR. For example, Is this change a bugfix of the VM or an improvement for a specific use? This is important because changing to |
Hi, so we would like it to not fail the division when trace padding is disabled, since it won't be a multiple of the number of steps in this case. This check is only done in proof_mode (in finalize_segments), and most of the time it will throw an error when we disable trace padding, which would not let us complete the non padded run. |
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
29a302a to
d6d42f5
Compare
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 @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
a discussion (no related file):
Blocking for now as we need to understand if the other logic we talked about overrides this one or lives along it. If it overrides, then maybe no need to merge this one. You can change this PR to the new logic or send a different one, as you see fit.
CHANGELOG.md
Outdated
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| #### Upcoming Changes | |||
|
|
|||
| * fix: Fix no trace padding flow in proof mode [#1909](https://github.com/lambdaclass/cairo-vm/pull/1909) | |||
There was a problem hiding this comment.
This is an API breaking change. It must be documented as such.
There was a problem hiding this comment.
Done.
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
d6d42f5 to
23e6312
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)
a discussion (no related file):
Previously, yuvalsw wrote…
Blocking for now as we need to understand if the other logic we talked about overrides this one or lives along it. If it overrides, then maybe no need to merge this one. You can change this PR to the new logic or send a different one, as you see fit.
Changed to a different logic as discussed
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @YairVaknin-starkware)
vm/src/vm/runners/builtin_runner/mod.rs line 223 at r6 (raw file):
.map_err(|_| MemoryError::ErrorCalculatingMemoryUnits)? };
please revert this newline (just to avoid having traces here while you didn't actually change anything here).
vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):
let used_cells = self.get_used_cells(&vm.segments)?; if vm.disable_trace_padding { // If trace padding is disabled, we pad the used cells to still ensure that the
Sorry for nagging, but can you please doc the effects of disable_trace_padding in its definition? I'd like to avoid the case of proof_mode in which no one knows what exactly it does and thus are worried changing/touching it.
vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):
} else { 0 };
assert that padded_used_cells >= used_cells?
Or maybe even compute the used_cells as used_instances * cells_per_instance, and assert it is equal to used_cells?
The computation seems messy and I am concerned it may have inconsistencies.
Or maybe just compute it that way, and convert the assert with a test (asserting both ways of computation yield the same numbers, for all the builtins).
vm/src/vm/runners/builtin_runner/mod.rs line 935 at r6 (raw file):
for (builtin_runner_false, builtin_runner_true) in builtin_runners_false .iter() .zip(builtin_runners_true.iter())
this assumes they are in the same order. Makes sense, but consider documenting it.
Code quote:
for (builtin_runner_false, builtin_runner_true) in builtin_runners_false
.iter()
.zip(builtin_runners_true.iter())vm/src/vm/runners/builtin_runner/mod.rs line 945 at r6 (raw file):
let (_, allocated_size_false) = builtin_runner_false .get_used_cells_and_allocated_size(&runner_false.vm) .unwrap_or((0, 0));
any reason this should be Err? If not, just unwrap should do better.
Code quote:
unwrap_or((0, 0));vm/src/vm/runners/builtin_runner/mod.rs line 954 at r6 (raw file):
) .unwrap(); let allocated_instances_true = safe_div_usize(
Suggestion:
let n_allocated_instances_false = safe_div_usize(
allocated_size_false,
builtin_runner_false.cells_per_instance() as usize,
)
.unwrap();
let n_allocated_instances_true = safe_div_usize(vm/src/vm/runners/builtin_runner/mod.rs line 966 at r6 (raw file):
allocated_instances_true == 0 || allocated_instances_true != allocated_instances_false );
unclear. What does it check?
Code quote:
// True for this specific program, not always. That is, instance could could happen to
// be the same.
assert!(
allocated_instances_true == 0
|| allocated_instances_true != allocated_instances_false
);vm/src/vm/runners/builtin_runner/mod.rs line 223 at r5 (raw file):
let numerator = vm.current_step * ratio_den as usize; let allocated_instances = if vm.disable_trace_padding {
How come this is not needed? Aren't the safe_divs failing? Ratio is still irrelevant in the case of disable_trace_padding.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)
vm/src/vm/runners/builtin_runner/mod.rs line 223 at r5 (raw file):
Previously, yuvalsw wrote…
How come this is not needed? Aren't the safe_divs failing? Ratio is still irrelevant in the case of disable_trace_padding.
We don't reach here when disabling trace padding. TAL at fn get_used_cells_and_allocated_size.
vm/src/vm/runners/builtin_runner/mod.rs line 223 at r6 (raw file):
Previously, yuvalsw wrote…
please revert this newline (just to avoid having traces here while you didn't actually change anything here).
Done.
vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):
Previously, yuvalsw wrote…
Sorry for nagging, but can you please doc the effects of disable_trace_padding in its definition? I'd like to avoid the case of proof_mode in which no one knows what exactly it does and thus are worried changing/touching it.
it's defined in the config of the runner, then passed to the vm. In which struct would you like it documented (or both)? Also, not sure I get you remark regarding proof_mode.
vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):
Previously, yuvalsw wrote…
assert that padded_used_cells >= used_cells?
Or maybe even compute the used_cells as used_instances * cells_per_instance, and assert it is equal to used_cells?
The computation seems messy and I am concerned it may have inconsistencies.
Or maybe just compute it that way, and convert the assert with a test (asserting both ways of computation yield the same numbers, for all the builtins).
the >= check is done implicitly. get_used_instances just does div_ceil(used_cells, cells_per_instance()). Also, the second remark isn't always true for the last instance (some outputs might not be filled).
vm/src/vm/runners/builtin_runner/mod.rs line 935 at r6 (raw file):
Previously, yuvalsw wrote…
this assumes they are in the same order. Makes sense, but consider documenting it.
Yeah, it could be assumed, but you're right that it's much clearer if state explicitly. I added an assert at the start of the loop that that's the case.
vm/src/vm/runners/builtin_runner/mod.rs line 945 at r6 (raw file):
Previously, yuvalsw wrote…
any reason this should be Err? If not, just unwrap should do better.
Done.
vm/src/vm/runners/builtin_runner/mod.rs line 966 at r6 (raw file):
Previously, yuvalsw wrote…
unclear. What does it check?
That there was indeed a different outcome in the number of allocated instances (basically, that true didn't pad according to the ratio)
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
23e6312 to
a8a42d8
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @YairVaknin-starkware)
vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):
Previously, YairVaknin-starkware wrote…
it's defined in the config of the runner, then passed to the vm. In which struct would you like it documented (or both)? Also, not sure I get you remark regarding proof_mode.
Both. I'd write it in the outer one (runner), and in the vm just refer to it for more details.
re proof_mode - it has many effects, it got to the state where it's hard to tell exactly what they are. I don't want disable_trace_padding to get to the same state.
vm/src/vm/runners/builtin_runner/mod.rs line 967 at r7 (raw file):
// Checks that the number of allocated instances is diffrent when trace padding is // disabled. True for this specific program, not always. That is, instance could could // happen to be the same.
clearer than before. My suggestion for more clarity:
Suggestion:
// Checks that the number of allocated instances is diffrent when trace padding is
// enabled/disabled. Holds for this specific program, not always (that is, in other programs, padding may be of size 0).
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
a8a42d8 to
a79d35a
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)
vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):
Previously, yuvalsw wrote…
Both. I'd write it in the outer one (runner), and in the vm just refer to it for more details.
re proof_mode - it has many effects, it got to the state where it's hard to tell exactly what they are. I don't want disable_trace_padding to get to the same state.
Done.
vm/src/vm/runners/builtin_runner/mod.rs line 967 at r7 (raw file):
Previously, yuvalsw wrote…
clearer than before. My suggestion for more clarity:
Done.
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
a79d35a to
29d1d44
Compare
There was a problem hiding this comment.
Reviewable status: 10 of 14 files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
CHANGELOG.md
Outdated
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| #### Upcoming Changes | |||
|
|
|||
| * fix: Fix no trace padding flow in proof mode [#1909](https://github.com/lambdaclass/cairo-vm/pull/1909) | |||
There was a problem hiding this comment.
Done.
|
Previously, YairVaknin-starkware wrote…
Then, as discussed, maybe assert (in the test) that (something like) |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/cairo_run.rs line 38 at r9 (raw file):
pub secure_run: Option<bool>, // disable padding of the trace to accommodate the expected ratios/n_steps relationships w.r.t // the layout.
Please mention how. It doesn't pad steps at all, but does pad segments to a power of 2...
Suggestion:
// Disable padding of the trace to accommodate the expected ratios/n_steps relationships w.r.t
// the layout.
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
29d1d44 to
c9b8416
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):
Previously, yuvalsw wrote…
Then, as discussed, maybe assert (in the test) that (something like)
get_used_instances * cells_per_instance - get_used_cells < cells_per_instance.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/cairo_run.rs line 39 at r10 (raw file):
// Disable padding of the trace to accommodate the expected ratios-n_steps relationships w.r.t // the layout. It does, however, pad the number of builtin instances to the next power of 2 // compared to their values at the end of the execution, but doesn't modify n_steps.
It confuses with double negations. My suggestion:
Suggestion:
/// Disable padding of the trace.
/// By default, the trace is padded to accommodate the expected builtins-n_steps relationships according to
/// the layout.
/// When the padding is disabled:
/// - It doesn't modify/pad n_steps.
/// - It still pads each builtin segment to the next power of 2 (counting instances of the builtin) compared to their sizes at the end of the execution.vm/src/vm/runners/builtin_runner/mod.rs line 543 at r10 (raw file):
let n_output_cells = self.cells_per_instance() as usize - self.n_input_cells() as usize; assert!(
- Please doc what you explained to me f2f - about why it is not exactly equal.
- why not assert in a test?
- can the >= become >? Shouldn't there be at least one full output cell ? (not super important of course)
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
c9b8416 to
7c8c978
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/cairo_run.rs line 39 at r10 (raw file):
Previously, yuvalsw wrote…
It confuses with double negations. My suggestion:
Done.
vm/src/vm/runners/builtin_runner/mod.rs line 543 at r10 (raw file):
Previously, yuvalsw wrote…
- Please doc what you explained to me f2f - about why it is not exactly equal.
- why not assert in a test?
- can the >= become >? Shouldn't there be at least one full output cell ? (not super important of course)
- Done.
- thought it couldn't hurt, but moved it to the test and made it more robust (more programs/more used builtins).
3 . no, for range_check there's no input/output distinction.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/runners/builtin_runner/mod.rs line 985 at r11 (raw file):
// the number of used_cells might not be a multiple of cells_per_instance, so we // make sure that the discrepancy is up to the number of output cells. // This is the same for both cases, so we only check one (true).
IMO wouldn't hurt to test both. It should be the same for both, and this is exactly why we should test this hold for both.
Code quote:
so we only check one (true).
YairVaknin-starkware
force-pushed
the
yairv/fix_no_trace_padding_flow_in_proof_mode
branch
from
7c8c978 to
41987f5
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)
| /// - It still pads each builtin segment to the next power of 2 w.r.t the number of used | ||
| /// instances of the builtin) compared to their sizes at the end of the execution. |
There was a problem hiding this comment.
In line 43 there's only a closing parenthesis but not the opening one
TITLE
Description
Avoid using
safe_div_usizewhen disabling trace padding, and useceil_divinstead.Checklist
This change is