feat(cairo_native): use contract class manager in papyrus reader

[avi-starkware]

• feat(cairo_native): use contract class manager in papyrus reader

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-17T15:36:17Z
• Started at 2024-12-17T15:36:36Z
• Started at 2024-12-17T15:37:00Z
• Started at 2024-12-18T13:23:55Z
• Started at 2024-12-18T13:29:13Z
• Started at 2024-12-18T13:40:38Z
• Started at 2024-12-18T14:10:02Z
• Started at 2024-12-18T15:13:09Z
• Started at 2024-12-18T15:24:07Z

[noaov1]

Reviewed 4 of 7 files at r1, all commit messages.
Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @alonh5, @avi-starkware, @meship-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/state/contract_class_manager.rs line 60 at r1 (raw file):

        );
        #[cfg(feature = "cairo_native")]
        {

Consider adding an assert verifying that config.run_cairo_native is set to true only if the native feature is on.

Code quote:

        #[cfg(not(feature = "cairo_native"))]
        unimplemented!(
            "Native compilation cannot be enabled when the cairo_native feature is turned off."
        );
        #[cfg(feature = "cairo_native")]
        {

---

crates/native_blockifier/src/py_block_executor.rs line 368 at r1 (raw file):

        // Clear global class cache, to properly revert classes declared in the reverted block.
        // TODO(Avi, 01/01/2025): Consider what exactly to clear in native compilation context.
        self.contract_class_manager.clear();

I think that we will want to clear everything, given that we first check the native cache.

Code quote:

        // TODO(Avi, 01/01/2025): Consider what exactly to clear in native compilation context.
        self.contract_class_manager.clear();

---

crates/blockifier/src/state/global_cache.rs line 56 at r1 (raw file):

pub struct ContractCaches {
    pub casm_cache: GlobalContractCache<VersionedRunnableCompiledClass>,

WDYT?

Suggestion:

pub compiled_class_cache: GlobalContractCache<VersionedRunnableCompiledClass>,

---

crates/blockifier/src/state/global_cache.rs line 58 at r1 (raw file):

    pub casm_cache: GlobalContractCache<VersionedRunnableCompiledClass>,
    #[cfg(feature = "cairo_native")]
    pub native_cache: GlobalContractCache<CachedCairoNative>,

Consider defining this field last

Code quote:

    #[cfg(feature = "cairo_native")]
    pub native_cache: GlobalContractCache<CachedCairoNative>,

[noaov1]

Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5, @avi-starkware, @meship-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/state/global_cache.rs line 58 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining this field last

On second though, as we discussed, we don't need the sierra as well, so you can leave it in this order

---

crates/starknet_batcher/src/batcher.rs line 449 at r1 (raw file):

        wait_on_native_compilation: false,
        contract_cache_size: config.global_contract_cache_size,
    };

?

Suggestion:

    let contract_class_manager_config = ContractClassManagerConfig::default();

---

crates/native_blockifier/src/py_block_executor.rs line 437 at r1 (raw file):

            wait_on_native_compilation: false,
            contract_cache_size: GLOBAL_CONTRACT_CACHE_SIZE_FOR_TEST,
        };

?

Suggestion:

        let contract_class_manager_config = ContractClassManagerConfig::default();

[meship-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5, @avi-starkware, and @Yoni-Starkware)

[avi-starkware]

crates/native_blockifier/src/py_block_executor.rs line 368 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I think that we will want to clear everything, given that we first check the native cache.

Done.

[avi-starkware]

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @alonh5, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/state/contract_class_manager.rs line 60 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider adding an assert verifying that config.run_cairo_native is set to true only if the native feature is on.

I just made it impossible to run the compilation thread if the feature flag is turned off.
That is, config.run_cairo_native does nothing in that case.

---

crates/blockifier/src/state/global_cache.rs line 56 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

WDYT?

I think this makes it more ambiguous because the native cache also contains compiled classes.

---

crates/native_blockifier/src/py_block_executor.rs line 437 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

?

Done.

[alonh5]

Reviewed 2 of 9 files at r2, 1 of 3 files at r4.
Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 446 at r4 (raw file):

        block_builder_config: config.block_builder_config.clone(),
        storage_reader: storage_reader.clone(),
        contract_class_manager: ContractClassManager::start(

Didn't we say it can be configurable and we don't have to run the thread here?

Code quote:

start(

[avi-starkware]

crates/starknet_batcher/src/batcher.rs line 446 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Didn't we say it can be configurable and we don't have to run the thread here?

Yes. The thread doesn't run if the config run_cairo_native is false (which is the default).
Moreover, the thread doesn't run without the cairo_native feature flag (which you don't even have in the starknet_batcher crate)

[avi-starkware]

crates/starknet_batcher/src/batcher.rs line 449 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

?

Done.

[alonh5]

Reviewable status: 4 of 11 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 446 at r4 (raw file):

Previously, avi-starkware wrote…

Yes. The thread doesn't run if the config run_cairo_native is false (which is the default).
Moreover, the thread doesn't run without the cairo_native feature flag (which you don't even have in the starknet_batcher crate)

Oh okay, the documentation confused me.

---

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

    /// Creates a new contract class manager and spawns a thread that listens for compilation
    /// requests and processes them (a.k.a. the compilation worker).
    /// Returns the contract class manager.

Move docstring over #[cfg(feature = "cairo_native")] or edit it to match both.

Code quote:

    /// Creates a new contract class manager and spawns a thread that listens for compilation
    /// requests and processes them (a.k.a. the compilation worker).
    /// Returns the contract class manager.

[alonh5]

Reviewable status: 4 of 11 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

[noaov1]

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

---

crates/blockifier/src/state/contract_class_manager.rs line 143 at r5 (raw file):

        let (class_hash, sierra, casm) = request.clone();
        let sierra_version = SierraVersion::extract_from_program(&sierra.sierra_program).unwrap();
        self.contract_caches.set_sierra(class_hash, sierra);

Do we want to cache the sierra if config.run_cairo_native = false?

Code quote:

        self.contract_caches.set_sierra(class_hash, sierra);

[avi-starkware]

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

Previously, alonh5 (Alon Haramati) wrote…

Move docstring over #[cfg(feature = "cairo_native")] or edit it to match both.

Done.
PTAL

[avi-starkware]

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

---

crates/blockifier/src/state/contract_class_manager.rs line 143 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Do we want to cache the sierra if config.run_cairo_native = false?

Note that this function is called inside send_compilation_request after an assert that the run_cairo_native config is turned on, so we aren't supposed to reach this line if the flag is off.

If we just consider the cache_request_contracts individually, I think we still want to cache the sierra since the function is supposed to cache the entire request. That is, if some other flow would use compilation requests (which include both sierra and casm) and wants to cache the request contracts it makes sense to cache the sierra, since without caching it the sierra would have to be fetched from the state the next time a compilation request would be constructed for that class.

[avi-starkware]

Reviewed 1 of 7 files at r1, 6 of 9 files at r2, 3 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: 10 of 11 files reviewed, all discussions resolved (waiting on @meship-starkware, @noaov1, and @Yoni-Starkware)

[noaov1]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @Yoni-Starkware)