-
Notifications
You must be signed in to change notification settings - Fork 206
Stav/disable relocate trace with flag #2133
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
Stav/disable relocate trace with flag #2133
Conversation
2d7cfa6 to
29fc7dc
Compare
|
29fc7dc to
e0eec12
Compare
|
Benchmark Results for unmodified programs 🚀
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2133 +/- ##
=========================================================
- Coverage 96.63% 96.63% -0.01%
=========================================================
Files 104 104
Lines 43903 43915 +12
=========================================================
+ Hits 42427 42436 +9
- Misses 1476 1479 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
anatgstarkware
left a comment
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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Stavbe)
vm/src/cairo_run.rs line 31 at r1 (raw file):
pub trace_enabled: bool, pub relocate_mem: bool, pub relocate_trace: bool,
Please document
Code quote:
pub relocate_mem: bool,
pub relocate_trace: bool,e0eec12 to
b4a5c88
Compare
Stavbe
left a comment
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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @anatgstarkware)
vm/src/cairo_run.rs line 31 at r1 (raw file):
Previously, anatgstarkware (anatg) wrote…
Please document
Done.
anatgstarkware
left a comment
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: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @Stavbe)
vm/src/cairo_run.rs line 33 at r2 (raw file):
pub relocate_mem: bool, // Disable trace relocation. // By default, if trace is enabled, it is also relocated.
It's not clear if you refer to the default below or to the main default args. I would remove this line
Code quote:
By default, if trace is enabled, it is also relocatedvm/src/cairo_run.rs line 59 at r2 (raw file):
trace_enabled: false, relocate_mem: false, relocate_trace: true,
This should probably be false, as the rest of the fields here
Code quote:
relocate_trace: truevm/src/cairo_run.rs line 499 at r2 (raw file):
let end = cairo_runner.initialize(false).unwrap(); assert!(cairo_runner.run_until_pc(end, &mut hint_processor).is_ok()); assert!(cairo_runner.relocate(false, true).is_ok());
Since this test runs with no trace, it makes more sense to put here false
Code quote:
truecairo-vm-cli/src/main.rs line 195 at r2 (raw file):
// we need air public input or trace file. relocate_trace: trace_enabled && (args.trace_file.is_some() || args.air_public_input.is_some()),
I think this main is only used for stone, so maybe these should be true here, WDYT?
Code quote:
relocate_mem: args.memory_file.is_some() || args.air_public_input.is_some(),
// If trace not enabled, it is not relevant. Otherwise, we need to relocate the trace only if
// we need air public input or trace file.
relocate_trace: trace_enabled
&& (args.trace_file.is_some() || args.air_public_input.is_some()),b4a5c88 to
d7f56e9
Compare
Stavbe
left a comment
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: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @anatgstarkware)
cairo-vm-cli/src/main.rs line 195 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
I think this main is only used for stone, so maybe these should be true here, WDYT?
I think it’s also used to run the VM outside of any prover context, which is why prove_mode is an argument, along with the public and private inputs.
I agree that it definitely won’t be used by Stwo, so I think that the best approach is to set 'reloacte_trace' to match trace_enabled' and by that to turn off Stwo "feature" . As for the original relocate_mem` flag—I think it’s best not to touch it.
vm/src/cairo_run.rs line 33 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
It's not clear if you refer to the default below or to the main default args. I would remove this line
Done.
vm/src/cairo_run.rs line 59 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
This should probably be false, as the rest of the fields here
Done.
vm/src/cairo_run.rs line 499 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
Since this test runs with no trace, it makes more sense to put here false
Done.
55a5d43 to
4c4ec02
Compare
anatgstarkware
left a comment
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 2 files at r3, 2 of 6 files at r4.
Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @Stavbe)
cairo-vm-cli/src/main.rs line 195 at r2 (raw file):
Previously, Stavbe wrote…
I think it’s also used to run the VM outside of any prover context, which is why
prove_modeis an argument, along with the public and private inputs.
I agree that it definitely won’t be used by Stwo, so I think that the best approach is to set 'reloacte_trace' to matchtrace_enabled' and by that to turn off Stwo "feature" . As for the originalrelocate_mem` flag—I think it’s best not to touch it.
Didn't the vm relocate the trace also when prove_mode was false?
What do you mean by "original relocate_mem flag"? Was it in stone?
vm/src/cairo_run.rs line 59 at r2 (raw file):
Previously, Stavbe wrote…
Done.
I see you had to change the default in many tests, which means I was wrong. Are these tests only for stone? If so, maybe the default should be the same as in main (i.e., true, as you did before, or trace_enabled), WDYT?
Also, if relocated_mem was added for stwo, then maybe its default should also change.
4c4ec02 to
bfb6552
Compare
Stavbe
left a comment
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: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @anatgstarkware)
cairo-vm-cli/src/main.rs line 195 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
Didn't the vm relocate the trace also when
prove_modewas false?
What do you mean by "originalrelocate_memflag"? Was it in stone?
-
The trace was relocated when either
args.trace_file.is_some()orargs.air_public_input.is_some()
air_public_input requiresproof_mode = true
trace_file does not require it.
So in theory the trace could be relocated even whenproof_mode = falsebut I don't see why anyone would want that. -
Yes
vm/src/cairo_run.rs line 59 at r2 (raw file):
Previously, anatgstarkware (anatg) wrote…
I see you had to change the default in many tests, which means I was wrong. Are these tests only for stone? If so, maybe the default should be the same as in main (i.e., true, as you did before, or
trace_enabled), WDYT?
Also, ifrelocated_memwas added for stwo, then maybe its default should also change.
These tests are old, and they run the VM with the Stone configuration—so yes.
I think the best approach is to align it with trace_enabled, but you need to fix a value in the default configuration so it is not really an option. I changed it back to true so it won't affect the current VM expected behavior.
Relocate mem was not added for stwo
anatgstarkware
left a comment
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 4 files at r1, 1 of 2 files at r3, 2 of 7 files at r5, all commit messages.
Reviewable status: 4 of 9 files reviewed, all discussions resolved (waiting on @Stavbe)
cairo-vm-cli/src/main.rs line 195 at r2 (raw file):
Previously, Stavbe wrote…
The trace was relocated when either
args.trace_file.is_some()orargs.air_public_input.is_some()
air_public_input requiresproof_mode = true
trace_file does not require it.
So in theory the trace could be relocated even whenproof_mode = falsebut I don't see why anyone would want that.Yes
thanks
|
Previously, Stavbe wrote…
Can you please document why it was set to true? |
bfb6552 to
9d2256f
Compare
Up until now, whenever we wanted to keep the trace during execution, we also performed relocation at the end.
In the new adapter flow, we want to enable the option to keep the trace without allocating it.
I’ve set the default value to true to stay consistent with the current behavior.
This change is