refactor(blockifier): share get_block_hash syscall code between native and casm

[meship-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[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]

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

---

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

    {
        let out_of_range_error =
            Felt::from_hex(BLOCK_NUMBER_OUT_OF_RANGE_ERROR).map_err(SyscallExecutionError::from)?;

I am not sure this is the right way to handle the error here because, in the native implementation, we panicked.

Code quote:

        let out_of_range_error =
            Felt::from_hex(BLOCK_NUMBER_OUT_OF_RANGE_ERROR).map_err(SyscallExecutionError::from)?;

---

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

        let (key, block_hash_contract_address) =
            get_block_hash_base(current_block_number, block_number)
                .map_err(|e| self.handle_error(remaining_gas, e.into()))?;

I am not sure this is the right way to handle the error here.

Code quote:

.map_err(|e| self.handle_error(remaining_gas, e.into()))?;

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.32%. Comparing base (e3165c4) to head (fadd3b1).
Report is 466 commits behind head on main.

Files with missing lines	Patch %	Lines
.../blockifier/src/execution/syscalls/syscall_base.rs	88.46%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2107       +/-   ##
===========================================
+ Coverage   40.10%   77.32%   +37.22%     
===========================================
  Files          26      383      +357     
  Lines        1895    40180    +38285     
  Branches     1895    40180    +38285     
===========================================
+ Hits          760    31071    +30311     
- Misses       1100     6816     +5716     
- Partials       35     2293     +2258     

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

[meship-starkware]

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

---

crates/blockifier/cairo_native line 1 at r1 (raw file):

Subproject commit 0bc6f408884370e4fbbf421c4e8e109911c3d73e

This is a mistake. Working on it

Code quote:

Subproject commit 0bc6f408884370e4fbbf421c4e8e109911c3d73e

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewed 1 of 5 files at r1.
Reviewable status: 1 of 5 files reviewed, 8 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure this is the right way to handle the error here.

Why not?

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure this is the right way to handle the error here because, in the native implementation, we panicked.

Panic here as well. It is safe

---

crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs line 120 at r2 (raw file):

    };

    let call_info = entry_point_call.execute_directly(&mut state).unwrap();

Thanks :)

---

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

    OUT_OF_GAS_ERROR,
};
use crate::execution::syscalls::syscall_base::get_block_hash_base;

Suggestion:

ase;

---

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

        let current_block_number =
            self.context.tx_context.block_context.block_info().block_number.0;

Share this code as well

Code quote:

        if self.context.execution_mode == ExecutionMode::Validate {
            let err = SyscallExecutionError::InvalidSyscallInExecutionMode {
                syscall_name: "get_block_hash".to_string(),
                execution_mode: ExecutionMode::Validate,
            };
            return Err(self.handle_error(remaining_gas, err));
        }

        let current_block_number =
            self.context.tx_context.block_context.block_info().block_number.0;

---

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

        let (key, block_hash_contract_address) =
            get_block_hash_base(current_block_number, block_number)

Suggestion:

syscall_base::get_block_hash(

---

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

                .map_err(|e| self.handle_error(remaining_gas, e.into()))?;

        match self.state.get_storage_at(block_hash_contract_address, key) {

Share this as well

Code quote:

self.state.get_storage_at(block_hash_contract_address, key) {

---

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

    BLOCK_NUMBER_OUT_OF_RANGE_ERROR,
};

Document this file shortly.

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Share this code as well

Done.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Share this as well

Seems like it fails the native tests. I'll look into it.

---

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

    OUT_OF_GAS_ERROR,
};
use crate::execution::syscalls::syscall_base::get_block_hash_base;

Done.

---

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

        let (key, block_hash_contract_address) =
            get_block_hash_base(current_block_number, block_number)

Done.

[meship-starkware]

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

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Seems like it fails the native tests. I'll look into it.

NM seems OK in the CI, although it fails locally.
Done.

[Yoni-Starkware]

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

crates/blockifier/src/execution/syscalls/mod.rs line 400 at r3 (raw file):

    let block_hash = BlockHash(syscall_base::get_block_hash_base(
        syscall_handler.execution_mode(),
        current_block_number,

Pass the context instead.

Suggestion:

    let block_hash = BlockHash(syscall_base::get_block_hash_base(
        self.context,

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Why not?

Not relevant anymore

---

crates/blockifier/src/execution/syscalls/mod.rs line 400 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Pass the context instead.

Done.

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Document this file shortly.

Done.

[Yoni-Starkware]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @avi-starkware)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Done.

Not yet :) but non-blocking

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

---

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

/// This file is for sharing common logic between Native and Casm syscalls implementations.

Is this not enough documentation, or is it in the wrong place?

Code quote:

/// This file is for sharing common logic between Native and Casm syscalls implementations.

[Yoni-Starkware]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

[meship-starkware]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)