feat(blockifier): add l2_gas to the receipt

[TzahiTaub]

No description provided.

[reviewable-StarkWare]

This change is 

[TzahiTaub]

• feat(blockifier): add l2_gas to the receipt #2031 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 75.38462% with 16 lines in your changes missing coverage. Please review.

Project coverage is 69.40%. Comparing base (e3165c4) to head (cb2409c).
Report is 457 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/fee/resources.rs	37.50%	8 Missing and 2 partials ⚠️
crates/blockifier/src/execution/call_info.rs	73.68%	4 Missing and 1 partial ⚠️
.../blockifier/src/transaction/account_transaction.rs	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2031       +/-   ##
===========================================
+ Coverage   40.10%   69.40%   +29.29%     
===========================================
  Files          26      106       +80     
  Lines        1895    13902    +12007     
  Branches     1895    13902    +12007     
===========================================
+ Hits          760     9648     +8888     
- Misses       1100     3849     +2749     
- Partials       35      405      +370     

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

[dorimedini-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @TzahiTaub and @Yoni-Starkware)

---

crates/starknet_api/src/execution_resources.rs line 15 at r1 (raw file):

)]
#[derive(
    derive_more::Add,

no, be explicit: checked_add or saturating_add.
apparently when building with --release mode, integer overflows are silently wrapped by default - not what we want

Code quote:

derive_more::Add,

---

crates/starknet_api/src/execution_resources.rs line 73 at r1 (raw file):

#[cfg_attr(any(test, feature = "testing"), derive(derive_more::Sum, derive_more::AddAssign))]
#[derive(derive_more::Add, Clone, Copy, Debug, Default, Eq, PartialEq, Deserialize, Serialize)]

ditto

Code quote:

derive_more::Add,

---

crates/blockifier/src/fee/resources.rs line 75 at r1 (raw file):

            self.n_reverted_steps,
            computation_mode,
        ) + GasVector::from_l2_gas(self.l2_gas + self.reverted_l2_gas)

checked or saturating (see below)

Code quote:

 + GasVector

---

crates/blockifier/src/fee/receipt.rs line 34 at r1 (raw file):

    execution_summary_without_fee_transfer: ExecutionSummary,
    execution_resources: &'a ExecutionResources,
    l2_gas: GasAmount,

see above: these should be combined in a struct IMO

Code quote:

    execution_resources: &'a ExecutionResources,
    l2_gas: GasAmount,

---

crates/blockifier/src/fee/receipt.rs line 93 at r1 (raw file):

                n_reverted_steps: reverted_steps,
                l2_gas,
                reverted_l2_gas: GasAmount(0), // TODO(tzahi): compute value.

from what? how?

Code quote:

/ TODO(tzahi): compute value.

---

crates/blockifier/src/transaction/transaction_execution.rs line 160 at r1 (raw file):

            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

code dup (see above)

Code quote:

        let l2_gas = match &execute_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

---

crates/blockifier/src/fee/gas_usage_test.rs line 366 at r1 (raw file):

    let n_reverted_steps = 15;
    let l2_gas = GasAmount(0);
    let reverted_l2_gas = GasAmount(0);

why zero values...? not supported yet? how will you remember to set these to nonzero values?

Code quote:

    let l2_gas = GasAmount(0);
    let reverted_l2_gas = GasAmount(0);

---

crates/blockifier/src/blockifier/stateful_validator.rs line 131 at r1 (raw file):

            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

should be equivalent, non-blocking

Suggestion:

        let gas_for_fee = validate_call_info
            .map(|call_info| call_info.charged_resources.gas_for_fee)
            .unwrap_or(GasAmount(0));

---

crates/blockifier/src/blockifier/stateful_validator.rs line 143 at r1 (raw file):

                .get_actual_state_changes()?,
            &execution_resources,
            gas_for_fee,

didn't you combine these into a single "ChargedResources" struct?

Code quote:

            &execution_resources,
            gas_for_fee,

---

crates/blockifier/src/transaction/account_transaction.rs line 589 at r1 (raw file):

            validate_call_info = self.handle_validate_tx(
                state,
                &mut resources,

why don't you mutate a gas amount, to align behavior to the resources?
strange, the different approaches to the same concept (non-blocking)

Code quote:

&mut resources,

---

crates/blockifier/src/transaction/account_transaction.rs line 605 at r1 (raw file):

            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

1. see above for style (.map(..).unwrap_or(..)), non-blocking
2. checked or saturating Add (see below)

Code quote:

        let gas_for_fee = match &validate_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        } + match &execute_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

---

crates/blockifier/src/transaction/account_transaction.rs line 605 at r1 (raw file):

            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

some logic is duplicated... I suggest you implement a fn gas_for_fee_from_call_infos(validate: Option<&CallInfo>, execute: Option<&CallInfo>) -> GasAmount which does this, + use it above in the stateful validator (with None as the execute callinfo)

Code quote:

        let gas_for_fee = match &validate_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        } + match &execute_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

---

crates/blockifier/src/transaction/account_transaction.rs line 674 at r1 (raw file):

            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

duplicate logic (see above)

Code quote:

        let validate_gas_for_fee = match &validate_call_info {
            Some(call_info) => call_info.charged_resources.gas_for_fee,
            None => GasAmount(0),
        };

---

crates/blockifier/src/transaction/account_transaction.rs line 687 at r1 (raw file):

            validate_gas_for_fee,
            CallInfo::summarize_many(validate_call_info.iter()),
            execution_steps_consumed,

3 of the 4 fields here - along with reverted L2 gas - already have their own struct, right? can you use it?

Code quote:

            &resources,
            validate_gas_for_fee,
            CallInfo::summarize_many(validate_call_info.iter()),
            execution_steps_consumed,

---

crates/blockifier/src/transaction/account_transaction.rs line 706 at r1 (raw file):

                            Some(call_info) => call_info.charged_resources.gas_for_fee,
                            None => GasAmount(0),
                        },

duplicate logic (see above) - although the first amount is already computed, the second amount computation is duped

Code quote:

                    validate_gas_for_fee
                        + match &execute_call_info {
                            Some(call_info) => call_info.charged_resources.gas_for_fee,
                            None => GasAmount(0),
                        },

---

crates/blockifier/src/transaction/account_transaction.rs line 718 at r1 (raw file):

                    &tx_receipt,
                    charge_fee,
                )?;

what does the post-execution check do with the L2 gas for fee? I would expect a bounds check

Code quote:

                let post_execution_report = PostExecutionReport::new(
                    &mut execution_state,
                    &tx_context,
                    &tx_receipt,
                    charge_fee,
                )?;

[Yoni-Starkware]

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

---

crates/blockifier/src/fee/receipt.rs line 34 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see above: these should be combined in a struct IMO

Please rename (sierra_gas or something else).
L2 gas is too general (and part of the receipt anyway)

[TzahiTaub]

Reviewable status: 0 of 8 files reviewed, 14 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 143 at r1 (raw file):

Previously, dorimedini-starkware wrote…

didn't you combine these into a single "ChargedResources" struct?

I have it for calls, not for a tx. Tried my best, the preparation of the input with all of the Optional structs is a bit annoying.

---

crates/blockifier/src/fee/gas_usage_test.rs line 366 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why zero values...? not supported yet? how will you remember to set these to nonzero values?

Actually not, didn't see what this test is doing. Done.

---

crates/blockifier/src/fee/receipt.rs line 34 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please rename (sierra_gas or something else).
L2 gas is too general (and part of the receipt anyway)

Done.

---

crates/blockifier/src/fee/receipt.rs line 93 at r1 (raw file):

Previously, dorimedini-starkware wrote…

from what? how?

In a few PRs, the revert mechanism will send the reverted gas together with the error.

---

crates/blockifier/src/fee/resources.rs line 75 at r1 (raw file):

Previously, dorimedini-starkware wrote…

checked or saturating (see below)

Done. I don't see we have a similar check in the get_vm_resources_cost computation.

---

crates/blockifier/src/transaction/account_transaction.rs line 589 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why don't you mutate a gas amount, to align behavior to the resources?
strange, the different approaches to the same concept (non-blocking)

I agree this is weird, but it goes very deep into the current implementation, we can think of refactoring this. But I think the difference is due to the fact we ignore the remaining_gas for some of the calls. resources should be the sum of execution_resources in the calls "so far", but this is not the case for remaining_gas and gas_for_fee which is sort of the equivalent.

---

crates/blockifier/src/transaction/account_transaction.rs line 605 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. see above for style (.map(..).unwrap_or(..)), non-blocking
2. checked or saturating Add (see below)

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 605 at r1 (raw file):

Previously, dorimedini-starkware wrote…

some logic is duplicated... I suggest you implement a fn gas_for_fee_from_call_infos(validate: Option<&CallInfo>, execute: Option<&CallInfo>) -> GasAmount which does this, + use it above in the stateful validator (with None as the execute callinfo)

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 674 at r1 (raw file):

Previously, dorimedini-starkware wrote…

duplicate logic (see above)

Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 687 at r1 (raw file):

Previously, dorimedini-starkware wrote…

3 of the 4 fields here - along with reverted L2 gas - already have their own struct, right? can you use it?

I can, but it will be a bit of misuse as ChargedResources is currently a field of CallInfo representing the resources of a call. The

---

crates/blockifier/src/transaction/account_transaction.rs line 706 at r1 (raw file):

Previously, dorimedini-starkware wrote…

duplicate logic (see above) - although the first amount is already computed, the second amount computation is duped

This OK?

---

crates/blockifier/src/transaction/account_transaction.rs line 718 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what does the post-execution check do with the L2 gas for fee? I would expect a bounds check

It does check it.

---

crates/blockifier/src/transaction/transaction_execution.rs line 160 at r1 (raw file):

Previously, dorimedini-starkware wrote…

code dup (see above)

Done.

---

crates/starknet_api/src/execution_resources.rs line 15 at r1 (raw file):

Previously, dorimedini-starkware wrote…

no, be explicit: checked_add or saturating_add.
apparently when building with --release mode, integer overflows are silently wrapped by default - not what we want

Done.

---

crates/starknet_api/src/execution_resources.rs line 73 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ditto

Done.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

---

crates/blockifier/src/fee/resources.rs line 75 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Done. I don't see we have a similar check in the get_vm_resources_cost computation.

yes, step amount is still a usize... but gas counting will be with us forever so better do the right thing for gas at least

---

crates/blockifier/src/transaction/account_transaction.rs line 663 at r2 (raw file):

            remaining_gas,
        );
        let gas_for_fee = gas_for_fee_from_call_infos(&validate_call_info, &None);

if you don't want to pass Nones everywhere you can change the signature of this function to accept a &[&Option<CallInfo>]. non-blocking

Code quote:

gas_for_fee_from_call_infos(&validate_call_info, &None);

---

crates/blockifier/src/execution/call_info.rs line 135 at r2 (raw file):

    })
}
/// Represents the full effects of executing an entry point, including the inner calls it invoked.

newline (non-blocking)

Suggestion:

}

/// Represents the full effects of executing an entry point, including the inner calls it invoked.

[TzahiTaub]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 663 at r2 (raw file):

Previously, dorimedini-starkware wrote…

if you don't want to pass Nones everywhere you can change the signature of this function to accept a &[&Option<CallInfo>]. non-blocking

I think I prefer the explicit params.

[github-actions]

Artifacts upload triggered. View details here

[TzahiTaub]

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

[dorimedini-starkware]

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