chore(blockifier): share syscall handler's fields

[Yoni-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-24T14:54:24Z
• Started at 2024-11-24T15:52:53Z
• Started at 2024-11-24T16:06:01Z
• Started at 2024-11-24T16:06:58Z
• Started at 2024-11-24T16:08:11Z
• Started at 2024-11-24T16:30:30Z

[codecov]

Codecov Report

Attention: Patch coverage is 91.22807% with 10 lines in your changes missing coverage. Please review.

Project coverage is 68.58%. Comparing base (e3165c4) to head (b0ee29f).
Report is 566 commits behind head on main.

Files with missing lines	Patch %	Lines
...lockifier/src/execution/syscalls/hint_processor.rs	87.50%	3 Missing and 4 partials ⚠️
crates/blockifier/src/execution/syscalls/mod.rs	81.25%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2251       +/-   ##
===========================================
+ Coverage   40.10%   68.58%   +28.48%     
===========================================
  Files          26      109       +83     
  Lines        1895    13931    +12036     
  Branches     1895    13931    +12036     
===========================================
+ Hits          760     9555     +8795     
- Misses       1100     3965     +2865     
- Partials       35      411      +376     

☔ 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

[Yoni-Starkware]

Prep for syscall code sharing

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @meship-starkware)

[amosStarkware]

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

---

-- commits line 2 at r1:
even though this is a refactor, it would have been better to split this PR. I should have asked you to split before I started reviewing

Code quote:

- b0ee29f: chore(blockifier): share syscall handler's fields

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 36 at r1 (raw file):

    // Additional information gathered during execution.
    pub read_values: Vec<Felt>,
    pub accessed_keys: HashSet<StorageKey, RandomState>,

why does this have a different type than in SyscallHintProcessor?

Code quote:

pub accessed_keys: HashSet<StorageKey, RandomState>,

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 43 at r1 (raw file):

    // The original storage value of the executed contract.
    // Should be moved back `context.revert_info` before executing an inner call.
    pub original_values: HashMap<StorageKey, Felt>,

NativeSyscallHandler didn't have this, right, as far as I can see - is this because revert in native is only implemented now?

Code quote:

pub original_values: HashMap<StorageKey, Felt>,

---

crates/blockifier/src/execution/native/syscall_handler.rs line 60 at r1 (raw file):

};
use crate::execution::syscalls::{exceeds_event_size_limit, syscall_base};
use crate::state::state_api::State;

why does this have to stay here instead of moving to base?

Code quote:

pub unrecoverable_error: Option<SyscallExecutionError>,

---

crates/blockifier/src/execution/native/syscall_handler.rs line 70 at r1 (raw file):

    ) -> NativeSyscallHandler<'state> {
        NativeSyscallHandler {
            base: syscall_base::SyscallHandlerBase::new(call, state, context),

why not import SyscallHandlerBase instead of using syscall_base::?
non blocking

Code quote:

base: syscall_base::SyscallHandlerBase::new(call, state, context),

[amosStarkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 23 at r1 (raw file):

/// This file is for sharing common logic between Native and VM syscall implementations.

pub struct SyscallHandlerBase<'state> {

Are there any methods which can move here?

Code quote:

pub struct SyscallHandlerBase<'state> {

[Yoni-Starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @meship-starkware)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 60 at r1 (raw file):

Previously, amosStarkware wrote…

why does this have to stay here instead of moving to base?

Native cannot return rust errors. The VM can.
We are handling this by keeping this field. It might be a good idea to share this behaviour with the VM, but not now - it is still under dev (in particular, we need to make the stack traces compatible with the VM's)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 70 at r1 (raw file):

Previously, amosStarkware wrote…

why not import SyscallHandlerBase instead of using syscall_base::?
non blocking

Leftovers of the prev design (bundle of functions instead of a shared impl)
I'm removing this in a future PR in the stack

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 23 at r1 (raw file):

Previously, amosStarkware wrote…

Are there any methods which can move here?

Sure, next PRs

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 36 at r1 (raw file):

Previously, amosStarkware wrote…

why does this have a different type than in SyscallHintProcessor?

IDK why native did that explicitly. Maybe for safety. You can see that this is the default state for hash sets:

pub struct HashSet<T, S = RandomState>

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 43 at r1 (raw file):

Previously, amosStarkware wrote…

NativeSyscallHandler didn't have this, right, as far as I can see - is this because revert in native is only implemented now?

Right

[amosStarkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 36 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

IDK why native did that explicitly. Maybe for safety. You can see that this is the default state for hash sets:

pub struct HashSet<T, S = RandomState>

The default RandomState is different:
https://doc.rust-lang.org/std/collections/hash_map/struct.RandomState.html
please check why it needs it be different / if it's ok that SyscallHintProcessor will now also use this type.

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @meship-starkware)

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 36 at r1 (raw file):

Previously, amosStarkware wrote…

The default RandomState is different:
https://doc.rust-lang.org/std/collections/hash_map/struct.RandomState.html
please check why it needs it be different / if it's ok that SyscallHintProcessor will now also use this type.

It's the same struct - I checked

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @meship-starkware)

---

crates/blockifier/src/execution/syscalls/syscall_base.rs line 36 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It's the same struct - I checked

It's not needed. I'll remove it in the next PR

[amosStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @meship-starkware, and @Yoni-Starkware)