-
Notifications
You must be signed in to change notification settings - Fork 187
[BREAKING] Compute_missing_builtin_cells_only_in_proof_mode #2088
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
[BREAKING] Compute_missing_builtin_cells_only_in_proof_mode #2088
Conversation
This PR changes the firm of |
|
Benchmark Results for unmodified programs 🚀
|
8cd9b22
to
a56fb44
Compare
added to commit msg and PR name |
a56fb44
to
bddd477
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2088 +/- ##
=========================================================
- Coverage 96.62% 96.54% -0.08%
=========================================================
Files 102 102
Lines 44396 43224 -1172
=========================================================
- Hits 42897 41731 -1166
+ Misses 1499 1493 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bddd477
to
ec16c63
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 5 of 7 files at r1.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions
vm/src/cairo_run.rs
line 108 at r1 (raw file):
)?; cairo_runner.vm.verify_auto_deductions()?;
I also saw this extra verification, but I don't know if it's here for some reason, so I'm not sure about deleting it.
vm/src/vm/runners/cairo_runner.rs
line 3925 at r1 (raw file):
.expect("Call to `CairoRunner::run_until_pc()` failed."); assert_matches!( cairo_runner.end_run(false, false, &mut hint_processor, true),
can you do the same as in the next test and declaring proof_mode=true and use it in Cairo runner and end run ?
ec16c63
to
642ea1e
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: 4 of 7 files reviewed, 2 unresolved discussions (waiting on @Stavbe)
vm/src/cairo_run.rs
line 108 at r1 (raw file):
Previously, Stavbe wrote…
I also saw this extra verification, but I don't know if it's here for some reason, so I'm not sure about deleting it.
No reason to have it twice. It was also in other funcs that initiate a run, so removed there as well (PTAL).
vm/src/vm/runners/cairo_runner.rs
line 3925 at r1 (raw file):
Previously, Stavbe wrote…
can you do the same as in the next test and declaring proof_mode=true and use it in Cairo runner and end run ?
thanks, missed it.
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 r2.
Reviewable status: 5 of 7 files reviewed, all discussions resolved
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 7 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @YairVaknin-starkware)
b1a91f9
into
starkware-development
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is