chore(cairo_native): add the compilation of native to the flow #2770

meship-starkware · 2024-12-18T15:56:08Z

No description provided.

reviewable-StarkWare · 2024-12-18T15:56:14Z

This change is

meship-starkware

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1)

crates/papyrus_state_reader/src/papyrus_state.rs line 156 at r1 (raw file):

        #[cfg(feature = "cairo_native")]
        {
            let versioned_constants = VersionedConstants::latest_constants();

We should probably add the min Sierra version to the Papyrus reader instead.

Code quote:

let versioned_constants = VersionedConstants::latest_constants();

crates/papyrus_state_reader/src/papyrus_state.rs line 195 at r1 (raw file):

                            self.contract_class_manager
                                .get_sierra(&class_hash)
                                .expect("Sierra class not found"),

this is not right if we are in cairo0 we wont have the sierra in the cache need to fix

Code quote:

                            self.contract_class_manager
                                .get_sierra(&class_hash)
                                .expect("Sierra class not found"),

avi-starkware · 2024-12-18T17:28:42Z

crates/papyrus_state_reader/src/papyrus_state.rs line 195 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

this is not right if we are in cairo0 we wont have the sierra in the cache need to fix

You can't assume that sierra must be in cache if the casm is in cache. If for some reason the sierra cache dropped the class while the casm cache still holds it, you will panic here. You should fetch the sierra from the state in that case. (You can skip all the checks for Cairo0 and all that and just assume the Sierra is where it is supposed to be.)

github-actions · 2024-12-19T11:50:56Z

Artifacts upload workflows:

noaov1

Reviewed 1 of 3 files at r2, 2 of 3 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 55 at r5 (raw file):

    /// Returns a V1 contract if found, or a V0 contract if a V1 contract is not
    /// found, or an `Error` otherwise.
    fn get_compiled_class_inner(&self, class_hash: ClassHash) -> StateResult<InnerCallReturnType> {

Why not cache the sierra (and the casm) within this method to avoid returning the sierra?

Code quote:

    fn get_compiled_class_inner(&self, class_hash: ClassHash) -> StateResult<InnerCallReturnType> {

crates/papyrus_state_reader/src/papyrus_state.rs line 122 at r5 (raw file):

    #[cfg(feature = "cairo_native")]
    fn get_casm_and_sierra_when_casm_in_cache(

Consider defining a method that fetches sierra (starting from the sierra cache and then papyrus)

Code quote:

    fn get_casm_and_sierra_when_casm_in_cache(

crates/papyrus_state_reader/src/papyrus_state.rs line 249 at r5 (raw file):

                                    Some(Arc::new(
                                        sierra.expect("Sierra class should exist in cairo1 flow."),
                                    )),

Note that this is already checked in couple_casm_and_sierra (which is called in get_compiled_class_inner.

Code quote:

                            match versioned_casm {
                                VersionedRunnableCompiledClass::Cairo0(casm) => {
                                    assert!(sierra.is_none(), "Sierra should be None for Cairo0.");
                                    (VersionedRunnableCompiledClass::Cairo0(casm), None)
                                }
                                VersionedRunnableCompiledClass::Cairo1((casm, sierra_version)) => (
                                    VersionedRunnableCompiledClass::Cairo1((casm, sierra_version)),
                                    Some(Arc::new(
                                        sierra.expect("Sierra class should exist in cairo1 flow."),
                                    )),

crates/papyrus_state_reader/src/papyrus_state.rs line 256 at r5 (raw file):

                    let versioned_casm = casm.clone();
                    match sierra {

Consider matching the cairo version

Code quote:

                    match sierra {

crates/papyrus_state_reader/src/papyrus_state.rs line 265 at r5 (raw file):

                                _,
                            )) = casm
                            {

Can you please cache the sierra as well?
cc @avi-starkware

avi-starkware · 2024-12-22T09:03:40Z

crates/papyrus_state_reader/src/papyrus_state.rs line 155 at r5 (raw file):

            }
        }
    }

If you don't have both casm and sierra, you anyway have to go through get_casm_and_sierra to get the missing contract. So I think we can reduce the amount of flows if we just try to get both contracts from cache, and if both are not in cache then fetch both from the state_reader.

Alternatively, if it is easy to add get_sierra to the state reader, then we should consider separating the flow that fetches sierra from the flow that fetches casm.

Code quote:

    #[cfg(feature = "cairo_native")]
    fn get_casm_and_sierra_when_casm_in_cache(
        &self,
        class_hash: ClassHash,
        versioned_casm: VersionedRunnableCompiledClass,
    ) -> StateResult<(VersionedRunnableCompiledClass, Option<Arc<SierraContractClass>>)> {
        match versioned_casm {
            // Cairo0 does not have a sierra class.
            VersionedRunnableCompiledClass::Cairo0(casm) => {
                Ok((VersionedRunnableCompiledClass::Cairo0(casm), None))
            }
            VersionedRunnableCompiledClass::Cairo1((casm, sierra_version)) => {
                let new_versioned_casm =
                    VersionedRunnableCompiledClass::Cairo1((casm, sierra_version));
                //
                let sierra_option = self.contract_class_manager.get_sierra(&class_hash);
                match sierra_option {
                    Some(sierra) => Ok((new_versioned_casm, Some(sierra))),
                    None => {
                        // We assume that the class is already declared because the casm was in the
                        // cache.
                        let (_, option_sierra) = self
                            .reader()?
                            .get_casm_and_sierra(&class_hash)
                            .map_err(|err| StateError::StateReadError(err.to_string()))?;
                        let sierra = option_sierra.expect(
                            "Should be able to fetch a Sierra class if its definition exists, \
                             database is inconsistent.",
                        );
                        Ok((new_versioned_casm, Some(Arc::new(sierra))))
                    }
                }
            }
        }
    }

avi-starkware · 2024-12-22T09:26:37Z

crates/papyrus_state_reader/src/papyrus_state.rs line 225 at r5 (raw file):

                        }
                    }
                }

These nested matchs get confusing.

Suggestion:

            if let Some(native_compiled_class) = native_versioned_contract_class {
                // we already compiled to native and have the cached version
                Some(cached_native) => {
                    match cached_native {
                        CachedCairoNative::Compiled(compiled_native) => {
                            Ok(RunnableCompiledClass::from(compiled_native))
                        }
                        // for some reason the compilation failed, we use the non cairo native flow.
                        CachedCairoNative::CompilationFailed => {
                            self.get_compiled_class_non_native_flow(class_hash)
                        }
                    }
                }
            }

avi-starkware

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

crates/papyrus_state_reader/src/papyrus_state.rs line 267 at r5 (raw file):

                            {
                                self.contract_class_manager
                                    .send_compilation_request((class_hash, sierra, casm));

You can also add the flow where native compilation is blocking here (see #2771)
You can share code with the case where the compiled native already exists in cache.

Suggestion:

                                self.contract_class_manager
                                    .send_compilation_request((class_hash, sierra, casm));
                                if self.contract_class_manager.config.wait_on_native_compilation {
                                    /// Get native compiled class from cache
                                    ...
                                    Ok(RunnableCompiledClass::from(native)
                                }

crates/papyrus_state_reader/src/papyrus_state.rs

meship-starkware

Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 1 of 5 files reviewed, 7 unresolved discussions (waiting on @avi-starkware, @graphite-app[bot], and @noaov1)

crates/papyrus_state_reader/src/papyrus_state.rs line 55 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why not cache the sierra (and the casm) within this method to avoid returning the sierra?

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 122 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining a method that fetches sierra (starting from the sierra cache and then papyrus)

Not relevant.

crates/papyrus_state_reader/src/papyrus_state.rs line 155 at r5 (raw file):

Previously, avi-starkware wrote…

If you don't have both casm and sierra, you anyway have to go through get_casm_and_sierra to get the missing contract. So I think we can reduce the amount of flows if we just try to get both contracts from cache, and if both are not in cache then fetch both from the state_reader.

Alternatively, if it is easy to add get_sierra to the state reader, then we should consider separating the flow that fetches sierra from the flow that fetches casm.

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 225 at r5 (raw file):

Previously, avi-starkware wrote…

These nested matchs get confusing.

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 249 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Note that this is already checked in couple_casm_and_sierra (which is called in get_compiled_class_inner.

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 265 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please cache the sierra as well?
cc @avi-starkware

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 267 at r5 (raw file):

Previously, avi-starkware wrote…

You can also add the flow where native compilation is blocking here (see #2771)
You can share code with the case where the compiled native already exists in cache.

Done.

crates/blockifier/src/state/contract_class_manager.rs line 156 at r6 (raw file):

    pub fn cache_request_contracts(&self, request: &CompilationRequest) {
        let (class_hash, sierra, casm) = request.clone();
        self.contract_caches.set_sierra(class_hash, sierra);

It won't be necessary once the Sierra and Casm Cache are united

Code quote:

    pub fn cache_request_contracts(&self, request: &CompilationRequest) {
        let (class_hash, sierra, casm) = request.clone();
        self.contract_caches.set_sierra(class_hash, sierra);

crates/papyrus_state_reader/src/papyrus_state.rs

meship-starkware

Reviewable status: 1 of 5 files reviewed, 7 unresolved discussions (waiting on @avi-starkware, @graphite-app[bot], and @noaov1)

crates/papyrus_state_reader/src/papyrus_state.rs line 203 at r7 (raw file):

                    // for some reason the compilation failed, we use the non cairo native flow.
                    CachedCairoNative::CompilationFailed => {
                        return self.get_compiled_class_non_native_flow(class_hash);

This creates a small inconsistency. If the compilation failed and the Casm and Sierra were in the cache, we would have both. If they were not in the cache, we would only save the cache. The more common case would be that we have the Casm and sierra in the cache.

Code quote:

     return self.get_compiled_class_non_native_flow(class_hash);

noaov1

Reviewed 2 of 4 files at r6, 1 of 2 files at r7, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @graphite-app[bot], and @meship-starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 31 at r7 (raw file):

type RawPapyrusReader<'env> = papyrus_storage::StorageTxn<'env, RO>;
type GetCompiledIneerReturnType =
    StateResult<(RunnableCompiledClass, Option<Arc<SierraContractClass>>)>;

Can you use CachedCasm?

Code quote:

type GetCompiledIneerReturnType =
    StateResult<(RunnableCompiledClass, Option<Arc<SierraContractClass>>)>;

avi-starkware · 2024-12-23T13:50:14Z

crates/papyrus_state_reader/src/papyrus_state.rs line 203 at r7 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This creates a small inconsistency. If the compilation failed and the Casm and Sierra were in the cache, we would have both. If they were not in the cache, we would only save the cache. The more common case would be that we have the Casm and sierra in the cache.

I think this is okay.
We will still only recompile after the cached Cairo native entry gets dropped from cache, which shouldn't happen very often.

WDYT @Yoni-Starkware @noaov1 ?

noaov1

Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 127 at r7 (raw file):

        casm: RunnableCompiledClass,
    ) -> RunnableCompiledClass {
        let cached_native = self

Can you please assert that wait_on_native_compilation == true?

Code quote:

        let cached_native = self

crates/papyrus_state_reader/src/papyrus_state.rs line 135 at r7 (raw file):

                RunnableCompiledClass::from(compiled_native)
            }
            CachedCairoNative::CompilationFailed => casm,

In this case, do we want to cache the casm?

Code quote:

CachedCairoNative::CompilationFailed => casm,

crates/papyrus_state_reader/src/papyrus_state.rs line 218 at r7 (raw file):

            if let Some(sierra) = option_sierra {
                if let RunnableCompiledClass::V1(casm_v1) = casm.clone() {

We will always enter this if, right? You only use it to extract the casm?

Code quote:

 if let RunnableCompiledClass::V1(casm_v1) = casm.clone() {

crates/papyrus_state_reader/src/papyrus_state.rs line 224 at r7 (raw file):

                        return Ok(self.handle_sync_compilation(class_hash, casm));
                    }
                } else if casm_cached.is_none() {

Why is this if needed?

Code quote:

if casm_cached.is_none()

crates/papyrus_state_reader/src/papyrus_state.rs line 229 at r7 (raw file):

                }
            }
            Ok(casm)

Do we cache the casm in case of no sierra?

Code quote:

Ok(casm)

meship-starkware

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 135 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

In this case, do we want to cache the casm?

It is for testing, so I do not know if it's that important, but we have already cached both the Sierra and Casm.
in here

crates/papyrus_state_reader/src/papyrus_state.rs line 218 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

We will always enter this if, right? You only use it to extract the casm?

Yes, I just don't know of any other way.

crates/papyrus_state_reader/src/papyrus_state.rs line 224 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is this if needed?

not relevant

crates/papyrus_state_reader/src/papyrus_state.rs line 229 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Do we cache the casm in case of no sierra?

not relevant

meship-starkware

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 135 at r7 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It is for testing, so I do not know if it's that important, but we have already cached both the Sierra and Casm.
in here

sequencer/crates/papyrus_state_reader/src/papyrus_state.rs

Line 218 in 18129b5

cached_casm

avi-starkware

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 117 at r12 (raw file):

    #[cfg(feature = "cairo_native")]
    fn extract_contract_in_sync_compilation(

Suggestion:

get_contract_after_waiting_on_native_compilation

crates/papyrus_state_reader/src/papyrus_state.rs line 184 at r12 (raw file):

        #[cfg(feature = "cairo_native")]
        {
            // If we turned off the cairo native compilation, we use the non cairo native flow.

Suggestion:

// If Cairo native compilation is disabled, use the non-Cairo-native flow (i.e., save only casm in cache, and make no compilation requests).

crates/papyrus_state_reader/src/papyrus_state.rs line 189 at r12 (raw file):

            }

            // We have the Native in cache.

Suggestion:

            // The cache contains a Cairo-native entry for the given class hash.

crates/papyrus_state_reader/src/papyrus_state.rs line 195 at r12 (raw file):

                        return Ok(RunnableCompiledClass::from(compiled_native));
                    }
                    // for some reason the compilation failed, we use the non cairo native flow.

Suggestion:

                    // The compilation previously failed. Make no further compilation attempts. Use the non-Cairo-native flow.

crates/papyrus_state_reader/src/papyrus_state.rs line 203 at r12 (raw file):

            let cached_casm = match self.contract_class_manager.get_casm(&class_hash) {
                // Casm is in cache.

Suggestion:

            // Get the `CachedCasm` for the given class hash. Upon cache miss, fetch it from state and cache it. 
            let cached_casm = match self.contract_class_manager.get_casm(&class_hash) {

crates/papyrus_state_reader/src/papyrus_state.rs line 212 at r12 (raw file):

            };

            match cached_casm {

Suggestion:

            // If the fetched casm includes a Sierra, send a compilation request.
            // Return the casm.
            // NOTE: We assume that whenever the fetched casm does not include a Sierra, compilation to native is not required.
            match cached_casm {

crates/papyrus_state_reader/src/papyrus_state.rs line 220 at r12 (raw file):

                            casm_v1.clone(),
                        ));
                        if self.contract_class_manager.wait_on_native_compilation() {

Suggestion:

                        if self.contract_class_manager.wait_on_native_compilation() {
                            // With this config, sending a compilation request blocks the sender until the compilation compilation completes.
                            // Fetch the resulting native compiled class and return it if compilation succeeded.

avi-starkware · 2024-12-24T10:01:16Z

crates/papyrus_state_reader/src/papyrus_state.rs line 98 at r12 (raw file):

    }

    fn get_compiled_class_non_native_flow(

Add docstring

Code quote:

    fn get_compiled_class_non_native_flow(

avi-starkware · 2024-12-24T10:01:37Z

crates/papyrus_state_reader/src/papyrus_state.rs line 117 at r12 (raw file):

    #[cfg(feature = "cairo_native")]
    fn extract_contract_in_sync_compilation(

add docstring

avi-starkware · 2024-12-24T10:02:54Z

crates/papyrus_state_reader/src/papyrus_state.rs line 52 at r12 (raw file):

    /// Returns a V1 contract if found, or a V0 contract if a V1 contract is not
    /// found, or an `Error` otherwise.

Fix the docstring given our changes.

Code quote:

    /// Returns a V1 contract if found, or a V0 contract if a V1 contract is not
    /// found, or an `Error` otherwise.

avi-starkware · 2024-12-24T10:05:17Z

crates/papyrus_state_reader/src/papyrus_state.rs line 117 at r12 (raw file):

Previously, avi-starkware wrote…

add docstring

Actually, maybe get_compiled_class_after_waiting_on_native_compilation
(to be consistent with other method names in this impl)

avi-starkware

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

dorimedini-starkware

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

a discussion (no related file):
rebase over main-v0.13.4

avi-starkware

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

meship-starkware

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @noaov1, and @Yoni-Starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

rebase over main-v0.13.4

Done.

avi-starkware

Reviewed 3 of 4 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware, @meship-starkware, and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 193 at r14 (raw file):

            // If Cairo native compilation is disabled, use the non-Cairo-native flow (i.e., save
            // only casm in cache, and make no compilation requests).
            if !self.contract_class_manager.run_cairo_native() {

Suggestion:

            if !self.contract_class_manager.run_cairo_native() {
                // Cairo native is disabled - use the non-Cairo-native flow.

crates/papyrus_state_reader/src/papyrus_state.rs line 199 at r14 (raw file):

            // This flow is when we have the Native in cache, in this flow we return a runnable
            // compiled native if the compilation succeed and runnable casm otherwise.
            if let Some(cached_native) = self.contract_class_manager.get_native(&class_hash) {

Suggestion:

            // Try fetching native from cache.
            if let Some(cached_native) = self.contract_class_manager.get_native(&class_hash) {

crates/papyrus_state_reader/src/papyrus_state.rs line 206 at r14 (raw file):

                    // The compilation previously failed. Make no further compilation attempts. Use
                    // the non-Cairo-native flow.
                    CachedCairoNative::CompilationFailed => {

Suggestion:

                    CachedCairoNative::CompilationFailed => {
                        // The compilation previously failed. Make no further compilation attempts. Use
                        // the non-Cairo-native flow.

crates/papyrus_state_reader/src/papyrus_state.rs line 213 at r14 (raw file):

            // Get the `CachedCasm` for the given class hash. Upon cache miss, fetch it from state
            // and cache it.

Suggestion:

            // Native not found in cache. Get the `CachedCasm` - if not in cache, fetch it from state
            // and cache it.

crates/papyrus_state_reader/src/papyrus_state.rs line 240 at r14 (raw file):

                            // completes. Fetch the resulting native
                            // compiled class and return it if compilation
                            // succeeded.

plus fix the line breaks

Suggestion:

                            // With this config, sending a compilation request blocks the sender
                            // until compilation
                            // completes. Retry fetching Native from cache.

crates/papyrus_state_reader/src/papyrus_state.rs line 251 at r14 (raw file):

                            "A Sierra file was saved in cache for a Cairo0 contract - class hash \
                             {class_hash}.
                        This is probably a bug as no Sierra file exists for a Cairo0 contract."

Suggestion:

                            This is probably a bug as no Sierra file exists for a Cairo0 contract."

dorimedini-starkware

Reviewed 1 of 4 files at r13.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

avi-starkware · 2024-12-24T12:17:38Z

crates/papyrus_state_reader/src/papyrus_state.rs line 100 at r14 (raw file):

    // Handle get compiled class when cairo native is turned off.
    // Returns the Runnable Casm from the cache if it exists, otherwise fetches it from the
    // database.

Suggestion:

    // Handle `get_compiled_class` when cairo native is turned off.
    // Returns casm from cache if exists, otherwise fetches it from state.

meship-starkware

Reviewable status: 6 of 7 files reviewed, 7 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 240 at r14 (raw file):

Previously, avi-starkware wrote…

plus fix the line breaks

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 193 at r14 (raw file):

            // If Cairo native compilation is disabled, use the non-Cairo-native flow (i.e., save
            // only casm in cache, and make no compilation requests).
            if !self.contract_class_manager.run_cairo_native() {

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 199 at r14 (raw file):

            // This flow is when we have the Native in cache, in this flow we return a runnable
            // compiled native if the compilation succeed and runnable casm otherwise.
            if let Some(cached_native) = self.contract_class_manager.get_native(&class_hash) {

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 206 at r14 (raw file):

                    // The compilation previously failed. Make no further compilation attempts. Use
                    // the non-Cairo-native flow.
                    CachedCairoNative::CompilationFailed => {

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 251 at r14 (raw file):

                            "A Sierra file was saved in cache for a Cairo0 contract - class hash \
                             {class_hash}.
                        This is probably a bug as no Sierra file exists for a Cairo0 contract."

Done.

avi-starkware · 2024-12-24T12:20:43Z

crates/papyrus_state_reader/src/papyrus_state.rs line 119 at r14 (raw file):

    #[cfg(feature = "cairo_native")]
    // Returns the native compiled class if the compilation succeeded and the runnable casm upon
    // failure.

Suggestion:

    // Handles `get_compiled_class` under the assumption that native compilation has finished.
    // Returns the native compiled class if the compilation succeeded and the runnable casm upon
    // failure.

avi-starkware

Reviewed 1 of 4 files at r6, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

avi-starkware · 2024-12-24T12:24:51Z

crates/papyrus_state_reader/src/papyrus_state.rs line 100 at r14 (raw file):

    // Handle get compiled class when cairo native is turned off.
    // Returns the Runnable Casm from the cache if it exists, otherwise fetches it from the
    // database.

Handles sorry.

meship-starkware

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 100 at r14 (raw file):

    // Handle get compiled class when cairo native is turned off.
    // Returns the Runnable Casm from the cache if it exists, otherwise fetches it from the
    // database.

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 119 at r14 (raw file):

    #[cfg(feature = "cairo_native")]
    // Returns the native compiled class if the compilation succeeded and the runnable casm upon
    // failure.

Done.

crates/papyrus_state_reader/src/papyrus_state.rs line 213 at r14 (raw file):

            // Get the `CachedCasm` for the given class hash. Upon cache miss, fetch it from state
            // and cache it.

Done.

avi-starkware

Reviewed 1 of 4 files at r6, 1 of 1 files at r16.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @Yoni-Starkware)

non blocking

noaov1

Reviewed 1 of 4 files at r13, 1 of 1 files at r14, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

crates/papyrus_state_reader/src/papyrus_state.rs line 52 at r14 (raw file):

    /// Returns a CachedCasm with Sierra if V1 contract is found, or a CachedCasm without Sierra if
    /// a V1 contract is not found, or an `Error` otherwise.

Suggestion:

    /// Returns a V1 contract with Sierra if V1 contract is found, or a V0 contract without Sierra if
    /// a V1 contract is not found, or an `Error` otherwise.

crates/papyrus_state_reader/src/papyrus_state.rs line 100 at r16 (raw file):

    // Handles `get_compiled_class` when cairo native is turned off.
    // Returns casm from cache if exists, otherwise fetches it from state.
    fn get_compiled_class_non_native_flow(

Suggestion:

    /// Returns casm from cache if exists, otherwise fetches it from state.
    fn fetch_casm(

crates/papyrus_state_reader/src/papyrus_state.rs line 119 at r16 (raw file):

    // Handles `get_compiled_class` under the assumption that native compilation has finished.
    // Returns the native compiled class if the compilation succeeded and the runnable casm upon
    // failure.

Suggestion:

    /// Handles `get_compiled_class` under the assumption that native compilation has finished.
    /// Returns the native compiled class if the compilation succeeded and the runnable casm upon
    /// failure.

crates/papyrus_state_reader/src/papyrus_state.rs line 219 at r16 (raw file):

                    cached_casm
                }
            };

This is exactly what get_compiled_class_non_native_flow does, no?
Two differences:

return value.
with/without sierra.

Consider sharing code (non-blocking)

Code quote:

            let cached_casm = match self.contract_class_manager.get_casm(&class_hash) {
                Some(cached_casm) => cached_casm,
                None => {
                    let cached_casm = self.get_compiled_class_inner(class_hash)?;
                    self.contract_class_manager.set_casm(class_hash, cached_casm.clone());
                    cached_casm
                }
            };

crates/papyrus_state_reader/src/papyrus_state.rs line 247 at r16 (raw file):

                             {class_hash}. This is probably a bug as no Sierra file exists for a \
                             Cairo0 contract."
                        );

I think it should be a panic.

Code quote:

                        log::warn!(
                            "A Sierra file was saved in cache for a Cairo0 contract - class hash \
                             {class_hash}. This is probably a bug as no Sierra file exists for a \
                             Cairo0 contract."
                        );

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 2acfc46 to 1600a7d Compare December 18, 2024 15:59

meship-starkware requested review from avi-starkware and noaov1 December 18, 2024 15:59

meship-starkware commented Dec 18, 2024

View reviewed changes

Base automatically changed from avi/compilation-thread/papyrus-reader to main December 18, 2024 17:14

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 1600a7d to 664b3ee Compare December 19, 2024 11:50

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch 4 times, most recently from 14b683b to 9925284 Compare December 19, 2024 14:27

noaov1 requested changes Dec 22, 2024

View reviewed changes

avi-starkware requested changes Dec 22, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 9925284 to 5c87317 Compare December 22, 2024 15:37

graphite-app bot reviewed Dec 22, 2024

View reviewed changes

crates/papyrus_state_reader/src/papyrus_state.rs Outdated Show resolved Hide resolved

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 5c87317 to 83cf6f2 Compare December 23, 2024 11:08

meship-starkware commented Dec 23, 2024

View reviewed changes

crates/papyrus_state_reader/src/papyrus_state.rs Outdated Show resolved Hide resolved

meship-starkware commented Dec 23, 2024

View reviewed changes

noaov1 reviewed Dec 23, 2024

View reviewed changes

noaov1 requested changes Dec 23, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 83cf6f2 to 18129b5 Compare December 23, 2024 14:12

meship-starkware commented Dec 23, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch 2 times, most recently from 6d1e8ce to 5a60a63 Compare December 23, 2024 15:22

avi-starkware requested changes Dec 24, 2024

View reviewed changes

avi-starkware approved these changes Dec 24, 2024

View reviewed changes

dorimedini-starkware previously requested changes Dec 24, 2024

View reviewed changes

avi-starkware approved these changes Dec 24, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from e42fa94 to 6c05d73 Compare December 24, 2024 11:29

meship-starkware changed the base branch from main to main-v0.13.4 December 24, 2024 11:42

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 6c05d73 to 0b6461c Compare December 24, 2024 11:45

meship-starkware commented Dec 24, 2024

View reviewed changes

avi-starkware requested changes Dec 24, 2024

View reviewed changes

dorimedini-starkware reviewed Dec 24, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 0b6461c to 70e85d4 Compare December 24, 2024 12:20

meship-starkware commented Dec 24, 2024

View reviewed changes

avi-starkware reviewed Dec 24, 2024

View reviewed changes

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 70e85d4 to 56d4550 Compare December 24, 2024 12:24

chore(cairo_native): add the compilation of native to the flow

e1a7869

meship-starkware force-pushed the meship/add_native_compilation_to_flow branch from 56d4550 to e1a7869 Compare December 24, 2024 12:25

meship-starkware commented Dec 24, 2024

View reviewed changes

avi-starkware approved these changes Dec 24, 2024

View reviewed changes

noaov1 approved these changes Dec 24, 2024

View reviewed changes

meship-starkware merged commit 423f696 into main-v0.13.4 Dec 24, 2024
21 checks passed

meship-starkware deleted the meship/add_native_compilation_to_flow branch December 24, 2024 12:56

github-actions bot locked and limited conversation to collaborators Dec 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cairo_native): add the compilation of native to the flow #2770

chore(cairo_native): add the compilation of native to the flow #2770

meship-starkware commented Dec 18, 2024

reviewable-StarkWare commented Dec 18, 2024

meship-starkware left a comment

avi-starkware commented Dec 18, 2024

github-actions bot commented Dec 19, 2024 •

edited

Loading

noaov1 left a comment

avi-starkware commented Dec 22, 2024

avi-starkware commented Dec 22, 2024

avi-starkware left a comment

meship-starkware left a comment

meship-starkware left a comment

noaov1 left a comment

avi-starkware commented Dec 23, 2024

noaov1 left a comment

meship-starkware left a comment

meship-starkware left a comment

avi-starkware left a comment

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware left a comment

dorimedini-starkware left a comment

avi-starkware left a comment

meship-starkware left a comment

avi-starkware left a comment

dorimedini-starkware left a comment

avi-starkware commented Dec 24, 2024

meship-starkware left a comment

avi-starkware commented Dec 24, 2024

avi-starkware left a comment

avi-starkware commented Dec 24, 2024

meship-starkware left a comment

avi-starkware left a comment

noaov1 left a comment

chore(cairo_native): add the compilation of native to the flow #2770

chore(cairo_native): add the compilation of native to the flow #2770

Conversation

meship-starkware commented Dec 18, 2024

reviewable-StarkWare commented Dec 18, 2024

meship-starkware left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 18, 2024

github-actions bot commented Dec 19, 2024 • edited Loading

noaov1 left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 22, 2024

avi-starkware commented Dec 22, 2024

avi-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

noaov1 left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 23, 2024

noaov1 left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

avi-starkware left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware commented Dec 24, 2024

avi-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

avi-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

avi-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 24, 2024

meship-starkware left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 24, 2024

avi-starkware left a comment

Choose a reason for hiding this comment

avi-starkware commented Dec 24, 2024

meship-starkware left a comment

Choose a reason for hiding this comment

avi-starkware left a comment

Choose a reason for hiding this comment

noaov1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024 •

edited

Loading