refactor: feature contracts to use native together with casm

[varex83]

No description provided.

[lotem-starkware]

This change is 

[codecov]

Codecov Report

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

Project coverage is 68.91%. Comparing base (e3165c4) to head (b80eda2).
Report is 479 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/test_utils/contracts.rs	9.37%	29 Missing ⚠️
crates/blockifier/src/test_utils/cairo_compile.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1455       +/-   ##
===========================================
+ Coverage   40.10%   68.91%   +28.81%     
===========================================
  Files          26      105       +79     
  Lines        1895    13687    +11792     
  Branches     1895    13687    +11792     
===========================================
+ Hits          760     9433     +8673     
- Misses       1100     3851     +2751     
- Partials       35      403      +368     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 2 of 3 files at r16, all commit messages.
Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

// remove the file manually to continue.
// """
static GIT_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

@dorimedini-starkware WDYT?

Code quote:

static GIT_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

[avi-starkware]

Reviewed 1 of 3 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @varex83)

[dorimedini-starkware]

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

---

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

@dorimedini-starkware WDYT?

can you explain why this is needed? not sure it's a good idea (let's chat)

---

crates/blockifier/src/test_utils.rs line 88 at r16 (raw file):

        }
    }
}

I don't think you really need this... and converting 2 to Native is non-intuitive

Code quote:

impl From<isize> for CairoVersion {
    fn from(value: isize) -> Self {
        match value {
            0 => Self::Cairo0,
            1 => Self::Cairo1,
            #[cfg(feature = "cairo_native")]
            2 => Self::Native,
            _ => panic!("Invalid value for CairoVersion: {}", value),
        }
    }
}

---

crates/blockifier/src/test_utils/contracts.rs line 120 at r16 (raw file):

    ///
    /// This order is defined in [CairoVersion] enum.
    fn supported_versions(&self) -> u32 {

please use more explicit typing...

Suggestion:

HashSet<CairoVersion>

---

crates/blockifier/src/test_utils/contracts.rs line 333 at r16 (raw file):

                    #[cfg(feature = "cairo_native")]
                    CairoVersion::Native => "_sierra",
                },

Suggestion:

                "feature_contracts/cairo{}/{}{}.json",
                match cairo_version {
                    CairoVersion::Cairo0 => "0/compiled",
                    CairoVersion::Cairo1 => "1/compiled_casm",
                    #[cfg(feature = "cairo_native")]
                    CairoVersion::Native => "1/compiled_sierra",
                },

---

crates/blockifier/src/test_utils/contracts.rs line 460 at r16 (raw file):

                    .into_iter()
            }
        })

refactor: just iterate over supported versions and add the different variants (see above, supported_versions should return a set of versions, not a u32)

Code quote:

        Self::iter().flat_map(|contract| {
            // If only one supported version exists, add the contract to the array as is.
            if contract.supported_versions().is_power_of_two() {
                vec![contract].into_iter()
            } else {
                let supported_versions = contract.supported_versions();

                #[cfg(feature = "cairo_native")]
                let range = 0..3isize;
                #[cfg(not(feature = "cairo_native"))]
                let range = 0..2isize;

                // If multiple supported versions exist, add each supported version of the
                // contract to the array.
                range
                    .filter(|i| supported_versions & (1u32 << i) != 0)
                    .map(move |i| {
                        let mut contract = contract;
                        contract.set_cairo_version(CairoVersion::from(i));

                        contract
                    })
                    .collect::<Vec<_>>()
                    .into_iter()
            }
        })

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 5 of 8 files at r17, all commit messages.
Reviewable status: 17 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

[meship-starkware]

Reviewed 6 of 8 files at r17, all commit messages.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @noaov1, and @varex83)

[avi-starkware]

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, dorimedini-starkware wrote…

can you explain why this is needed? not sure it's a good idea (let's chat)

As far as I understood, CI errors may appear when running git commands in the function prepare_cairo1_compiler_deps because it is run concurrently in tests, causing a race condition on the index.lock file.

[dorimedini-starkware]

Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, avi-starkware wrote…

As far as I understood, CI errors may appear when running git commands in the function prepare_cairo1_compiler_deps because it is run concurrently in tests, causing a race condition on the index.lock file.

who runs it concurrently?
there is an open task to do this, it is not simple for this reason.
we do not want to parallelize blindly.
different cairo1 compiler versions used should be checked out sequentially, so we don't build the same tag multiple times.
when parallelizing, we should group compile jobs by compiler version, and execute groups sequentially,
or checkout different copies of the compiler repo (worktrees?) to be able to compile different versions in parallel

[avi-starkware]

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, dorimedini-starkware wrote…

who runs it concurrently?
there is an open task to do this, it is not simple for this reason.
we do not want to parallelize blindly.
different cairo1 compiler versions used should be checked out sequentially, so we don't build the same tag multiple times.
when parallelizing, we should group compile jobs by compiler version, and execute groups sequentially,
or checkout different copies of the compiler repo (worktrees?) to be able to compile different versions in parallel

I think it is used in multiple tests and cargo test runs these tests concurrently in the CI

[dorimedini-starkware]

Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, avi-starkware wrote…

I think it is used in multiple tests and cargo test runs these tests concurrently in the CI

then this is new.
since when do multiple tests recompile?
this is not intended, and if it is done, the cairo repo should be explicitly checked out to a different directory

[dorimedini-starkware]

Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

---

crates/blockifier/src/test_utils/cairo_compile.rs line 213 at r16 (raw file):

Previously, dorimedini-starkware wrote…

then this is new.
since when do multiple tests recompile?
this is not intended, and if it is done, the cairo repo should be explicitly checked out to a different directory

or, implement "smart" parallelization...
waiting for a lock is not a good solution, we will end up recompiling the same compiler tag multiple times

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 1 files at r18, all commit messages.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @noaov1, and @varex83)

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Can we avoid the extra casm compilation here?

Reviewed 2 of 15 files at r9, 1 of 13 files at r13, 4 of 8 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: 17 of 20 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @varex83)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Closing PR since a bigger rework is required which will include this fix as part of it