feat: post compilation size limit validation

[ArniStarkware]

This PR is imported into this repo. The original PR: starkware-libs/mempool#298.

Note the stack of PRs has a different order in this repo.

---

This change is 

[ArniStarkware]

• feat: post compilation size limit validation #284 : 2 dependent PRs (#354 , #369 ) 👈
• chore(gateway): move compiler config to gateway component #374
• refactor(gateway): remove reduncdent config level on gateway compiler #373
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ArniStarkware and the rest of your teammates on Graphite

[Yael-Starkware]

Reviewed all commit messages.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 32 at r1 (raw file):

            sierra_to_casm_compiler: SierraToCasmCompiler {
                config: config.sierra_to_casm_compiler_config,
            },

why the duplication of sierra_to_casm_compiler_config?

Code quote:

            config,
            sierra_to_casm_compiler: SierraToCasmCompiler {
                config: config.sierra_to_casm_compiler_config,
            },

[Yael-Starkware]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/config.rs line 242 at r1 (raw file):

pub struct GatewayCompilerConfig {
    pub sierra_to_casm_compiler_config: SierraToCasmCompilationConfig,
    pub max_raw_casm_class_size: usize,

the word raw confuses me, I am think about eating steak.

Code quote:

raw

[Yael-Starkware]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/config.rs line 242 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

the word raw confuses me, I am think about eating steak.

*thinking

[ArniStarkware]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 32 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why the duplication of sierra_to_casm_compiler_config?

Done.
Now there is no duplication. - The price is the multiplicity of objects.

[dafnamatsry]

Reviewed 4 of 8 files at r1, 2 of 3 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 27 at r5 (raw file):

impl GatewayCompiler {
    pub fn new(config: GatewayCompilerConfig) -> Self {

In the following PR: https://reviewable.io/reviews/starkware-libs/sequencer/319#- you define different initialisation methods. Are you planning to delete this new later?

Code quote:

pub fn new(config: GatewayCompilerConfig)

---

crates/gateway/src/config.rs line 230 at r5 (raw file):

#[derive(Clone, Copy, Debug, Default, Serialize, Deserialize, Validate, PartialEq)]
pub struct GatewayCompilerConfig {
    pub sierra_to_casm_compiler_config: SierraToCasmCompilationConfig,

Maybe better to put it outside of GatewayCompilerConfig?
The SierraToCasmCompilation is given to the GatewayCompiler with a dependency injection (https://reviewable.io/reviews/starkware-libs/sequencer/319#-), so I think it makes sense that it would be outside.

Code quote:

pub sierra_to_casm_compiler_config: SierraToCasmCompilationConfig,

---

crates/gateway/src/config.rs line 251 at r5 (raw file):

/// The configuration for the post compilation process in the gateway compiler.
#[derive(Clone, Copy, Debug, Serialize, Deserialize, Validate, PartialEq)]
pub struct PostCompilationConfig {

Why do we need this extra level of config, and not just define max_raw_casm_class_size to the GatewayCompilerConfig?

Code quote:

PostCompilationConfig 

[ArniStarkware]

This PR will be restacked over #357.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.961 ms 65.046 ms 65.141 ms]
change: [-10.061% -6.9538% -4.2677%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

[ArniStarkware]

Reviewable status: 1 of 9 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 27 at r5 (raw file):

Previously, dafnamatsry wrote…

In the following PR: https://reviewable.io/reviews/starkware-libs/sequencer/319#- you define different initialisation methods. Are you planning to delete this new later?

Deleted. Replaced with the existing new_cario_lang_compiler function.

Rebases made this comment had to follow.

---

crates/gateway/src/config.rs line 230 at r5 (raw file):

Previously, dafnamatsry wrote…

Maybe better to put it outside of GatewayCompilerConfig?
The SierraToCasmCompilation is given to the GatewayCompiler with a dependency injection (https://reviewable.io/reviews/starkware-libs/sequencer/319#-), so I think it makes sense that it would be outside.

Done. See previous PR in the stack. #374.

Now: GatewayCompilerConfig sits directly in the mempool node config.

---

crates/gateway/src/config.rs line 251 at r5 (raw file):

Previously, dafnamatsry wrote…

Why do we need this extra level of config, and not just define max_raw_casm_class_size to the GatewayCompilerConfig?

I think it's cleaner that there is a level with all sub-configs. But it is a taste thing.
I plan to also have other similar numbers in this PostCompilerConfig level - like the size of the casm-bytecode.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.458 ms 64.513 ms 64.578 ms]
change: [-3.6139% -2.6079% -1.7130%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

[codecov]

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.13%. Comparing base (e6fcc96) to head (ff67b95).

Files	Patch %	Lines
crates/gateway/src/config.rs	92.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           graphite-base/284     #284   +/-   ##
==================================================
  Coverage              76.12%   76.13%           
==================================================
  Files                    329      329           
  Lines                  35241    35288   +47     
  Branches               35241    35288   +47     
==================================================
+ Hits                   26826    26865   +39     
- Misses                  6119     6128    +9     
+ Partials                2296     2295    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dafnamatsry]

Reviewed 5 of 10 files at r6, all commit messages.
Reviewable status: 6 of 11 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 22 at r6 (raw file):

#[derive(Clone)]
pub struct GatewayCompiler {
    config: PostCompilationConfig,

Suggestion:

config: GatewayCompilerConfig,

---

crates/gateway/src/compilation.rs line 27 at r6 (raw file):

impl GatewayCompiler {
    pub fn new_cairo_lang_compiler(config: GatewayCompilerConfig) -> Self {

Once you remove the SierraToCasmCompilationConfig from the GatewayConfig you can pass it here as an argument.

---

crates/gateway/src/config.rs line 242 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Addressed in #354 (And #355).

Not critical but: for the future, I prefer you address this kind of comments in the same PR.
This field was introduced in this PR, so merging it and immediately renaming it in a following PR feels like extra work that could have been avoided.

---

crates/gateway/src/config.rs line 230 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done. See previous PR in the stack. #374.

Now: GatewayCompilerConfig sits directly in the mempool node config.

With the previous PR the SierraToCasmCompilationConfig sits directly in the mempool node config. So you can remove it from here no?

---

crates/gateway/src/config.rs line 251 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I think it's cleaner that there is a level with all sub-configs. But it is a taste thing.
I plan to also have other similar numbers in this PostCompilerConfig level - like the size of the casm-bytecode.

Ok.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.602 ms 64.689 ms 64.785 ms]
change: [-5.2926% -4.0285% -2.8571%] (p = 0.00 < 0.05)
Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
7 (7.00%) high mild
5 (5.00%) high severe

[ArniStarkware]

Reviewable status: 5 of 11 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 22 at r6 (raw file):

#[derive(Clone)]
pub struct GatewayCompiler {
    config: PostCompilationConfig,

The struct called GatewayCompilerConfig also holds the config of the SierraToCasmCompiler. There will be duplication - The GatewayCompiler will have access to unused info - which is used by the SierraToCasmCompiler.

Are you sure?

---

crates/gateway/src/config.rs line 230 at r5 (raw file):

Previously, dafnamatsry wrote…

With the previous PR the SierraToCasmCompilationConfig sits directly in the mempool node config. So you can remove it from here no?

Got it.
Do we want to do something similar with the RpcStateReaderConfig.

[dafnamatsry]

Reviewed 3 of 10 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 22 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The struct called GatewayCompilerConfig also holds the config of the SierraToCasmCompiler. There will be duplication - The GatewayCompiler will have access to unused info - which is used by the SierraToCasmCompiler.

Are you sure?

I think GatewayCompilerConfig shouldn't hold the SierraToCasmCompilerConfig.

[ArniStarkware]

A lot of requests regarding this PR. Please Do not review it until I re-publish it.