feat(blockifier): add call_contract cairo native syscall

[PearsonWhite]

Implements call_contract in NativeSyscallHandler.

[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

[PearsonWhite]

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

a discussion (no related file):
In test_track_resources, the logic for what we expect has changed. Now, I'm not sure if the result is expected or not and I need clarification.

Previously, both the outer call and the inner call had to be (Cairo1 | Native) for the inner tracked_resource to be SierraGas.
Now, long as the inner is (Cairo1 | Native) I think SierraGas should be used.

Currently, one case fails: when outer is Cairo0 and inner is Cairo1, SierraGas is expected, but CairoSteps is used.
I've added a complete table of the results here in csv format:

Code snippet:

outer , inner , expected_outer , expected_inner , actual_outer , actual_expected , pass/fail
Cairo0 , Cairo0 , CairoSteps , CairoSteps , CairoSteps , CairoSteps , pass
Cairo0 , Cairo1 , CairoSteps , SierraGas , CairoSteps , CairoSteps , failed <--- Should this fail? Or is it expected?
Cairo1 , Cairo0 , SierraGas , CairoSteps , SierraGas , CairoSteps , pass
Cairo1 , Cairo1 , SierraGas , SierraGas , SierraGas , SierraGas , pass
Cairo0 , Native , CairoSteps , SierraGas , CairoSteps , SierraGas , pass
Native , Cairo1 , SierraGas , SierraGas , SierraGas , SierraGas , pass
Native , Cairo0 , SierraGas , CairoSteps , SierraGas , CairoSteps , pass
Cairo1 , Native , SierraGas , SierraGas , SierraGas , SierraGas , pass
Native , Native , SierraGas , SierraGas , SierraGas , SierraGas , pass

---

[varex83]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PearsonWhite)

---

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

        let contract_address = ContractAddress::try_from(address)
            .expect("Failed to convert address argument to a ContractAddress");

Don't use expect, return runtime error instead

[varex83]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @PearsonWhite)

---

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

            caller_address: self.contract_address,
            call_type: CallType::Call,
            initial_gas: u64::try_from(*remaining_gas).unwrap(),

Use expect instead of unwrap here

[github-actions]

Artifacts upload triggered. View details here

[PearsonWhite]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @varex83)

---

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

Previously, varex83 (Bohdan Ohorodnii) wrote…

Don't use expect, return runtime error instead

Done.

---

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

Previously, varex83 (Bohdan Ohorodnii) wrote…

Use expect instead of unwrap here

Done.

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.55%. Comparing base (e63d1b9) to head (09de68c).

Additional details and impacted files

@@                     Coverage Diff                     @@
##           rdr/native-storage-read    #1548      +/-   ##
===========================================================
+ Coverage                    68.48%   68.55%   +0.06%     
===========================================================
  Files                          102      102              
  Lines                        13662    13687      +25     
  Branches                     13662    13687      +25     
===========================================================
+ Hits                          9357     9383      +26     
+ Misses                        3904     3903       -1     
  Partials                       401      401              

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

[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 2 files reviewed, 3 unresolved discussions (waiting on @noaov1 and @varex83)

a discussion (no related file):

Previously, PearsonWhite wrote…

In test_track_resources, the logic for what we expect has changed. Now, I'm not sure if the result is expected or not and I need clarification.

Previously, both the outer call and the inner call had to be (Cairo1 | Native) for the inner tracked_resource to be SierraGas.
Now, long as the inner is (Cairo1 | Native) I think SierraGas should be used.

Currently, one case fails: when outer is Cairo0 and inner is Cairo1, SierraGas is expected, but CairoSteps is used.
I've added a complete table of the results here in csv format:

If I am not mistaken, if cairo0 contract calls cairo1 contract, we have decided to use the VM resources (meaning we do not use native in these cases), so both the outer and inner should be cairo steps in this case. @noaov1 could you please confirm

---

[github-actions]

Artifacts upload triggered. View details here

[PearsonWhite]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @varex83)

a discussion (no related file):

Previously, meship-starkware (Meshi Peled) wrote…

If I am not mistaken, if cairo0 contract calls cairo1 contract, we have decided to use the VM resources (meaning we do not use native in these cases), so both the outer and inner should be cairo steps in this case. @noaov1 could you please confirm

This has been changed in a recent rebase. Now I think it is working as expected. See updated logic/comments in crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs.

---

[github-actions]

Artifacts upload triggered. View details here

[PearsonWhite]

Dismissed @varex83 from 2 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @noaov1)

Fixed

[meship-starkware]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[github-actions]

Artifacts upload triggered. View details here

[noaov1]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PearsonWhite)

a discussion (no related file):

Previously, PearsonWhite wrote…

This has been changed in a recent rebase. Now I think it is working as expected. See updated logic/comments in crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs.

@meship-starkware you are right. Wrote some comment below.

---

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 193 at r4 (raw file):

    #[cfg(feature = "cairo_native")]
    // If inner is Native, we expect inner to use SierraGas.
    // This overrides previous logic.

If the outer call uses cairo steps, then we choose to run all the inner calls also with VM resources.
This should not work, see here.

Code quote:

    // If inner is Native, we expect inner to use SierraGas.
    // This overrides previous logic.

[github-actions]

Artifacts upload triggered. View details here

[PearsonWhite]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 193 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

If the outer call uses cairo steps, then we choose to run all the inner calls also with VM resources.
This should not work, see here.

After rebasing, this no longer worked. Since this shouldn't have worked, that means the rebase fixed the execution logic, leaving only the test wrong. I've changed the logic for the test to match the VM. It should all be working as expected and passing now.