feat: use the starknet-sierra-compile downloaded binary to compile

[ArniStarkware]

This change is 

[ArniStarkware]

• feat: use the command line compiler in gateway #599
• feat: use the starknet-sierra-compile downloaded binary to compile #598 👈
• feat: use the build script to download the cairo crate from the starkware release page #597
• chore: streamline the error types in starknet_sierra_compile crate #566 : 2 other dependent PRs (#331 , #581 )
• main

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

Join @ArniStarkware and the rest of your teammates on Graphite

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [46.834 ms 46.879 ms 46.931 ms]
change: [-2.9864% -2.0496% -1.4993%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.479 ms 65.571 ms 65.676 ms]
change: [-7.8989% -4.5978% -1.8157%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 12 lines in your changes missing coverage. Please review.

Project coverage is 76.63%. Comparing base (d1aa365) to head (f3ecaa2).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...arknet_sierra_compile/src/command_line_compiler.rs	79.31%	0 Missing and 6 partials ⚠️
crates/starknet_sierra_compile/src/errors.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #598   +/-   ##
=======================================
  Coverage   76.62%   76.63%           
=======================================
  Files         351      353    +2     
  Lines       37223    37273   +50     
  Branches    37223    37273   +50     
=======================================
+ Hits        28522    28564   +42     
- Misses       6388     6390    +2     
- Partials     2313     2319    +6     

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

[dafnamatsry]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 37 at r1 (raw file):

        let mut temp_file = NamedTempFile::new()?;
        temp_file.write_all(serialized_contract_class.as_bytes())?;
        let temp_file_path = temp_file.path().to_str().ok_or(

Shouldn't it be before write_all?

Code quote:

let temp_file_path = temp_file.path()

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 38 at r1 (raw file):

        temp_file.write_all(serialized_contract_class.as_bytes())?;
        let temp_file_path = temp_file.path().to_str().ok_or(
            CompilationUtilError::CompilationError("Failed to get temporary file path".to_owned()),

UnexpectedError?

Code quote:

CompilationUtilError::CompilationError

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 74 at r1 (raw file):

            .nth(3)
            .expect("Failed to navigate up three levels from OUT_DIR")
            .join("cairo");

Consider moving to a common location, as it is used in the build.rs as well.

Code quote:

        let out_dir = env::var("OUT_DIR").expect("Failed to get the OUT_DIR environment variable");
        let out_dir = Path::new(&out_dir);

        // Navigate up the directory tree until reaching the 'target' directory
        let target_dir = out_dir
            .ancestors()
            .nth(3)
            .expect("Failed to navigate up three levels from OUT_DIR")
            .join("cairo");

---

crates/starknet_sierra_compile/src/compile_test.rs line 26 at r1 (raw file):

    COMMAND_LINE_COMPILER
        .get_or_init(|| CommandLineCompiler::new(SIERRA_TO_CASM_COMPILATION_CONFIG))
}

Please make these a simple functions.
Seems to me like an overkill to use OneLock in the test.

Code quote:

const CAIRO_LANG_COMPILER: CairoLangSierraToCasmCompiler =
    CairoLangSierraToCasmCompiler { config: SIERRA_TO_CASM_COMPILATION_CONFIG };

static COMMAND_LINE_COMPILER: OnceLock<CommandLineCompiler> = OnceLock::new();
fn commnad_line_compiler() -> &'static CommandLineCompiler {
    COMMAND_LINE_COMPILER
        .get_or_init(|| CommandLineCompiler::new(SIERRA_TO_CASM_COMPILATION_CONFIG))
}

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.028 ms 65.152 ms 65.311 ms]
change: [-10.679% -7.2807% -4.1233%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe

[ArniStarkware]

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry)

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 37 at r1 (raw file):

Previously, dafnamatsry wrote…

Shouldn't it be before write_all?

Tried replacing the line: got:

error[E0502]: cannot borrow `temp_file` as mutable because it is also borrowed as immutable

Does not seem critical IIUC.

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 38 at r1 (raw file):

Previously, dafnamatsry wrote…

UnexpectedError?

Done.

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 74 at r1 (raw file):

Previously, dafnamatsry wrote…

Consider moving to a common location, as it is used in the build.rs as well.

Done.

---

crates/starknet_sierra_compile/src/compile_test.rs line 26 at r1 (raw file):

Previously, dafnamatsry wrote…

Please make these a simple functions.
Seems to me like an overkill to use OneLock in the test.

Done.

[dafnamatsry]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_sierra_compile/src/build_utils.rs line 3 at r2 (raw file):

/// Get the crate's `OUT_DIR` and navigates up to reach the `target/BUILD_FLAVOR` directory.
/// This directory is shared accross all crates in this project.
pub fn get_traget_build_flavor_dir() -> &'static std::path::Path {

Why static?
I think the caller should be responsible of the lifetime of this.

Is it possible to return std::path::Path, and remove the Box::leak?

Code quote:

&'static std::path::Path {

---

crates/starknet_sierra_compile/src/compile_test.rs line 19 at r2 (raw file):

const CAIRO_LANG_COMPILER: CairoLangSierraToCasmCompiler =
    CairoLangSierraToCasmCompiler { config: SIERRA_TO_CASM_COMPILATION_CONFIG };

Just for consistency, consider changing this to a function as well (like the command_line_compiler)

Code quote:

const CAIRO_LANG_COMPILER: CairoLangSierraToCasmCompiler =
    CairoLangSierraToCasmCompiler { config: SIERRA_TO_CASM_COMPILATION_CONFIG };

[ArniStarkware]

crates/starknet_sierra_compile/src/command_line_compiler.rs line 47 at r2 (raw file):

            "--max-bytecode-size",
            &self.config.max_bytecode_size.to_string(),
        ]);

add todo - add Ulimits.

Code quote:

        // Set the parameters for the compile process.
        let mut command = Command::new(self.path_to_starknet_sierra_compile_binary);
        command.args([
            temp_file_path,
            "--add-pythonic-hints",
            "--max-bytecode-size",
            &self.config.max_bytecode_size.to_string(),
        ]);

[ArniStarkware]

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry)

---

crates/starknet_sierra_compile/src/build_utils.rs line 3 at r2 (raw file):

Previously, dafnamatsry wrote…

Why static?
I think the caller should be responsible of the lifetime of this.

Is it possible to return std::path::Path, and remove the Box::leak?

Done.

---

crates/starknet_sierra_compile/src/compile_test.rs line 19 at r2 (raw file):

Previously, dafnamatsry wrote…

Just for consistency, consider changing this to a function as well (like the command_line_compiler)

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.585 ms 65.670 ms 65.765 ms]
change: [-8.0656% -4.9401% -2.2831%] (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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.407 ms 66.487 ms 66.575 ms]
change: [-8.7426% -5.3266% -2.3175%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
1 (1.00%) high mild
3 (3.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.440 ms 65.510 ms 65.592 ms]
change: [-10.128% -6.7349% -3.7192%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

[dafnamatsry]

Reviewed 6 of 6 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.962 ms 66.256 ms 66.601 ms]
change: [-8.4867% -4.9162% -1.9202%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe

[dafnamatsry]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ArniStarkware]

Merge activity

• Sep 3, 2:07 AM EDT: @ArniStarkware started a stack merge that includes this pull request via Graphite.
• Sep 3, 2:10 AM EDT: Graphite rebased this pull request as part of a merge.
• Sep 3, 2:19 AM EDT: @ArniStarkware merged this pull request with Graphite.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.532 ms 65.604 ms 65.689 ms]
change: [-9.5255% -6.1694% -3.1979%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

[shaharsamocha7]

This pr stack is for taking the sierra binary instead of compiling it, right?
Why do you want that? whouldn't it be more safe if this code compiles sierra binary by itself?

[shaharsamocha7]

crates/starknet_sierra_compile/src/build_utils.rs line 6 at r5 (raw file):

fn out_dir() -> PathBuf {
    Path::new(&std::env::var("OUT_DIR").expect("Failed to get the OUT_DIR environment variable"))

If this library gets compiled into a binary won't this always fail since OUT_DIR is only set in the context of a cargo command? Is this an issue?

[shaharsamocha7]

crates/starknet_sierra_compile/src/errors.rs line 25 at r5 (raw file):

}

impl From<serde_json::Error> for CompilationUtilError {

Why both of these errors are wrapped into the same error?
Wouldn't you want different error types?

Code quote:

impl From<serde_json::Error> for CompilationUtilError {

[ArniStarkware]

Previously, shaharsamocha7 wrote…

This pr stack is for taking the sierra binary instead of compiling it, right?
Why do you want that? whouldn't it be more safe if this code compiles sierra binary by itself?

This PR stack uses the starknet_sierra_compile binary to compile Sierra -> Casm instead of using the CairoLang crate as a gateway dependency.

Why do we want that:

1. the compiler crate does not intend to be used as a dependency. There is no guarantee that the code will not panic or how much resources the process will take. This is the issue with the previous solution (with CairoLangSierraToCasmCompiler replaced in the following PR in this stack (feat: use the command line compiler in gateway #599).
2. With this solution - we import the binary as a cargo dependency. We then run the binary as a process this way - at some point, we can better control the resources the compilation process takes (see TODO(Arni): Setup the ulimit for the process.) and any panic will be handled gracefully.
   a. One more upside of using the cargo install command is that it is OS-agnostic. If this code is run on a MAC, the correct binary will be downloaded.

Did that answer the question? Because I am afraid we do not use the same terminology (What does "sierra binary" mean?

• the sierra program to be compiled during the "process tx" flow for a declare transaction?
• or the binary that compiled sierra to casm?
  )

[ArniStarkware]

crates/starknet_sierra_compile/src/build_utils.rs line 6 at r5 (raw file):

Previously, shaharsamocha7 wrote…

If this library gets compiled into a binary won't this always fail since OUT_DIR is only set in the context of a cargo command? Is this an issue?

I am not sure.
We want to run this library as a separate microservice at some point. If this indeed requires us to compile this lib into a bin, what you pointed out is indeed a problem.

[ArniStarkware]

crates/starknet_sierra_compile/src/errors.rs line 25 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Why both of these errors are wrapped into the same error?
Wouldn't you want different error types?

These errors should never happen (Thus, they are converted into UnexpectedError). I do not intend to handle these errors differently than other UnexpectedErrors.

My motivation was "each error type should be mapped to some handle of this error".
and "unexpected error points to a bug in the code - it should be handled gracefully but the bug should be fixed".