Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge main-v0.13.4 into main #3995

Merged
merged 16 commits into from
Feb 6, 2025

Conversation

Yoni-Starkware
Copy link
Collaborator

No description provided.

@Yoni-Starkware
Copy link
Collaborator Author

Src Dst crates/blockifier/src/execution/syscalls/syscall_tests/get_execution_info.rs
Src Dst crates/blockifier/src/execution/syscalls/syscall_tests/out_of_gas.rs
Src Dst crates/blockifier/src/state/global_cache.rs
Src Dst crates/papyrus_state_reader/Cargo.toml
Src Dst crates/papyrus_state_reader/src/papyrus_state.rs
Src Dst crates/starknet_sequencer_node/Cargo.toml
Src Dst crates/starknet_sequencer_node/src/config/config_test.rs

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 47 files reviewed, 1 unresolved discussion


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 750 at r14 (raw file):

    #[external(v0)]  
    fn test_pedersen (ref self: ContractState) {

@noaov1 / @meship-starkware we should fix the format of this file. Please run the cairo formatter on it (in 0.13.4)

Code quote:

    #[external(v0)]
    fn test_pedersen (ref self: ContractState) {

Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 35 files at r5, 1 of 5 files at r6, 2 of 4 files at r8, 6 of 21 files at r9, 3 of 13 files at r11, 1 of 5 files at r12, 4 of 4 files at r13, 27 of 28 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 35 files at r5, 1 of 5 files at r6, 1 of 4 files at r8, 3 of 21 files at r9, 2 of 13 files at r11, 1 of 5 files at r12, 4 of 4 files at r13, 14 of 28 files at r14.
Reviewable status: all files reviewed, 1 unresolved discussion

@Yoni-Starkware Yoni-Starkware force-pushed the yonatan/merge-main-v0.13.4-into-main-1738827577 branch 3 times, most recently from 27d8133 to ba27928 Compare February 6, 2025 09:39
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 7 files at r15, 2 of 6 files at r16, all commit messages.
Reviewable status: 43 of 49 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


crates/blockifier/src/state/contract_class_manager.rs line 0 at r16 (raw file):
what happened here? why isn't this from the merge (why is it in r4<>r16)?


crates/papyrus_state_reader/src/papyrus_state.rs line 0 at r16 (raw file):
@noaov1 PTAL?


crates/blockifier/src/state/global_cache.rs line 0 at r16 (raw file):
@noaov1 PTAL?

Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 43 of 49 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @meship-starkware)


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

Previously, dorimedini-starkware wrote…

what happened here? why isn't this from the merge (why is it in r4<>r16)?

@elintul moved a struct to a different crate, so I couldn't impl it from this crate.
Instead, I created a struct holding the type and impled it.

@Yoni-Starkware Yoni-Starkware force-pushed the yonatan/merge-main-v0.13.4-into-main-1738827577 branch 2 times, most recently from 5f2ca23 to 45a98bc Compare February 6, 2025 10:13
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 21 files at r9, 3 of 7 files at r15, 6 of 7 files at r18, all commit messages.
Reviewable status: 46 of 49 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/state/global_cache.rs line 185 at r14 (raw file):

        Self(Arc::new(Mutex::new(ContractLRUCache::<T>::with_size(cache_size))))
    }
}

Where is it defined now?

Code quote:

pub const GLOBAL_CONTRACT_CACHE_SIZE_FOR_TEST: usize = 600;

impl<T: Clone> GlobalContractCache<T> {
    /// Locks the cache for atomic access. Although conceptually shared, writing to this cache is
    /// only possible for one writer at a time.
    pub fn lock(&self) -> LockedClassCache<'_, T> {
        self.0.lock().expect("Global contract cache is poisoned.")
    }

    pub fn get(&self, class_hash: &ClassHash) -> Option<T> {
        self.lock().cache_get(class_hash).cloned()
    }

    pub fn set(&self, class_hash: ClassHash, contract_class: T) {
        self.lock().cache_set(class_hash, contract_class);
    }

    pub fn clear(&mut self) {
        self.lock().cache_clear();
    }

    pub fn new(cache_size: usize) -> Self {
        Self(Arc::new(Mutex::new(ContractLRUCache::<T>::with_size(cache_size))))
    }
}

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 7 files at r17, 1 of 7 files at r18.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @meship-starkware, and @Yoni-Starkware)


crates/starknet_sequencer_node/Cargo.toml line 10 at r17 (raw file):

[features]
cairo_native = []
testing = []

No?

Suggestion:

testing = []

@Yoni-Starkware Yoni-Starkware force-pushed the yonatan/merge-main-v0.13.4-into-main-1738827577 branch from 45a98bc to 924d8c3 Compare February 6, 2025 11:08
Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 7 files at r17.
Reviewable status: 47 of 49 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @meship-starkware, and @noaov1)


crates/blockifier/src/state/global_cache.rs line 185 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Where is it defined now?

@elintul moved it to starknet_api::class_cache::GlobalContractCache;


crates/starknet_sequencer_node/Cargo.toml line 10 at r17 (raw file):

Previously, noaov1 (Noa Oved) wrote…

No?

Done.

@Yoni-Starkware Yoni-Starkware force-pushed the yonatan/merge-main-v0.13.4-into-main-1738827577 branch from 924d8c3 to 6653f75 Compare February 6, 2025 11:18
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r19, 2 of 2 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @meship-starkware, and @noaov1)

Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r20.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@Yoni-Starkware Yoni-Starkware added this pull request to the merge queue Feb 6, 2025
Merged via the queue into main with commit daf4118 Feb 6, 2025
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants