test(concurrency): add unitests for concurrency fee transfer (#1773)

crates/blockifier/src/concurrency.rs

@@ -1,3 +1,4 @@
+pub mod fee_utils;
 pub mod scheduler;
 #[cfg(any(feature = "testing", test))]
 pub mod test_utils;

crates/blockifier/src/concurrency/fee_utils.rs

@@ -0,0 +1,30 @@
+use starknet_api::hash::StarkFelt;
+
+use crate::execution::call_info::CallInfo;
+#[cfg(test)]
+#[path = "fee_utils_test.rs"]
+mod test;
+
+// We read account balance (sender), and sequencer balance (recipient). The balance is of type
+// `Uint256`, consist of two felts (lsb, msb). Hence, storage read values =
+// [account_balance, 0, sequencer_balance, 0]
+const STORAGE_READ_SEQUENCER_BALANCE_INDICES: (usize, usize) = (2, 3);
+
+// Completes the fee transfer execution by fixing the call info to have the correct sequencer
+// balance. In concurrency mode, the fee transfer is executed with a false (constant) sequencer
+// balance. This affects the call info.
+pub fn fill_sequencer_balance_reads(
+    fee_transfer_call_info: &mut CallInfo,
+    sequencer_balance_low: StarkFelt,
+    sequencer_balance_high: StarkFelt,
+) {
+    let storage_read_values = &mut fee_transfer_call_info.storage_read_values;
+    assert_eq!(storage_read_values.len(), 4, "Storage read values should have 4 elements");
+
+    let (low_index, high_index) = STORAGE_READ_SEQUENCER_BALANCE_INDICES;
+    for index in [low_index, high_index] {
+        assert_eq!(storage_read_values[index], StarkFelt::ZERO, "Sequencer balance should be zero");
+    }
+    storage_read_values[low_index] = sequencer_balance_low;
+    storage_read_values[high_index] = sequencer_balance_high;
+}

crates/blockifier/src/concurrency/fee_utils_test.rs

@@ -0,0 +1,44 @@
+use rstest::rstest;
+use starknet_api::hash::StarkFelt;
+use starknet_api::transaction::TransactionVersion;
+
+use crate::concurrency::fee_utils::fill_sequencer_balance_reads;
+use crate::concurrency::test_utils::create_fee_transfer_call_info;
+use crate::context::BlockContext;
+use crate::invoke_tx_args;
+use crate::test_utils::contracts::FeatureContract;
+use crate::test_utils::initial_test_state::{fund_account, test_state};
+use crate::test_utils::{
+    create_trivial_calldata, CairoVersion, BALANCE, MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE,
+};
+use crate::transaction::test_utils::{account_invoke_tx, block_context, l1_resource_bounds};
+
+#[rstest]
+pub fn test_fill_sequencer_balance_reads(block_context: BlockContext) {
+    let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1);
+    let account_tx = account_invoke_tx(invoke_tx_args! {
+        sender_address: account.get_instance_address(0),
+        calldata: create_trivial_calldata(account.get_instance_address(0)),
+        resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
+        version: TransactionVersion::THREE
+    });
+    let chain_info = &block_context.chain_info;
+    let state = &mut test_state(chain_info, BALANCE, &[(account, 1)]);
+
+    let sequencer_balance = 100;
+    let sequencer_address = block_context.block_info.sequencer_address;
+    fund_account(chain_info, sequencer_address, sequencer_balance, &mut state.state);
+
+    let mut concurrency_call_info = create_fee_transfer_call_info(state, &account_tx, true);
+    let call_info = create_fee_transfer_call_info(state, &account_tx, false);
+
+    assert_ne!(concurrency_call_info, call_info);
+
+    fill_sequencer_balance_reads(
+        &mut concurrency_call_info,
+        StarkFelt::from(sequencer_balance),
+        StarkFelt::ZERO,
+    );
+
+    assert_eq!(concurrency_call_info, call_info);
+}

crates/blockifier/src/concurrency/test_utils.rs

@@ -1,5 +1,11 @@
 use crate::concurrency::versioned_state_proxy::{ThreadSafeVersionedState, VersionedState};
+use crate::context::BlockContext;
+use crate::execution::call_info::CallInfo;
+use crate::state::cached_state::CachedState;
+use crate::state::state_api::StateReader;
 use crate::test_utils::dict_state_reader::DictStateReader;
+use crate::transaction::account_transaction::AccountTransaction;
+use crate::transaction::transactions::ExecutableTransaction;
 
 #[macro_export]
 macro_rules! default_scheduler {
@@ -35,3 +41,23 @@ pub fn safe_versioned_state_for_testing(
 ) -> ThreadSafeVersionedState<DictStateReader> {
     ThreadSafeVersionedState::new(VersionedState::new(block_state))
 }
+
+// Note: this function does not mutate the state.
+pub fn create_fee_transfer_call_info<S: StateReader>(
+    state: &mut CachedState<S>,
+    account_tx: &AccountTransaction,
+    concurrency_mode: bool,
+) -> CallInfo {
+    let block_context =
+        BlockContext::create_for_account_testing_with_concurrency_mode(concurrency_mode);
+    let mut transactional_state = CachedState::create_transactional(state);
+    let charge_fee = true;
+    let validate = true;
+    let execution_info = account_tx
+        .execute_raw(&mut transactional_state, &block_context, charge_fee, validate)
+        .unwrap();
+
+    let execution_info = execution_info.fee_transfer_call_info.unwrap();
+    transactional_state.abort();
+    execution_info
+}

crates/blockifier/src/fee/fee_utils.rs

@@ -2,9 +2,12 @@ use std::collections::HashSet;
 
 use cairo_vm::vm::runners::cairo_runner::ExecutionResources;
 use starknet_api::hash::StarkFelt;
+use starknet_api::state::StorageKey;
 use starknet_api::transaction::Fee;
 
+use crate::abi::abi_utils::get_fee_token_var_address;
 use crate::abi::constants;
+use crate::abi::sierra_types::next_storage_key;
 use crate::blockifier::block::BlockInfo;
 use crate::context::{BlockContext, TransactionContext};
 use crate::state::state_api::StateReader;
@@ -141,3 +144,11 @@ pub fn verify_can_pay_committed_bounds(
         })
     }
 }
+
+pub fn get_sequencer_balance_keys(block_context: &BlockContext) -> (StorageKey, StorageKey) {
+    let sequencer_address = block_context.block_info.sequencer_address;
+    let sequencer_balance_key_low = get_fee_token_var_address(sequencer_address);
+    let sequencer_balance_key_high = next_storage_key(&sequencer_balance_key_low)
+        .expect("Cannot get sequencer balance high key.");
+    (sequencer_balance_key_low, sequencer_balance_key_high)
+}

crates/blockifier/src/transaction/account_transaction.rs

@@ -7,15 +7,16 @@ use starknet_api::deprecated_contract_class::EntryPointType;
 use starknet_api::hash::StarkFelt;
 use starknet_api::transaction::{Calldata, Fee, ResourceBounds, TransactionVersion};
 
-use crate::abi::abi_utils::{get_fee_token_var_address, selector_from_name};
-use crate::abi::sierra_types::next_storage_key;
+use crate::abi::abi_utils::selector_from_name;
 use crate::context::{BlockContext, TransactionContext};
 use crate::execution::call_info::{CallInfo, Retdata};
 use crate::execution::contract_class::ContractClass;
 use crate::execution::entry_point::{CallEntryPoint, CallType, EntryPointExecutionContext};
 use crate::fee::actual_cost::TransactionReceipt;
 use crate::fee::fee_checks::{FeeCheckReportFields, PostExecutionReport};
-use crate::fee::fee_utils::{get_fee_by_gas_vector, verify_can_pay_committed_bounds};
+use crate::fee::fee_utils::{
+    get_fee_by_gas_vector, get_sequencer_balance_keys, verify_can_pay_committed_bounds,
+};
 use crate::fee::gas_usage::{compute_discounted_gas_from_gas_vector, estimate_minimal_gas_vector};
 use crate::retdata;
 use crate::state::cached_state::{CachedState, StateChanges, TransactionalState};
@@ -378,10 +379,8 @@ impl AccountTransaction {
     ) -> TransactionExecutionResult<CallInfo> {
         let TransactionContext { block_context, tx_info } = tx_context.as_ref();
         let fee_address = block_context.chain_info.fee_token_address(&tx_info.fee_type());
-        let sequencer_address = block_context.block_info.sequencer_address;
-        let sequencer_balance_key_low = get_fee_token_var_address(sequencer_address);
-        let sequencer_balance_key_high = next_storage_key(&sequencer_balance_key_low)
-            .expect("Cannot get sequencer balance high key.");
+        let (sequencer_balance_key_low, sequencer_balance_key_high) =
+            get_sequencer_balance_keys(block_context);
         let mut transfer_state = CachedState::create_transactional(state);
 
         // Set the initial sequencer balance to avoid tarnishing the read-set of the transaction.

crates/blockifier/src/transaction/account_transactions_test.rs

@@ -17,13 +17,12 @@ use starknet_api::{calldata, class_hash, contract_address, patricia_key, stark_f
 use crate::abi::abi_utils::{
     get_fee_token_var_address, get_storage_var_address, selector_from_name,
 };
-use crate::abi::sierra_types::next_storage_key;
 use crate::context::BlockContext;
 use crate::execution::contract_class::{ContractClass, ContractClassV1};
 use crate::execution::entry_point::EntryPointExecutionContext;
 use crate::execution::execution_utils::{felt_to_stark_felt, stark_felt_to_felt};
 use crate::execution::syscalls::SyscallSelector;
-use crate::fee::fee_utils::get_fee_by_gas_vector;
+use crate::fee::fee_utils::{get_fee_by_gas_vector, get_sequencer_balance_keys};
 use crate::fee::gas_usage::estimate_minimal_gas_vector;
 use crate::state::cached_state::{CachedState, StateChangesCount};
 use crate::state::state_api::{State, StateReader};
@@ -1223,32 +1222,25 @@ fn test_count_actual_storage_changes(
 
 #[rstest]
 fn test_concurrency_execute_fee_transfer(#[values(FeeType::Eth, FeeType::Strk)] fee_type: FeeType) {
-    const STORAGE_WRITE_HIGH: u128 = 150;
+    // TODO(Meshi, 01/06/2024): make the test so it will include changes in
+    // sequencer_balance_key_high.
     const STORAGE_WRITE_LOW: u128 = 100;
     const STORAGE_READ_LOW: u128 = 50;
     let block_context = BlockContext::create_for_account_testing_with_concurrency_mode(true);
-    let empty_contract = FeatureContract::Empty(CairoVersion::Cairo1);
     let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1);
+    let account_tx = account_invoke_tx(invoke_tx_args! {
+        sender_address: account.get_instance_address(0),
+        calldata: create_trivial_calldata(account.get_instance_address(0)),
+        resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
+        version: TransactionVersion::THREE
+    });
     let chain_info = &block_context.chain_info;
+    let fee_token_address = block_context.chain_info.fee_token_address(&fee_type);
     let state = &mut test_state(chain_info, BALANCE, &[(account, 1)]);
-    let class_hash = empty_contract.get_class_hash();
-    let class_info = calculate_class_info_for_testing(empty_contract.get_class());
-    let sender_address = account.get_instance_address(0);
 
-    let account_tx = declare_tx(
-        declare_tx_args! {
-            sender_address,
-            version: TransactionVersion::THREE,
-            resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
-            class_hash,
-        },
-        class_info.clone(),
-    );
+    let (sequencer_balance_key_low, sequencer_balance_key_high) =
+        get_sequencer_balance_keys(&block_context);
 
-    let fee_token_address = block_context.chain_info.fee_token_address(&fee_type);
-    let sequencer_address = block_context.block_info.sequencer_address;
-    let sequencer_balance_key_low = get_fee_token_var_address(sequencer_address);
-    let sequencer_balance_key_high = next_storage_key(&sequencer_balance_key_low).unwrap();
     // Case 1: The transaction did not read form/ write to the sequenser balance before executing
     // fee transfer.
     let mut transactional_state = CachedState::create_transactional(state);
@@ -1266,40 +1258,52 @@ fn test_concurrency_execute_fee_transfer(#[values(FeeType::Eth, FeeType::Strk)]
     // Case 2: The transaction read from and write to the sequenser balance before executing fee
     // transfer.
 
+    let transfer_calldata = create_calldata(
+        fee_token_address,
+        TRANSFER_ENTRY_POINT_NAME,
+        &[
+            *block_context.block_info.sequencer_address.0.key(),
+            stark_felt!(STORAGE_WRITE_LOW),
+            stark_felt!(0_u8),
+        ],
+    );
+
     // Set the sequencer balance to a constant value to check that the read set did not changed.
-    fund_account(chain_info, sequencer_address, STORAGE_READ_LOW, &mut state.state);
+    fund_account(
+        chain_info,
+        block_context.block_info.sequencer_address,
+        STORAGE_READ_LOW,
+        &mut state.state,
+    );
     let mut transactional_state = CachedState::create_transactional(state);
 
-    // Set the sequencer balance write set to a constant value.
-    // Note that it is enough to set the storage_write as execute_raw will update the
-    // storage_initial_values.
-    for (seq_key, value) in [
-        (sequencer_balance_key_low, STORAGE_WRITE_LOW),
-        (sequencer_balance_key_high, STORAGE_WRITE_HIGH),
-    ] {
-        transactional_state.set_storage_at(fee_token_address, seq_key, stark_felt!(value)).unwrap();
-    }
+    // Invokes transfer to the sequencer.
+    let account_tx = account_invoke_tx(invoke_tx_args! {
+        sender_address: account.get_instance_address(0),
+        calldata: transfer_calldata,
+        resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
+        version: TransactionVersion::THREE
+    });
+
+    account_tx.execute_raw(&mut transactional_state, &block_context, true, true).unwrap();
 
-    account_tx.execute_raw(&mut transactional_state, &block_context, true, false).unwrap();
     // Check that the sequencer balance was not changed.
-    let storage_write = transactional_state.cache.borrow().writes.storage.clone();
-    let storage_initial_values = transactional_state.cache.borrow().initial_reads.storage.clone();
+    let storage_writes = transactional_state.cache.borrow().writes.storage.clone();
+    let storage_initial_reads = transactional_state.cache.borrow().initial_reads.storage.clone();
 
     for (seq_write_val, expexted_write_val) in [
         (
-            storage_write.get(&(fee_token_address, sequencer_balance_key_low)),
-            stark_felt!(STORAGE_WRITE_LOW),
+            storage_writes.get(&(fee_token_address, sequencer_balance_key_low)),
+            // Balance after `execute` and without the fee transfer.
+            stark_felt!(STORAGE_WRITE_LOW + STORAGE_READ_LOW),
         ),
         (
-            storage_initial_values.get(&(fee_token_address, sequencer_balance_key_low)),
+            storage_initial_reads.get(&(fee_token_address, sequencer_balance_key_low)),
             stark_felt!(STORAGE_READ_LOW),
         ),
+        (storage_writes.get(&(fee_token_address, sequencer_balance_key_high)), StarkFelt::ZERO),
         (
-            storage_write.get(&(fee_token_address, sequencer_balance_key_high)),
-            stark_felt!(STORAGE_WRITE_HIGH),
-        ),
-        (
-            storage_initial_values.get(&(fee_token_address, sequencer_balance_key_high)),
+            storage_initial_reads.get(&(fee_token_address, sequencer_balance_key_high)),
             StarkFelt::ZERO,
         ),
     ] {