feat(blockifier): add NativeSyscallHandler

[varex83]

This PR is a copy of #549 but with changed "from" branch

---

This change is 

[noaov1]

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @varex83)

---

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

    pub state: &'state mut dyn State,
    pub execution_resources: &'state mut ExecutionResources,
    pub execution_context: &'state mut EntryPointExecutionContext,

For consistency

Suggestion:

    pub resources: &'state mut ExecutionResources,
    pub context: &'state mut EntryPointExecutionContext,

---

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

    pub caller_address: ContractAddress,
    pub contract_address: ContractAddress,
    pub entry_point_selector: Felt,

WDYT?

Suggestion:

    pub call: CallEntryPoint,

---

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

    pub contract_address: ContractAddress,
    pub entry_point_selector: Felt,

Writing here for the record additional field of the cairo1 vm hint proccessor:
read_only_segments
secp256k\r1_hint_processor
sha256_segment_end_ptr
hints
execution_info_ptr

---

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

    // Additional execution result info.
    pub storage_read_values: Vec<Felt>,
    pub accessed_storage_keys: HashSet<StorageKey, RandomState>,

For consistency

Suggestion:

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

---

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

        &mut self,
        entry_point: CallEntryPoint,
        remaining_gas: &mut u128,

Possible? For consistency
Than you don't need the method update_remaining_gas

Suggestion:

        remaining_gas: &mut u64,

---

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

        self.inner_calls.push(call_info.clone());

        Ok(call_info)

Can you please explain what is the call info used for?

Code quote:

        Ok(call_info)

---

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

        if *remaining_gas < required_gas {
            // Out of gas failure.
            return Err(vec![Felt::from_hex(OUT_OF_GAS_ERROR).unwrap()]);

Suggestion:

return Err(vec![Felt::from_hex(OUT_OF_GAS_ERROR).expect("An informative error message")]

[varex83]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

WDYT?

I would like to keep things like they are because the current approach will cover all the needed fields, so we don't store extra unnecessary things, and also it will keep getting those fields much simpler (without .call.<...>)

---

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

Previously, noaov1 (Noa Oved) wrote…

For consistency

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Possible? For consistency
Than you don't need the method update_remaining_gas

We can't do it in that way because, in any case, we need to convert the u128 pointer and to make it work correctly we utilize the update_remaining_gas function, converting with the u64::try_from(...) will create a new integer losing the old pointer

---

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

        if *remaining_gas < required_gas {
            // Out of gas failure.
            return Err(vec![Felt::from_hex(OUT_OF_GAS_ERROR).unwrap()]);

Done.

[codecov]

Codecov Report

Attention: Patch coverage is 11.01449% with 307 lines in your changes missing coverage. Please review.

Please upload report for BASE (native/add-native-execution-engine@0a88987). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...blockifier/src/execution/native/syscall_handler.rs	0.00%	220 Missing ⚠️
crates/blockifier/src/execution/contract_class.rs	2.35%	82 Missing and 1 partial ⚠️
crates/blockifier/src/execution/execution_utils.rs	0.00%	2 Missing ⚠️
crates/blockifier/src/execution/entry_point.rs	83.33%	1 Missing ⚠️
crates/blockifier/src/test_utils/contracts.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##             native/add-native-execution-engine     #621   +/-   ##
=====================================================================
  Coverage                                      ?   68.57%           
=====================================================================
  Files                                         ?       90           
  Lines                                         ?    11481           
  Branches                                      ?    11481           
=====================================================================
  Hits                                          ?     7873           
  Misses                                        ?     3218           
  Partials                                      ?      390           

Flag	Coverage Δ
	68.57% <11.01%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[meship-starkware]

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 5 unresolved discussions (waiting on @noaov1 and @varex83)

[noaov1]

Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @varex83)

---

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

we need to convert the u128 pointer

Where is it done?

[noaov1]

Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @varex83)

---

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

            return Err(vec![
                Felt::from_hex(OUT_OF_GAS_ERROR)
                    .expect("Failed to parse OUT_OF_GAS_ERROR hex string"),

Note to self: in the vm syscall handler we return an error in case the conversion failed.

Code quote:

                Felt::from_hex(OUT_OF_GAS_ERROR)
                    .expect("Failed to parse OUT_OF_GAS_ERROR hex string"),

---

crates/blockifier/src/execution/native/utils.rs line 17 at r4 (raw file):

Previously, varex83 (Bohdan Ohorodnii) wrote…

Yes, it will be used in the future PRs for the error handling, since we can only return errors as vector of felt's due to NativeSyscallHandler trait bounds

Why are those methods needed? We convert each specific error message separately, e.g..

---

crates/blockifier/src/execution/native/utils.rs line 17 at r5 (raw file):

pub fn encode_str_as_felts(msg: &str) -> Vec<Felt> {
    const CHUNK_SIZE: usize = 32;

Can you please explain? Why 32?

Code quote:

const CHUNK_SIZE: usize = 32;