-
Notifications
You must be signed in to change notification settings - Fork 28
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
chore: expose max bytecode size arg for the compile sierra to casm util #283
chore: expose max bytecode size arg for the compile sierra to casm util #283
Conversation
Benchmark movements: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
==========================================
- Coverage 76.72% 76.71% -0.01%
==========================================
Files 319 320 +1
Lines 34642 34641 -1
Branches 34642 34641 -1
==========================================
- Hits 26578 26575 -3
- Misses 5775 5778 +3
+ Partials 2289 2288 -1 ☔ View full report in Codecov by Sentry. |
56b4de1
to
56c45e8
Compare
92f1dc5
to
f44f1cf
Compare
56c45e8
to
e47382b
Compare
f44f1cf
to
f54a4f5
Compare
e47382b
to
d52a40c
Compare
f54a4f5
to
b064637
Compare
d52a40c
to
0db4ea7
Compare
b064637
to
db6c928
Compare
0db4ea7
to
6b8de0d
Compare
db6c928
to
bd04a7e
Compare
This PR is very similar to: starkware-libs/mempool#348 |
bd04a7e
to
c55f0be
Compare
Benchmark movements: |
c55f0be
to
79ec169
Compare
Benchmark movements: |
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 8 of 15 files at r1, 4 of 6 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/config.rs
line 95 at r3 (raw file):
max_calldata_length: 4000, max_signature_length: 4000, max_bytecode_size: MAX_BYTECODE_SIZE,
Do we need to keep this validation in the stateless validator?
Code quote:
max_bytecode_size: MAX_BYTECODE_SIZE,
crates/starknet_sierra_compile/Cargo.toml
line 15 at r3 (raw file):
cairo-lang-starknet-classes.workspace = true cairo-lang-utils.workspace = true papyrus_config = { path = "../papyrus_config", version = "0.4.0-rc.0" }
Note that local crate deps have moved to the workspace.
Code quote:
papyrus_config = { path = "../papyrus_config", version = "0.4.0-rc.0" }
crates/starknet_sierra_compile/src/compile.rs
line 18 at r3 (raw file):
impl SierraToCasmCompiler { pub fn compile_sierra_to_casm(
Now that the struct name is SierraToCasmCompiler
...
Suggestion:
compile
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: 11 of 16 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/config.rs
line 95 at r3 (raw file):
Previously, dafnamatsry wrote…
Do we need to keep this validation in the stateless validator?
Yes. The stateless validator validates the size of the Sierra program.
The compilation validates the size of the Casm program.
crates/starknet_sierra_compile/Cargo.toml
line 15 at r3 (raw file):
Previously, dafnamatsry wrote…
Note that local crate deps have moved to the workspace.
Done.
crates/starknet_sierra_compile/src/compile.rs
line 18 at r3 (raw file):
Previously, dafnamatsry wrote…
Now that the struct name is
SierraToCasmCompiler
...
Done.
Benchmark movements: |
8bf1bdf
to
98c3439
Compare
Previously, ArniStarkware (Arnon Hod) wrote…
Sorry - I am mistaken. This PR indeed handles the I still would like to have this validation in the lightweight - stateless validator, as early as possible. |
Benchmark movements: |
98c3439
to
74dd64e
Compare
Benchmark movements: |
74dd64e
to
a0d234f
Compare
Benchmark movements: |
a0d234f
to
734a4b7
Compare
Benchmark movements: |
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 6 files at r4.
Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/config.rs
line 95 at r3 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Sorry - I am mistaken. This PR indeed handles the
max_bytecode_size
flag for the compiler which is relevant for the size of the Sierra.I still would like to have this validation in the lightweight - stateless validator, as early as possible.
I will add a comment documenting this.
Discussed f2f. I think we can remove this from the stateless validator.
734a4b7
to
ae56362
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: 13 of 19 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/config.rs
line 95 at r3 (raw file):
Previously, dafnamatsry wrote…
Discussed f2f. I think we can remove this from the stateless validator.
Done.
Benchmark movements: |
ae56362
to
cb4fef6
Compare
Benchmark movements: |
cb4fef6
to
59228b7
Compare
Benchmark movements: |
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 1 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware and @yair-starkware)
Merge activity
|
This is an imported version of the PR: starkware-libs/mempool#348.
Note the PRs are stacked in a different order in this REPO.
This change is