feat(blockifier): define ChargedResources in CallInfo

[TzahiTaub]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/execution/call_info.rs line 111 at r1 (raw file):

impl ChargedResources {
    pub fn from_execution_resources(resources: ExecutionResources) -> Self {
        Self { vm_resources: resources, gas_for_fee: GasAmount::default() }

Suggestion:

..Default::default()

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

    /// From the total amount of steps available for execution, deduct the steps consumed during
    /// validation and the overhead steps required, among the rest, for fee transfer.

better?

Suggestion:

charged resources required 

---

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

            vm_resources: ExecutionResources::default(),
            gas_for_fee: GasAmount(gas_consumed),
        },

this is already an implementation, right?
please add @noaov1 as reviewer

Code quote:

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

[TzahiTaub]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @noaov1)

---

crates/blockifier/src/execution/call_info.rs line 111 at r1 (raw file):

impl ChargedResources {
    pub fn from_execution_resources(resources: ExecutionResources) -> Self {
        Self { vm_resources: resources, gas_for_fee: GasAmount::default() }

Done.

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, dorimedini-starkware wrote…

better?

Nope. I didn't change this comment because of this PR change, it's a by the way change - the "overhead steps" mentioned in the comment aren't only fee transfer related (e.g., every call have some constant steps from VC we charge here) so I just wanted to make it clearer. Actually from looking at the code, I don't really understand the connection to the fee transfer - do you know if the execute_txs_inner mapping has the fee transfer cost embedded in it?

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Nope. I didn't change this comment because of this PR change, it's a by the way change - the "overhead steps" mentioned in the comment aren't only fee transfer related (e.g., every call have some constant steps from VC we charge here) so I just wanted to make it clearer. Actually from looking at the code, I don't really understand the connection to the fee transfer - do you know if the execute_txs_inner mapping has the fee transfer cost embedded in it?

IDK, question for Noa or Meshi I think

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.25%. Comparing base (e3165c4) to head (f737291).
Report is 188 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1722       +/-   ##
===========================================
+ Coverage   40.10%   64.25%   +24.15%     
===========================================
  Files          26      135      +109     
  Lines        1895    17534    +15639     
  Branches     1895    17534    +15639     
===========================================
+ Hits          760    11267    +10507     
- Misses       1100     5475     +4375     
- Partials       35      792      +757     

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

[TzahiTaub]

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

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, dorimedini-starkware wrote…

IDK, question for Noa or Meshi I think

@noaov1 if you may

[github-actions]

Artifacts upload triggered. View details here

[noaov1]

Reviewed 3 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

a discussion (no related file):
How do you plan to be backward compatible? Is that an issue?

---

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

@noaov1 if you may

It does.

---

crates/blockifier/src/execution/entry_point_execution.rs line 413 at r2 (raw file):

    let full_call_resources = &*syscall_handler.resources - &previous_resources;
    let charged_resources = ChargedResources {
        vm_resources: full_call_resources.filter_unused_builtins(),

I'm a bit confused.
Do we always charge for the vm resources? What about when the tracked resource is a sierra gas?

Code quote:

 vm_resources: full_call_resources.filter_unused_builtins()

---

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

Previously, dorimedini-starkware wrote…

this is already an implementation, right?
please add @noaov1 as reviewer

@TzahiTaub - why do you set here the gas_for_fee as the gas_consumed

[noaov1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

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

Previously, noaov1 (Noa Oved) wrote…

@TzahiTaub - why do you set here the gas_for_fee as the gas_consumed

Opps, please ignore.

[TzahiTaub]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @noaov1)

a discussion (no related file):

Previously, noaov1 (Noa Oved) wrote…

How do you plan to be backward compatible? Is that an issue?

With what? txs with L2 gas are new for this version. The gas_consumed stays and before and is compatible with the OS. And VM mode txs will stay unchanged.

---

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

It does.

This is not related to the PR (but the comment) but does the fee_transfer resources cost change between transactions? Assuming it doesn't, why not have a separate resource constant for the fee transfer, instead of embedding it in every tx resources?

---

crates/blockifier/src/execution/entry_point_execution.rs line 413 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I'm a bit confused.
Do we always charge for the vm resources? What about when the tracked resource is a sierra gas?

This is temporary, We'll have either vm_resources update or gas_for_fee update per call (gas_consumed will always be updated).

[TzahiTaub]

Reviewable status: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

Opps, please ignore.

Done.

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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

[noaov1]

Reviewed 4 of 11 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

a discussion (no related file):

Previously, TzahiTaub (Tzahi) wrote…

With what? txs with L2 gas are new for this version. The gas_consumed stays and before and is compatible with the OS. And VM mode txs will stay unchanged.

Oh, I see. Will only transactions that have signed non-trivial L2 gas bounds run in Sierra gas mode?
Where is this indicated here?

---

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This is not related to the PR (but the comment) but does the fee_transfer resources cost change between transactions? Assuming it doesn't, why not have a separate resource constant for the fee transfer, instead of embedding it in every tx resources?

I am not sure. It should not.
What will we benefit from the separation?

---

crates/blockifier/src/execution/entry_point_execution.rs line 413 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This is temporary, We'll have either vm_resources update or gas_for_fee update per call (gas_consumed will always be updated).

According to the tracked resources mode?

[TzahiTaub]

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

a discussion (no related file):

Previously, noaov1 (Noa Oved) wrote…

Oh, I see. Will only transactions that have signed non-trivial L2 gas bounds run in Sierra gas mode?
Where is this indicated here?

It in a future PR (you're on it) - https://reviewable.io/reviews/starkware-libs/sequencer/1771#-

---

---

crates/blockifier/src/execution/entry_point.rs line 363 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I am not sure. It should not.
What will we benefit from the separation?

One point of truth for the fee transfer cost, it will also be clear from the file what part is of the fee transfer and what cost comes directly from the tx "behavior".

---

crates/blockifier/src/execution/entry_point_execution.rs line 413 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

According to the tracked resources mode?

Yes, according to the tracked resource either the VM resource will be updated, or the fee_for_gas.

[dorimedini-starkware]

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

[noaov1]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)