Add_support_for_pies_with_simulated_builtins #2069
Conversation
|
Benchmark Results for unmodified programs 🚀
|
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_pie.rs line 151 at r1 (raw file):
pub extra_segments: Vec<SegmentInfo>, #[serde(default)] pub simulated_builtins: Vec<BuiltinName>,
doc - what are simulated builtins
vm/src/vm/runners/cairo_runner.rs line 1418 at r1 (raw file):
.map(|builtin| builtin.name()) .collect::<Vec<_>>(); // Sort the simulated builtins by their order in the program.
why? (doc the answer)
vm/src/cairo_run.rs line 185 at r1 (raw file):
cairo_runner.vm.finalize_segments_by_cairo_pie(pie); // Load builtin additional data for builtin_name in pie.metadata.simulated_builtins.iter() {
sorry, missed it, where are these added to the PIE?
Code quote:
pie.metadata.simulated_builtinsvm/src/cairo_run.rs line 203 at r1 (raw file):
.vm .simulated_builtin_runners .push(EcOpBuiltinRunner::new(Some(1), false).into());
if included is false, why isn't ratio None?
Code quote:
.push(SignatureBuiltinRunner::new(Some(1), false).into());
}
BuiltinName::keccak => {
cairo_runner
.vm
.simulated_builtin_runners
.push(KeccakBuiltinRunner::new(Some(1), false).into());
}
BuiltinName::ec_op => {
cairo_runner
.vm
.simulated_builtin_runners
.push(EcOpBuiltinRunner::new(Some(1), false).into());
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @YairVaknin-starkware)
a discussion (no related file):
I'd like Titelman's review on it too
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 1435 at r1 (raw file):
builtin_segments, extra_segments, simulated_builtins,
Added here to the PIE (answering above question).
Code quote:
simulated_builtins,There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yuvalsw)
vm/src/cairo_run.rs line 185 at r1 (raw file):
Previously, yuvalsw wrote…
sorry, missed it, where are these added to the PIE?
See other comment answering this.
vm/src/cairo_run.rs line 203 at r1 (raw file):
Previously, yuvalsw wrote…
if included is false, why isn't ratio None?
No particular reason. They won't be used. It's just there to align the output PIE if we load a PIE with simulated builtins.
YairVaknin-starkware
force-pushed
the
yairv/fix_zero_offset_output_base_assumption
branch
from
e89c602 to
7646a7e
Compare
YairVaknin-starkware
force-pushed
the
yairv/add_support_for_pies_with_simulated_builtins
branch
from
9dc6889 to
172c65d
Compare
|
YairVaknin-starkware
force-pushed
the
yairv/add_support_for_pies_with_simulated_builtins
branch
from
172c65d to
51ad4f0
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yuvalsw)
vm/src/vm/runners/cairo_pie.rs line 151 at r1 (raw file):
Previously, yuvalsw wrote…
doc - what are simulated builtins
Done.
vm/src/vm/runners/cairo_runner.rs line 1418 at r1 (raw file):
Previously, yuvalsw wrote…
why? (doc the answer)
Done.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2069 +/- ##
=========================================================
- Coverage 96.63% 96.56% -0.07%
=========================================================
Files 102 102
Lines 44374 43273 -1101
=========================================================
- Hits 42881 41787 -1094
+ Misses 1493 1486 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
YairVaknin-starkware
force-pushed
the
yairv/add_support_for_pies_with_simulated_builtins
branch
3 times, most recently
from
fa018bf to
b4f5774
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/cairo_run.rs line 203 at r1 (raw file):
Previously, YairVaknin-starkware wrote…
No particular reason. They won't be used. It's just there to align the output PIE if we load a PIE with simulated builtins.
I don't follow, can you elaborate more? Align what exactly in the pie, and to what?
If this alignment is important somehow, I am ok with this, but maybe doc here why Some. If not, I think it makes it much easier to read is you use None as the reader doesn't have to wonder why it's Some, like I did.
vm/src/vm/errors/cairo_run_errors.rs line 30 at r3 (raw file):
#[error(transparent)] CairoPieValidation(#[from] CairoPieValidationError), #[error("Unsupported simulated builtin in Cairo PIE: {0:?}")]
I see it impls display
Suggestion:
: {0}vm/src/tests/cairo_pie_test.rs line 315 at r3 (raw file):
let runner = result.unwrap(); let result = runner.get_cairo_pie(); assert!(result.is_ok());
Suggestion:
let runner = cairo_run::cairo_run_pie(&pie, &cairo_run_config, &mut hint_processor).unwrap();
assert!(runner.get_cairo_pie().is_ok());vm/src/vm/runners/cairo_pie.rs line 153 at r2 (raw file):
// rather than being outputted as their own segment and proven later (see example implementation // of this mechanism in `simple_bootloader.cairo`). #[serde(default, skip_serializing_if = "Vec::is_empty")]
Suggestion:
/// `simulated_builtins` are builtins that are being verified in the executed cairo code
/// rather than being outputted as their own segment and verified later (see example implementation
/// of this mechanism in `simple_bootloader.cairo`).
#[serde(default, skip_serializing_if = "Vec::is_empty")]vm/src/vm/runners/cairo_runner.rs line 5822 at r3 (raw file):
} _ => panic!("Expected unsupported simulated builtin error."), }
Doesn't this work?
Suggestion:
assert_matches!(result, Err(CairoRunError::UnsupportedSimulatedBuiltin(BuiltinName::pedersen)));
YairVaknin-starkware
force-pushed
the
yairv/add_support_for_pies_with_simulated_builtins
branch
from
b4f5774 to
d89ff6e
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yuvalsw)
vm/src/cairo_run.rs line 203 at r1 (raw file):
Previously, yuvalsw wrote…
I don't follow, can you elaborate more? Align what exactly in the pie, and to what?
If this alignment is important somehow, I am ok with this, but maybe doc here why Some. If not, I think it makes it much easier to read is you use None as the reader doesn't have to wonder why it's Some, like I did.
I mean that it'd work either way, but followed the practice in other places in code (initialize_program_builtins for example) when initializing runners with ratios that don't matter. added explanation to the doc.
If they won't be present the resulting PIE won't have them in the metadata (since the program inits them in hints when this mechanism's invoked).
vm/src/vm/errors/cairo_run_errors.rs line 30 at r3 (raw file):
Previously, yuvalsw wrote…
I see it impls display
Done.
vm/src/vm/runners/cairo_pie.rs line 153 at r2 (raw file):
// rather than being outputted as their own segment and proven later (see example implementation // of this mechanism in `simple_bootloader.cairo`). #[serde(default, skip_serializing_if = "Vec::is_empty")]
revised
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @YairVaknin-starkware)
vm/src/vm/runners/cairo_runner.rs line 1566 at r4 (raw file):
/// Loads builtin runners for simulated builtins in the pie's metadata, so the resulted pie is /// compatible with the one received. Note: they won't be used during the execution, and are
Great! This was indeed missing and is now much clearer.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)
a discussion (no related file):
Previously, yuvalsw wrote…
I'd like Titelman's review on it too
What's the status of this?
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is