feat(blockifier): add NativeSyscallHandler
#621
Conversation
…call-handler # Conflicts: # crates/blockifier/src/execution/native/utils.rs
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1.
Reviewable status: 1 of 5 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @varex83)
crates/blockifier/src/execution/native/utils.rs line 17 at r4 (raw file):
pub fn encode_str_as_felts(msg: &str) -> Vec<Felt> { const CHUNK_SIZE: usize = 32;
Are these methods only for error handling, or will they have another usage in future prs?
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @meship-starkware)
crates/blockifier/src/execution/native/utils.rs line 17 at r4 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Are these methods only for error handling, or will they have another usage in future prs?
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
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 5 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @varex83)
There was a problem hiding this comment.
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")]
There was a problem hiding this comment.
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 methodupdate_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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This PR is a copy of #549 but with changed "from" branch
This change is