feat(blockifier): add syscall counting for native syscall handler

[rodrigo-pino]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.51%. Comparing base (e3165c4) to head (3d4ab3d).
Report is 274 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1555       +/-   ##
===========================================
+ Coverage   40.10%   68.51%   +28.41%     
===========================================
  Files          26      102       +76     
  Lines        1895    13687    +11792     
  Branches     1895    13687    +11792     
===========================================
+ Hits          760     9378     +8618     
- Misses       1100     3908     +2808     
- Partials       35      401      +366     

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

---

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

            gas_consumed,
        },
        resources: syscall_handler.resources.clone(),

Why are we no longer using default resources?

Code quote:

resources: syscall_handler.resources.clone(),

---

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

        let syscall_count = self.syscall_counter.entry(*selector).or_default();
        *syscall_count += n
    }

Why are we increasing by n and not by 1? Are there cases where we want to increase it with a different number?

Code quote:

    fn increment_syscall_count_by(&mut self, selector: &SyscallSelector, n: usize) {
        let syscall_count = self.syscall_counter.entry(*selector).or_default();
        *syscall_count += n
    }

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 2 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rodrigo-pino)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

When running cairo native, the only resource that matters is the gas counter. Please revert.

Done, i've change it default to ChargedResources

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 65 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

WDYM?

No longer relevant

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 74 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

WDYM?

I think doesn't show up in the code anymore

---

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

Previously, noaov1 (Noa Oved) wrote…

Currently, we need n that is potentially greater for one in the keccak syscall.

Yes, that's it. Didn't want to add an increment_syscall_count_by_one, but if you prefer it that way we can add a specialized function

---

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

Previously, noaov1 (Noa Oved) wrote…

With this change the method can no linger be called substract_syscall_gas_cost. Consider renaming it or, alternatively, removing this functionality.

Done, tell me what you think!

---

a.xadfas line at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

What is this file? Can you remove it?

Oooooops! Removed!

[noaov1]

Reviewed 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

---

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

    /// handling all gas-related logics and additional metadata such as `SyscallCounter`). The
    /// difference for native execution is that we need to explicitly call this method at the
    /// beginning of each syscall.

Suggestion:

    /// Handles all gas-related logics and additional metadata such as `SyscallCounter`. In native,
    /// we need to explicitly call this method at the
    /// beginning of each syscall.

---

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

            gas_consumed,
        },
        charged_resources: ChargedResources::default(),

We charge according to the gas consumed in the native run

Suggestion:

charged_resources: ChargedResources {
            vm_resources: ExecutionResources::default(),
            gas_for_fee: GasAmount(gas_consumed),
        },
,

[meship-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rodrigo-pino)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

Yes, that's it. Didn't want to add an increment_syscall_count_by_one, but if you prefer it that way we can add a specialized function

No, that's ok. I think it is better this way.

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

We charge according to the gas consumed in the native run

Done

---

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

    /// handling all gas-related logics and additional metadata such as `SyscallCounter`). The
    /// difference for native execution is that we need to explicitly call this method at the
    /// beginning of each syscall.

Done Love the writing tips :)

[meship-starkware]

Reviewed 2 of 2 files at r5.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1)

[meship-starkware]

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

[noaov1]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rodrigo-pino)