chore(blockifier): combine tx_info.enforce_fee() with charge_fee flag

[avivg-starkware]

This change is 

[avivg-starkware]

there are comments including "aviv" for my documentation, will remove before merge

Reviewable status: 0 of 17 files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.13%. Comparing base (d7fd3d7) to head (7ecfacb).
Report is 46 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
+ Coverage   75.78%   76.13%   +0.34%     
==========================================
  Files         356      356              
  Lines       37762    37917     +155     
  Branches    37762    37917     +155     
==========================================
+ Hits        28619    28868     +249     
+ Misses       6848     6746     -102     
- Partials     2295     2303       +8     

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

[meship-starkware]

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

---

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

            self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
        );
        let tx_charge_fee = tx.create_tx_info().expect("Failed to create tx info.").enforce_fee(); //aviv: todo: msg -> CONST?

Remove all comments.

[meship-starkware]

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

---

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

        });
    });
}

I think it's better to declare a variable where you use it.

Suggestion:

    // Execute transactions
    thread::scope(|s| {
        s.spawn(move || {
            let charge_fee_1 = account_tx_1.create_tx_info().unwrap().enforce_fee(); // aviv: todo better implementation?
            let result = account_tx_1.execute(&mut state_1, &block_context_1, charge_fee_1, true); // aviv: was true, true
            assert_eq!(result.is_err(), enforce_fee);
        });
        s.spawn(move || {
            let charge_fee_2 = account_tx_2.create_tx_info().unwrap().enforce_fee(); // aviv: todo better implementation?
            account_tx_2.execute(&mut state_2, &block_context_2, charge_fee_2, true).unwrap(); // aviv: was true, true
            // Check that the constructor wrote ctor_arg to the storage.
            let storage_key = get_storage_var_address("ctor_arg", &[]);
            let deployed_contract_address = calculate_contract_address(
                ContractAddressSalt::default(),
                class_hash,
                &constructor_calldata,
                ContractAddress::default(),
            )
            .unwrap();
            let read_storage_arg =
                state_2.get_storage_at(deployed_contract_address, storage_key).unwrap();
            assert_eq!(ctor_storage_arg, read_storage_arg);
        });
    });
}

---

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

    ) -> TransactionExecutionResult<TransactionExecutionInfo> {
        let tx_context = Arc::new(block_context.to_tx_context(self)?);
        let limit_steps_by_resources = _execution_flags.charge_fee; // aviv: new

The _ notion is for unused variables.

Suggestion:

        execution_flags: ExecutionFlags,
    ) -> TransactionExecutionResult<TransactionExecutionInfo> {
        let tx_context = Arc::new(block_context.to_tx_context(self)?);
        let limit_steps_by_resources = execution_flags.charge_fee; // aviv: new

[meship-starkware]

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

---

crates/blockifier/src/test_utils/prices.rs line 81 at r1 (raw file):

                ),
                ExecutionMode::Execute,
                false, // aviv: limit_steps_by_resources: can be enforce_fee() instead of false.?

It is better here to have false because it is not dependent on enforce fee, here we just want to get the resources of fee_transfer without actually creating a transaction.

---

crates/papyrus_execution/src/lib.rs line 256 at r1 (raw file):

        // TODO(yair): fix when supporting v3 transactions
        Arc::new(TransactionContext { block_context, tx_info }),
        limit_steps_by_resources,

I think the todo refers to the Deprecated tx_info.

Suggestion:

    )?;
    // TODO(yair): fix when supporting v3 transactions
    let tx_info = TransactionInfo::Deprecated(DeprecatedTransactionInfo::default()); // aviv: new
    let limit_steps_by_resources = tx_info.enforce_fee(); // aviv: new

    let mut context = EntryPointExecutionContext::new_invoke(
        Arc::new(TransactionContext { block_context, tx_info }),
        limit_steps_by_resources,

[avivg-starkware]

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

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I think it's better to declare a variable where you use it.

Done.

---

crates/blockifier/src/test_utils/prices.rs line 81 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It is better here to have false because it is not dependent on enforce fee, here we just want to get the resources of fee_transfer without actually creating a transaction.

Thank you for explaining! I see what you're saying. I'd love to ask about it in person as well

---

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

Previously, meship-starkware (Meshi Peled) wrote…

The _ notion is for unused variables.

Done.

---

crates/papyrus_execution/src/lib.rs line 256 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think the todo refers to the Deprecated tx_info.

done

[meship-starkware]

Reviewed 1 of 17 files at r1, 1 of 4 files at r2, 12 of 14 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[meship-starkware]

Reviewed 1 of 17 files at r1, 1 of 14 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[meship-starkware]

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

---

crates/blockifier/src/transaction/account_transactions_test.rs line 171 at r4 (raw file):

            resource_bounds: l1_resource_bounds(u64::from(!zero_bounds), DEFAULT_STRK_L1_GAS_PRICE),
            version,
        },

We need to consider the relevance of these tests after our changes. I think they are still relevant, but maybe some changes are needed. @noaov1 WDYT?

---

crates/blockifier/src/transaction/account_transactions_test.rs line 191 at r4 (raw file):

        &mut state,
        &block_context,
        invoke_tx_args! {

Same here

[meship-starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 805 at r4 (raw file):

    let charge_fee = tx_info.enforce_fee();

    match tx_info {

Nice!

Code quote:

    let tx_info = invalid_tx.create_tx_info();
    let charge_fee = tx_info.enforce_fee();

    match tx_info {