feat(blockifier): add support for native testing

[rodrigo-pino]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 20.51282% with 31 lines in your changes missing coverage. Please review.

Project coverage is 60.74%. Comparing base (e3165c4) to head (d3ba32a).
Report is 247 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/test_utils/cairo_compile.rs	0.00%	31 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1301       +/-   ##
===========================================
+ Coverage   40.10%   60.74%   +20.64%     
===========================================
  Files          26      292      +266     
  Lines        1895    33969    +32074     
  Branches     1895    33969    +32074     
===========================================
+ Hits          760    20635    +19875     
- Misses       1100    11807    +10707     
- Partials       35     1527     +1492     

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

[meship-starkware]

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

        two_counters: starknet::storage::Map<felt252, (felt252, felt252)>,
        ec_point: (felt252, felt252),
    }

This contract is almost identical to the test contract of the Casm contract class. Can we add the new functions to the existing contract to avoid duplication?

[meship-starkware]

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rodrigo-pino)

---

crates/blockifier/src/test_utils.rs line 63 at r4 (raw file):

    Cairo0,
    Cairo1,
    Native,

Why did you add Native? Isn't the Native Cairo version Cairo1? Maybe we should change the Cario1 to Casm? This is a bit confusing because this enum no longer represents the Cairo version. I think we should create a new enum for the different compilations WDYT?

Code quote:

    Cairo0,
    Cairo1,
    Native,

---

crates/blockifier/src/test_utils/cairo_compile.rs line 151 at r4 (raw file):

    git_tag_override: Option<String>,
    cargo_nightly_arg: Option<String>,
) -> Vec<u8> {

Can you make cairo1_compile use this function to avoid this duplication

Code quote:

pub fn sierra_compile(
    path: String,
    git_tag_override: Option<String>,
    cargo_nightly_arg: Option<String>,
) -> Vec<u8> {

---

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 68 at r4 (raw file):

        CairoVersion::Cairo0 => CAIRO0_FEATURE_CONTRACTS_DIR,
        CairoVersion::Cairo1 => CAIRO1_FEATURE_CONTRACTS_DIR,
        CairoVersion::Native => NATIVE_FEATURE_CONTRACTS_DIR,

I can't entirely agree with this. Native contracts are still Cairo1 contracts. I don't see why they need a new directory.

Code quote:

CairoVersion::Native => NATIVE_FEATURE_CONTRACTS_DIR,

[rodrigo-pino]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @varex83)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This contract is almost identical to the test contract of the Casm contract class. Can we add the new functions to the existing contract to avoid duplication?

It is meant to be the same. Duplication is not intentional but the easiest way to integrate it into your current project structure.

Not doing it like this requires some greater changes in your testing architecture that span more creates than just the blockifier. We wanted to discuss this with you in the meantime.

@varex83 could you please add a comment explaining the problem in more detail.

---

crates/blockifier/src/test_utils.rs line 63 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why did you add Native? Isn't the Native Cairo version Cairo1? Maybe we should change the Cario1 to Casm? This is a bit confusing because this enum no longer represents the Cairo version. I think we should create a new enum for the different compilations WDYT?

I personally agree. I also think it should take it's own PR because even if very simple a lot of code is going to be changed as a result which might make it deserving.

---

crates/blockifier/src/test_utils/cairo_compile.rs line 151 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Can you make cairo1_compile use this function to avoid this duplication

Yes, @varex83 could you please address this.

---

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 68 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I can't entirely agree with this. Native contracts are still Cairo1 contracts. I don't see why they need a new directory.

I don't agree either. It's a matter of compatibility and an issue that spans other crates beyond the blockifier.

We don't want to risk fixing it the "right" way for us fearing it might be too opinionated and you guys understand your project better than us.

[varex83]

Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @meship-starkware, @rodrigo-pino, and @varex83)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

It is meant to be the same. Duplication is not intentional but the easiest way to integrate it into your current project structure.

Not doing it like this requires some greater changes in your testing architecture that span more creates than just the blockifier. We wanted to discuss this with you in the meantime.

@varex83 could you please add a comment explaining the problem in more detail.

I agree that duplication in projects is the number one thing to avoid. We made a PR in that way because it integrates into your codebase in the best way. So, if we put these "Native" contracts into the "cairo1" folder, we will need to generate all other artifacts for contracts not being used for native testing. I've managed to fix it in our fork, but we still made a PR with previous changes since we weren't sure about what solution you'd prefer, please take a look into this PR, to see what I mean: NethermindEth#29

---

crates/blockifier/src/test_utils.rs line 63 at r4 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

I personally agree. I also think it should take it's own PR because even if very simple a lot of code is going to be changed as a result which might make it deserving.

Yes, I agree with Rodrigo, these changes are better to make in the separate PR, they will be small, and easy to review, so we can refactor without any conflicts.

I propose to rename Cairo1 -> Casm and Native -> Sierra

---

crates/blockifier/src/test_utils/cairo_compile.rs line 151 at r4 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

Yes, @varex83 could you please address this.

Will do

---

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 68 at r4 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

I don't agree either. It's a matter of compatibility and an issue that spans other crates beyond the blockifier.

We don't want to risk fixing it the "right" way for us fearing it might be too opinionated and you guys understand your project better than us.

Answered above.

[meship-starkware]

Reviewed 4 of 7 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @varex83)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

Previously, varex83 (Bohdan Ohorodnii) wrote…

I agree that duplication in projects is the number one thing to avoid. We made a PR in that way because it integrates into your codebase in the best way. So, if we put these "Native" contracts into the "cairo1" folder, we will need to generate all other artifacts for contracts not being used for native testing. I've managed to fix it in our fork, but we still made a PR with previous changes since we weren't sure about what solution you'd prefer, please take a look into this PR, to see what I mean: NethermindEth#29

I liked the second solution with the subdirectories @noaov1 @avi-starkware WDYT?

---

crates/blockifier/src/test_utils.rs line 63 at r4 (raw file):

Previously, varex83 (Bohdan Ohorodnii) wrote…

Yes, I agree with Rodrigo, these changes are better to make in the separate PR, they will be small, and easy to review, so we can refactor without any conflicts.

I propose to rename Cairo1 -> Casm and Native -> Sierra

I agree it should be in a separate PR.

[rodrigo-pino]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @varex83)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I liked the second solution with the subdirectories @noaov1 @avi-starkware WDYT?

Discussed this with Noa. We agreed to allow the duplication in this instance and then change the infrastructure to a solution without it (such as individual subdirs for compilation artifacts).

This will allow us to continue working on the issue while advancing on the Syscall PRs as well

[rodrigo-pino]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @varex83)

---

crates/blockifier/feature_contracts/cairo_native/test_contract.cairo line 35 at r4 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

Discussed this with Noa. We agreed to allow the duplication in this instance and then change the infrastructure to a solution without it (such as individual subdirs for compilation artifacts).

This will allow us to continue working on the issue while advancing on the Syscall PRs as well

It was not my intention to resolve this conversation 🤦

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 2 of 2 files at r50, 1 of 8 files at r53, 3 of 9 files at r54, all commit messages.
Reviewable status: 13 of 24 files reviewed, 12 unresolved discussions (waiting on @avi-starkware, @noaov1, @rodrigo-pino, and @varex83)

[noaov1]

Reviewed 2 of 2 files at r50, 1 of 1 files at r52, 1 of 8 files at r53, 2 of 9 files at r54, all commit messages.
Reviewable status: 14 of 24 files reviewed, 13 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @rodrigo-pino, and @varex83)

---

crates/starknet_api/src/contract_class.rs line 32 at r54 (raw file):

    V1(CasmContractClass),
    #[cfg(feature = "cairo_native")]
    V1Native(SierraContractClass),

Please remove.
We will get the sierra later from the DB, and not via declare transaction.
Is removing it problematic?

Code quote:

    #[cfg(feature = "cairo_native")]
    V1Native(SierraContractClass),

[rodrigo-pino]

Reviewable status: 14 of 24 files reviewed, 13 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/contracts.rs line 256 at r48 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why? Do you mean currently?

ERC20 only exists in Cairo Zero

---

crates/starknet_api/src/contract_class.rs line 32 at r54 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove.
We will get the sierra later from the DB, and not via declare transaction.
Is removing it problematic?

Yes, it is. This was what I mentioned about being problematic. We have to put there some definition NativeContractClass, I am not sure what does that NativeContractClass should have or what is enough. I'll think on what's best to put there

[meship-starkware]

Reviewable status: 14 of 24 files reviewed, 13 unresolved discussions (waiting on @avi-starkware, @noaov1, @rodrigo-pino, and @varex83)

---

crates/blockifier/src/test_utils/contracts.rs line 256 at r48 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

ERC20 only exists in Cairo Zero

There is an ERC20 contract for Cairo1 as well.

sequencer/crates/blockifier/ERC20/ERC20_Cairo1/ERC20_cairo1.cairo

Line 16 in 35f5081

use integer::BoundedInt;

[avi-starkware]

Reviewed 11 of 11 files at r49, 1 of 8 files at r53, 1 of 9 files at r54.
Reviewable status: 16 of 24 files reviewed, 13 unresolved discussions (waiting on @noaov1, @rodrigo-pino, and @varex83)

[rodrigo-pino]

Reviewable status: 16 of 24 files reviewed, 11 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/cairo_compile.rs line 151 at r4 (raw file):

Previously, varex83 (Bohdan Ohorodnii) wrote…

Will do

Done

---

crates/blockifier/src/test_utils/cairo_compile.rs line 125 at r48 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can be removed, no?

For calling the starknet-sierra-compile we need to re build the compiler args. I don't find a simple way of removing this code duplication

---

crates/blockifier/src/test_utils/contracts.rs line 256 at r48 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

There is an ERC20 contract for Cairo1 as well.

sequencer/crates/blockifier/ERC20/ERC20_Cairo1/ERC20_cairo1.cairo

Line 16 in 35f5081

use integer::BoundedInt;

👍

---

crates/blockifier/src/test_utils/struct_impls.rs line 261 at r48 (raw file):

    /// If control over the compilation is desired use [Self::new] instead.
    pub fn try_from_json_string(
        raw_contract_class: &str,

Done,

---

crates/blockifier/src/test_utils/struct_impls.rs line 262 at r48 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why do we need a result in test utils? I think that you can use unwrap.

Done.

---

crates/blockifier/src/test_utils/struct_impls.rs line 268 at r48 (raw file):

            sierra_program: &cairo_lang_sierra::program::Program,
        ) -> Result<AotNativeExecutor, cairo_native::error::Error> {
            let native_context = cairo_native::context::NativeContext::new();

Done

---

crates/blockifier/src/test_utils/struct_impls.rs line 269 at r48 (raw file):

        ) -> Result<AotNativeExecutor, cairo_native::error::Error> {
            let native_context = cairo_native::context::NativeContext::new();
            let native_program = native_context.compile(sierra_program, false)?;

Done

---

crates/blockifier/src/test_utils/struct_impls.rs line 292 at r48 (raw file):

Previously, noaov1 (Noa Oved) wrote…

When is it used?

For loading feature contracts

---

crates/tests-integration/Cargo.toml line 12 at r48 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Is this necessary?

Integration depends on the blockifier and uses definitions that also involved Native. In this specific case it is only for Cairo Version

[github-actions]

Artifacts upload triggered. View details here

[avi-starkware]

Reviewed 3 of 8 files at r53, 12 of 12 files at r56, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @meship-starkware, @noaov1, @rodrigo-pino, and @varex83)

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.561 ms 35.061 ms 35.644 ms]
change: [+1.1032% +2.6498% +4.4584%] (p = 0.00 < 0.05)
Performance has regressed.
Found 17 outliers among 100 measurements (17.00%)
6 (6.00%) high mild
11 (11.00%) high severe

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[noaov1]

Reviewed 3 of 8 files at r53, 9 of 12 files at r56, 2 of 2 files at r57, 10 of 10 files at r58, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)

[noaov1]

Dismissed @varex83 from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @varex83)