chore(blockifier): have charge_fee flag represent enforce_fee return value

[avivg-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.34%. Comparing base (b0cfe82) to head (c62db67).
Report is 339 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
- Coverage   74.18%   71.34%   -2.85%     
==========================================
  Files         359       94     -265     
  Lines       36240    12742   -23498     
  Branches    36240    12742   -23498     
==========================================
- Hits        26886     9091   -17795     
+ Misses       7220     3263    -3957     
+ Partials     2134      388    -1746     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[meship-starkware]

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 273 at r1 (raw file):

    thread::scope(|s| {
        s.spawn(move || {
            let charge_fee_1 = account_tx_1.create_tx_info().enforce_fee(); // aviv: todo better implementation?

remove all comments that start with aviv

[meship-starkware]

Reviewed 4 of 11 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avivg-starkware)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 164 at r2 (raw file):

#[rstest]
fn test_fee_enforcement(

We need to consider this test's relevance with the new behavior. I think this is still relevant because max fee 0 still has a special
behavior @noaov1 WDYT?

---

crates/blockifier/src/transaction/account_transactions_test.rs line 182 at r2 (raw file):

    let account_tx = AccountTransaction::DeployAccount(deploy_account_tx);
    let enforce_fee = account_tx.create_tx_info().enforce_fee();

Suggestion:

let charge_fee = account_tx.create_tx_info().enforce_fee();

---

crates/blockifier/src/transaction/account_transactions_test.rs line 190 at r2 (raw file):

#[case(TransactionVersion::ZERO)]
#[case(TransactionVersion::ONE)]
#[case(TransactionVersion::THREE)]

I believe this test is irrelevant now. We can add the is_reverted check to the test_fee_enforcement @noaov1 WDYT?

---

crates/blockifier/src/transaction/test_utils.rs line 306 at r2 (raw file):

        true,
    ) // aviv: true, true
}

I don't feel strongly about it, but I think it's more readable.

Suggestion:

    let tx = account_invoke_tx(invoke_args.clone());
    let tx_context = Arc::new(block_context.to_tx_context(&tx));
    let charge_fee = tx_context.tx_info.enforce_fee();
    account_invoke_tx(invoke_args).execute(
        state,
        block_context,
        charge_fee,
        true,
    ) // aviv: true, true
}

---

crates/blockifier/src/transaction/transactions_test.rs line 1614 at r2 (raw file):

    // All tx has default resource bounds, thus the default charge fee is the same for all. (aviv)
    let default_charge_fee = account_tx.create_tx_info().enforce_fee();

why default_charge_fee? is it because we know that max_fre/ resource_bounds are Zero?

[avivg-starkware]

Reviewable status: 2 of 11 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 273 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

remove all comments that start with aviv

Done.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 164 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

We need to consider this test's relevance with the new behavior. I think this is still relevant because max fee 0 still has a special
behavior @noaov1 WDYT?

hi just sanity check to make sure i understand: this test asserts that when max_fee/resource_bounds is 0 the execution succeeds and when it is 1 - execution fails. If so - where is the special case w max_fee = 0 can be found in the code?

---

crates/blockifier/src/transaction/transactions_test.rs line 1614 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

why default_charge_fee? is it because we know that max_fre/ resource_bounds are Zero?

Yes Exactly. do you think it's better to just use "charge_fee" i thought it might be more indicative in this case since "account_tx" (which we're using) was created with "default_args"

[meship-starkware]

Reviewed 4 of 9 files at r3, 4 of 6 files at r4, all commit messages.
Reviewable status: 9 of 11 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/transaction/transactions_test.rs line 1614 at r2 (raw file):

Previously, avivg-starkware wrote…

Yes Exactly. do you think it's better to just use "charge_fee" i thought it might be more indicative in this case since "account_tx" (which we're using) was created with "default_args"

I think it is good with the comment. It is better than just 'charge_fee' because you use it for multiple txes.
,

[meship-starkware]

Reviewed 2 of 6 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[meship-starkware]

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

---

crates/blockifier/src/transaction/account_transactions_test.rs line 164 at r2 (raw file):

Previously, avivg-starkware wrote…

hi just sanity check to make sure i understand: this test asserts that when max_fee/resource_bounds is 0 the execution succeeds and when it is 1 - execution fails. If so - where is the special case w max_fee = 0 can be found in the code?

The special behavior is that we do not fail a transaction in the pre-validation test if 'enforce_fee' is false (meaning max_fee/ resource_bounds is 0), but if it is true and max_fee < actual_fee, then the transaction will fail. That is what this test checks.

[meship-starkware]

Reviewable status: 0 of 11 files reviewed, 4 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/transaction/transactions_test.rs line 120 at r5 (raw file):

    FeeType,
    HasRelatedFeeType,
    TransactionExecutionInfo,

TransactionInfoCreator was here

---

crates/blockifier/src/transaction/transactions_test.rs line 142 at r5 (raw file):

};
use crate::transaction::transaction_types::TransactionType;
use crate::transaction::transactions::{ExecutableTransaction, L1HandlerTransaction, TransactionInfoCreator};

TransactionInfoCreator remove from here

[avivg-starkware]

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

---

crates/blockifier/src/transaction/account_transactions_test.rs line 164 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

The special behavior is that we do not fail a transaction in the pre-validation test if 'enforce_fee' is false (meaning max_fee/ resource_bounds is 0), but if it is true and max_fee < actual_fee, then the transaction will fail. That is what this test checks.

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1614 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think it is good with the comment. It is better than just 'charge_fee' because you use it for multiple txes.
,

OK thanks

---

crates/blockifier/src/transaction/transactions_test.rs line 120 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

TransactionInfoCreator was here

you mean that it's better to import from "objects" rather than "transactions"?
any way for just in case this is what I did.

---

crates/blockifier/src/transaction/transactions_test.rs line 142 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

TransactionInfoCreator remove from here

Done

[meship-starkware]

Reviewed 5 of 11 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[noaov1]

Please open a python PR to test the code

Reviewed 3 of 11 files at r5, 4 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)

---

crates/blockifier/src/blockifier/transaction_executor.rs line 107 at r6 (raw file):

            self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
        );
        let tx_charge_fee = tx.create_tx_info().enforce_fee();

Consider creating a method for Transaction and AccountTransaction

Code quote:

let tx_charge_fee = tx.create_tx_info().enforce_fee();

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 277 at r6 (raw file):

        s.spawn(move || {
            let charge_fee_1 = account_tx_1.create_tx_info().enforce_fee();
            let result = account_tx_1.execute(&mut state_1, &block_context_1, charge_fee_1, true);

I think that you can leave it true here and below. This flag is not relevant for this test.

Code quote:

            let charge_fee_1 = account_tx_1.create_tx_info().enforce_fee();
            let result = account_tx_1.execute(&mut state_1, &block_context_1, charge_fee_1, true);

---

crates/blockifier/src/execution/stack_trace_test.rs line 624 at r6 (raw file):

    let re = Regex::new(r"pc=0:[0-9]+").unwrap();
    let cleaned_expected_error = &re.replace_all(&expected_error, "pc=0:*");
    let charge_fee = account_tx.create_tx_info().enforce_fee();

Please revert (see comment above)

Code quote:

    let charge_fee = account_tx.create_tx_info().enforce_fee();

---

crates/blockifier/src/execution/stack_trace_test.rs line 688 at r6 (raw file):

    // Compare expected and actual error.
    let charge_fee = deploy_account_tx.create_tx_info().enforce_fee();

Please revert

Code quote:

    let charge_fee = deploy_account_tx.create_tx_info().enforce_fee()

---

crates/blockifier/src/transaction/account_transactions_test.rs line 190 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I believe this test is irrelevant now. We can add the is_reverted check to the test_fee_enforcement @noaov1 WDYT?

I suggested a modification :)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 206 at r6 (raw file):

    .unwrap();
    assert!(!tx_execution_info.is_reverted());
    assert_eq!(tx_execution_info.receipt.fee, Fee(0));

Suggestion:

assert!(tx_execution_info.fee_transfer_info.is_none());

---

crates/blockifier/src/transaction/account_transactions_test.rs line 615 at r6 (raw file):

    let initial_balance = state.get_fee_token_balance(deploy_address, fee_token_address).unwrap();

    let charge_fee = deploy_account_tx.create_tx_info().enforce_fee();

Please revert to charge_fee = true in the rest of the file

Code quote:

    let charge_fee = deploy_account_tx.create_tx_info().enforce_fee();

---

crates/blockifier/src/transaction/test_utils.rs line 307 at r6 (raw file):

    let charge_fee = tx_context.tx_info.enforce_fee();

    account_invoke_tx(invoke_args).execute(state, block_context, charge_fee, true)

Please avoid calling twice to account_invoke_tx.

Code quote:

    let tx = account_invoke_tx(invoke_args.clone());
    let tx_context = Arc::new(block_context.to_tx_context(&tx));
    let charge_fee = tx_context.tx_info.enforce_fee();

    account_invoke_tx(invoke_args).execute(state, block_context, charge_fee, true)

[meship-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 277 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I think that you can leave it true here and below. This flag is not relevant for this test.

This test checks a flow that came from the gateway, right? In this case, charge_fee should always be equal to enforce_fee. Moreover, the test won't pass otherwise, as the resource_bounds are zero. If it also represents a flow of full node transactions, then I agree we should change the resource_bounds to a non-default one.

On another note, we did change the behavior of transactions on the full node side. Now, if they send a transaction with charge_fee true and enforce_fee false, the transaction will fail

[avivg-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 107 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider creating a method for Transaction and AccountTransaction

Done, it included creating some getter funcs.

Also, i left in comment under my implementation a "match implementation" alternative that didn't work for me. perhaps you'll have an idea? or better to leave as at is?

Also, I trivially implemented for L1Handler, with enforce_fee() -> false (for all cases), under the assumption this is not supposed to be called, and if so- false is the desired behaviour. I'd love to ask some questions face to face about L1Handler.

[meship-starkware]

Reviewable status: 3 of 15 files reviewed, 10 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 306 at r7 (raw file):

        //     TransactionVersion::THREE => self.resource_bounds().max_possible_fee() > Fee(0)
        // }
    }

I think the suggestion was something like this, but it might be a better idea to move enforce_fee to be a function of transaction and not transaction info. If that is the change you'd like to make, please do it in a separate PR

Suggestion:

    pub fn enforce_fee(&self) -> bool {
        self.create_tx_info().enforce_fee()
    }

---

crates/blockifier/src/transaction/transaction_execution.rs line 67 at r7 (raw file):

            Self::L1HandlerTransaction(tx) => tx.enforce_fee(),
        }
    }

I think this is better

Suggestion:

    pub fn enforce_fee(&self) -> bool {
        match self {
            Self::AccountTransaction(tx) => tx.enforce_fee(),
            Self::L1HandlerTransaction(tx) => false,
        }
    }

---

crates/blockifier/src/transaction/transactions.rs line 605 at r7 (raw file):

        // AvivG: todo understand this
        false
    }

See comment above

---

crates/starknet_api/src/executable_transaction.rs line 197 at r7 (raw file):

        (version, TransactionVersion),
        (resource_bounds, ValidResourceBounds),
        (max_fee, Fee),

@ArniStarkware is against this getter. Is there a reason you need it?
Anyway, I don't think it needs to be part of this PR

[meship-starkware]

Reviewable status: 3 of 15 files reviewed, 10 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/transaction/transaction_execution.rs line 67 at r7 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think this is better

Aviv, can you check if this function is needed? If L1Handler does not have max_fee, how did we handle it so far?
If it is needed, then maybe it is better to return an Option here, although an Option on a bool is ugly, so I'm not sure about it.

[avivg-starkware]

crates/blockifier/src/transaction/transaction_execution.rs line 67 at r10 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

can you remove the function as well

Done.

[meship-starkware]

Reviewed 5 of 12 files at r15, 8 of 8 files at r17, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/transaction/post_execution_test.rs line 123 at r17 (raw file):

    });
    let tx_info = approve_tx.create_tx_info();

Revert this file change

[avivg-starkware]

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

---

crates/blockifier/src/transaction/post_execution_test.rs line 123 at r17 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Revert this file change

you mean to remove this?

It is used later in line 154

Code snippet:

let tx_info = approve_tx.create_tx_info();
...
// Check the current balance, before next transaction.
let (balance, _) = state
    .get_fee_token_balance(account_address, chain_info.fee_token_address(&tx_info.fee_type()))
    .unwrap();

[meship-starkware]

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

---

crates/blockifier/src/transaction/post_execution_test.rs line 123 at r17 (raw file):

Previously, avivg-starkware wrote…

you mean to remove this?

It is used later in line 154

When I look at the changes in this file, I see that you just added a space. I meant removing the space between let tx_info= and let approval_execution_info =

[avivg-starkware]

Reviewable status: 8 of 15 files reviewed, 12 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/transaction/post_execution_test.rs line 123 at r17 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

When I look at the changes in this file, I see that you just added a space. I meant removing the space between let tx_info= and let approval_execution_info =

Done.

[meship-starkware]

Reviewed 5 of 7 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: 14 of 15 files reviewed, 11 unresolved discussions (waiting on @noaov1)

[meship-starkware]

Reviewed 5 of 6 files at r20, all commit messages.
Reviewable status: 13 of 15 files reviewed, 11 unresolved discussions (waiting on @noaov1)

[meship-starkware]

Reviewed 1 of 7 files at r18, 1 of 6 files at r20.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @noaov1)

[meship-starkware]

Reviewed 6 of 6 files at r21, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @noaov1)

[avivg-starkware]

python PR:
https://github.com/starkware-industries/starkware/pull/36055

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

[noaov1]

Reviewed 2 of 12 files at r15, 1 of 8 files at r17, 2 of 7 files at r18, 1 of 6 files at r20, 5 of 6 files at r21, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 280 at r21 (raw file):

        s.spawn(move || {
            let result = account_tx_1.execute(&mut state_1, &block_context_1, false, true); // Do not charge fee as account is not funded.
            assert_eq!(result.is_err(), enforce_fee);

Suggestion:

    thread::scope(|s| {
        s.spawn(move || {
            let result = account_tx_1.execute(&mut state_1, &block_context_1, enforce_fee, true);
            assert_eq!(result.is_err(), enforce_fee); // Transaction fails iff we enforced the fee charge (as the acount is not funded).

---

crates/blockifier/src/transaction/transactions_test.rs line 1766 at r21 (raw file):

    });
    // Do not charge fee as all tx has default resource bounds, account is not funded (bounds=0).
    let default_charge_fee = false;

Suggestion:

    // We test `__validate__`, and don't care about the cahrge fee flow.
    let charge_fee = false;

---

crates/blockifier/src/transaction/transactions_test.rs line 2059 at r21 (raw file):

    let value = calldata.0[2];
    let payload_size = tx.payload_size();
    let actual_execution_info = tx.execute(state, block_context, false, true).unwrap(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0.

@avivg-starkware not that this is temporary. We will need to preserve the l1 handler behaviour without requiring the caller of execute to specify false in charge fee (separate PR).

Code quote:

    let actual_execution_info = tx.execute(state, block_context, false, true).unwrap(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0.

[noaov1]

Reviewed 1 of 12 files at r15, 1 of 6 files at r21.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware)

[meship-starkware]

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

[avivg-starkware]

Reviewed 1 of 4 files at r9, 1 of 3 files at r12, 2 of 12 files at r15, 1 of 7 files at r18, 10 of 10 files at r23.
Reviewable status: 14 of 15 files reviewed, all discussions resolved (waiting on @avivg-starkware)

[avivg-starkware]

Reviewed 1 of 1 files at r24, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)