-
Notifications
You must be signed in to change notification settings - Fork 623
Bump VM version to 3.0.0-rc.3. #8440
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
base: dev-v2.12.3
Are you sure you want to change the base?
Conversation
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 3 files reviewed, 1 unresolved discussion (waiting on @Stavbe and @YairVaknin-starkware)
a discussion (no related file):
Blocking this from being merge to the parent branch.
This branch is for development purpose, if the content of the branch is fine we'll point sharp-dev
to point here.
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 3 files reviewed, 3 unresolved discussions (waiting on @YairVaknin-starkware)
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2315 at r1 (raw file):
runner.run_until_pc(end, hint_processor).map_err(CairoRunError::from)?; runner.end_run(true, false, hint_processor, false).map_err(CairoRunError::from)?;
I see that disable_trace_padding = true => proof mode = true (otherwise there is nothing to disable), why filling holes = false?
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2316 at r1 (raw file):
runner.run_until_pc(end, hint_processor).map_err(CairoRunError::from)?; runner.end_run(true, false, hint_processor, false).map_err(CairoRunError::from)?; runner.relocate(true, true).map_err(CairoRunError::from)?;
What this function used for? Is it need the relocation of the memory and the trace by the VM.
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 3 files reviewed, 3 unresolved discussions (waiting on @Stavbe and @YairVaknin-starkware)
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2315 at r1 (raw file):
Previously, Stavbe wrote…
I see that disable_trace_padding = true => proof mode = true (otherwise there is nothing to disable), why filling holes = false?
Not sure, this is the state in sharp-dev
.
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2316 at r1 (raw file):
Previously, Stavbe wrote…
What this function used for? Is it need the relocation of the memory and the trace by the VM.
Same.
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 3 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @YairVaknin-starkware)
a discussion (no related file):
Previously, gilbens-starkware (Gil Ben-Shachar) wrote…
Blocking this from being merge to the parent branch.
This branch is for development purpose, if the content of the branch is fine we'll pointsharp-dev
to point here.
LGTM but please wait on someone from Sharp
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.
@Stavbe reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @YairVaknin-starkware)
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: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @YairVaknin-starkware)
a discussion (no related file):
Previously, Stavbe wrote…
LGTM but please wait on someone from Sharp
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: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @YairVaknin-starkware)
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2315 at r1 (raw file):
Previously, gilbens-starkware (Gil Ben-Shachar) wrote…
Not sure, this is the state in
sharp-dev
.
The separation between proof mode and filling holes is explained in the PR:
lambdaclass/cairo-vm#2165
But I agree it seems like it should be true if in proof mode (false seem to be needed only for debugging/testing)
Previously, yuvalsw wrote…
@gilbens-starkware / @Stavbe can you point to the entry point that uses this, which we use in SHARP? I'll try to check where it's used |
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: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @YairVaknin-starkware)
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2315 at r1 (raw file):
Previously, yuvalsw wrote…
@gilbens-starkware / @Stavbe can you point to the entry point that uses this, which we use in SHARP? I'll try to check where it's used
This is a question for sharper people
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: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)
crates/cairo-lang-runner/src/casm_run/mod.rs
line 2315 at r1 (raw file):
Previously, Stavbe wrote…
This is a question for sharper people
Who approved this design choice by them of seperating filling holes and proof mode? Was this someone on our end? We must communicate to them that the pythonic vm is deprecated and they should never change code introduced by us to adhere to it. Those memory comparisons they wanted to re-enable mean nothing.
crates/cairo-lang-runner/src/casm_run/mod.rs
line 471 at r1 (raw file):
_reference_ids: &HashMap<String, usize>, _references: &[HintReference], _constants: &[String],
This rev contains the addition of accessible_scopes
here and not _constants
. If we want SN to also use this rev we need both, I believe.
Anyway, should be renamed to accessible_scopes
.
Code quote:
_constants: &[String],
No description provided.