Skip to content

Commit

Permalink
chore(blockifier): have charge_fee flag represent enforce_fee return …
Browse files Browse the repository at this point in the history
…value
  • Loading branch information
avivg-starkware committed Sep 19, 2024
1 parent 50e6493 commit c1309cb
Show file tree
Hide file tree
Showing 11 changed files with 80 additions and 37 deletions.
3 changes: 2 additions & 1 deletion crates/blockifier/src/blockifier/stateful_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::state::errors::StateError;
use crate::state::state_api::StateReader;
use crate::transaction::account_transaction::AccountTransaction;
use crate::transaction::errors::{TransactionExecutionError, TransactionPreValidationError};
use crate::transaction::objects::TransactionInfoCreator;
use crate::transaction::transaction_execution::Transaction;
use crate::transaction::transactions::ValidatableTransaction;

Expand Down Expand Up @@ -95,7 +96,7 @@ impl<S: StateReader> StatefulValidator<S> {
) -> StatefulValidatorResult<()> {
let strict_nonce_check = false;
// Run pre-validation in charge fee mode to perform fee and balance related checks.
let charge_fee = true;
let charge_fee = tx.create_tx_info().enforce_fee();
tx.perform_pre_validation_stage(
self.tx_executor.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
tx_context,
Expand Down
6 changes: 4 additions & 2 deletions crates/blockifier/src/blockifier/transaction_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::state::cached_state::{CachedState, CommitmentStateDiff, Transactional
use crate::state::errors::StateError;
use crate::state::state_api::StateReader;
use crate::transaction::errors::TransactionExecutionError;
use crate::transaction::objects::TransactionExecutionInfo;
use crate::transaction::objects::{TransactionExecutionInfo, TransactionInfoCreator};
use crate::transaction::transaction_execution::Transaction;
use crate::transaction::transactions::{ExecutableTransaction, ExecutionFlags};

Expand Down Expand Up @@ -90,9 +90,11 @@ impl<S: StateReader> TransactionExecutor<S> {
let mut transactional_state = TransactionalState::create_transactional(
self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
);
let tx_charge_fee = tx.create_tx_info().enforce_fee();

// Executing a single transaction cannot be done in a concurrent mode.
let execution_flags =
ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: false };
ExecutionFlags { charge_fee: tx_charge_fee, validate: true, concurrency_mode: false };
let tx_execution_result =
tx.execute_raw(&mut transactional_state, &self.block_context, execution_flags);
match tx_execution_result {
Expand Down
7 changes: 5 additions & 2 deletions crates/blockifier/src/concurrency/versioned_state_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,17 @@ fn test_run_parallel_txs(max_resource_bounds: ValidResourceBounds) {

let block_context_1 = block_context.clone();
let block_context_2 = block_context.clone();

// Execute transactions
thread::scope(|s| {
s.spawn(move || {
let result = account_tx_1.execute(&mut state_1, &block_context_1, true, true);
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);
assert_eq!(result.is_err(), enforce_fee);
});
s.spawn(move || {
account_tx_2.execute(&mut state_2, &block_context_2, true, true).unwrap();
let charge_fee_2 = account_tx_2.create_tx_info().enforce_fee();
account_tx_2.execute(&mut state_2, &block_context_2, charge_fee_2, true).unwrap();
// 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(
Expand Down
9 changes: 7 additions & 2 deletions crates/blockifier/src/concurrency/worker_logic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ use crate::state::cached_state::{
TransactionalState,
};
use crate::state::state_api::{StateReader, UpdatableState};
use crate::transaction::objects::{TransactionExecutionInfo, TransactionExecutionResult};
use crate::transaction::objects::{
TransactionExecutionInfo,
TransactionExecutionResult,
TransactionInfoCreator,
};
use crate::transaction::transaction_execution::Transaction;
use crate::transaction::transactions::{ExecutableTransaction, ExecutionFlags};

Expand Down Expand Up @@ -127,10 +131,11 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> {
fn execute_tx(&self, tx_index: TxIndex) {
let mut tx_versioned_state = self.state.pin_version(tx_index);
let tx = &self.chunk[tx_index];
let tx_charge_fee = tx.create_tx_info().enforce_fee();
let mut transactional_state =
TransactionalState::create_transactional(&mut tx_versioned_state);
let execution_flags =
ExecutionFlags { charge_fee: true, validate: true, concurrency_mode: true };
ExecutionFlags { charge_fee: tx_charge_fee, validate: true, concurrency_mode: true };
let execution_result =
tx.execute_raw(&mut transactional_state, self.block_context, execution_flags);

Expand Down
7 changes: 5 additions & 2 deletions crates/blockifier/src/execution/stack_trace_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use crate::transaction::constants::{
VALIDATE_DEPLOY_ENTRY_POINT_NAME,
VALIDATE_ENTRY_POINT_NAME,
};
use crate::transaction::objects::TransactionInfoCreator;
use crate::transaction::test_utils::{
account_invoke_tx,
block_context,
Expand Down Expand Up @@ -620,7 +621,8 @@ Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f ('Invalid s
// Clean pc locations from the trace.
let re = Regex::new(r"pc=0:[0-9]+").unwrap();
let cleaned_expected_error = &re.replace_all(&expected_error, "pc=0:*");
let actual_error = account_tx.execute(state, block_context, true, true).unwrap_err();
let charge_fee = account_tx.create_tx_info().enforce_fee();
let actual_error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err();
let actual_error_str = actual_error.to_string();
let cleaned_actual_error = &re.replace_all(&actual_error_str, "pc=0:*");
// Compare actual trace to the expected trace (sans pc locations).
Expand Down Expand Up @@ -683,7 +685,8 @@ An ASSERT_EQ instruction failed: 1 != 0.
};

// Compare expected and actual error.
let error = deploy_account_tx.execute(state, &block_context, true, true).unwrap_err();
let charge_fee = deploy_account_tx.create_tx_info().enforce_fee();
let error = deploy_account_tx.execute(state, &block_context, charge_fee, true).unwrap_err();
assert_eq!(error.to_string(), expected_error);
}

Expand Down
2 changes: 1 addition & 1 deletion crates/blockifier/src/fee/fee_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ impl PostExecutionReport {
let TransactionReceipt { fee, .. } = tx_receipt;

// If fee is not enforced, no need to check post-execution.
if !charge_fee || !tx_context.tx_info.enforce_fee() {
if !charge_fee {
return Ok(Self(FeeCheckReport::success_report(*fee)));
}

Expand Down
2 changes: 1 addition & 1 deletion crates/blockifier/src/transaction/account_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ impl AccountTransaction {
let tx_info = &tx_context.tx_info;
Self::handle_nonce(state, tx_info, strict_nonce_check)?;

if charge_fee && tx_info.enforce_fee() {
if charge_fee {
self.check_fee_bounds(tx_context)?;

verify_can_pay_committed_bounds(state, tx_context)?;
Expand Down
22 changes: 14 additions & 8 deletions crates/blockifier/src/transaction/account_transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,9 @@ fn test_fee_enforcement(
);

let account_tx = AccountTransaction::DeployAccount(deploy_account_tx);
let enforce_fee = account_tx.create_tx_info().enforce_fee();
let result = account_tx.execute(state, &block_context, true, true);
assert_eq!(result.is_err(), enforce_fee);
let charge_fee = account_tx.create_tx_info().enforce_fee();
let result = account_tx.execute(state, &block_context, charge_fee, true);
assert_eq!(result.is_err(), charge_fee);
}

#[rstest]
Expand Down Expand Up @@ -615,7 +615,9 @@ fn test_fail_deploy_account(

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

let error = deploy_account_tx.execute(state, &block_context, true, true).unwrap_err();
let charge_fee = deploy_account_tx.create_tx_info().enforce_fee();

let error = deploy_account_tx.execute(state, &block_context, charge_fee, true).unwrap_err();
// Check the error is as expected. Assure the error message is not nonce or fee related.
check_transaction_execution_error_for_invalid_scenario!(cairo_version, error, false);

Expand Down Expand Up @@ -941,9 +943,11 @@ fn test_max_fee_to_max_steps_conversion(
nonce: nonce_manager.next(account_address),
});
let tx_context1 = Arc::new(block_context.to_tx_context(&account_tx1));
let execution_context1 = EntryPointExecutionContext::new_invoke(tx_context1, true);
let charge_fee1 = account_tx1.create_tx_info().enforce_fee();
let execution_context1 = EntryPointExecutionContext::new_invoke(tx_context1, charge_fee1);
let max_steps_limit1 = execution_context1.vm_run_resources.get_n_steps();
let tx_execution_info1 = account_tx1.execute(&mut state, &block_context, true, true).unwrap();
let tx_execution_info1 =
account_tx1.execute(&mut state, &block_context, charge_fee1, true).unwrap();
let n_steps1 = tx_execution_info1.receipt.resources.vm_resources.n_steps;
let gas_used_vector1 = tx_execution_info1
.receipt
Expand All @@ -965,9 +969,11 @@ fn test_max_fee_to_max_steps_conversion(
nonce: nonce_manager.next(account_address),
});
let tx_context2 = Arc::new(block_context.to_tx_context(&account_tx2));
let execution_context2 = EntryPointExecutionContext::new_invoke(tx_context2, true);
let charge_fee2 = account_tx2.create_tx_info().enforce_fee();
let execution_context2 = EntryPointExecutionContext::new_invoke(tx_context2, charge_fee2);
let max_steps_limit2 = execution_context2.vm_run_resources.get_n_steps();
let tx_execution_info2 = account_tx2.execute(&mut state, &block_context, true, true).unwrap();
let tx_execution_info2 =
account_tx2.execute(&mut state, &block_context, charge_fee2, true).unwrap();
let n_steps2 = tx_execution_info2.receipt.resources.vm_resources.n_steps;
let gas_used_vector2 = tx_execution_info2
.receipt
Expand Down
3 changes: 2 additions & 1 deletion crates/blockifier/src/transaction/post_execution_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ fn test_revert_on_overdraft(
nonce: nonce_manager.next(account_address),
});
let tx_info = approve_tx.create_tx_info();
let charge_fee = tx_info.enforce_fee();
let approval_execution_info =
approve_tx.execute(&mut state, &block_context, true, true).unwrap();
approve_tx.execute(&mut state, &block_context, charge_fee, true).unwrap();
assert!(!approval_execution_info.is_reverted());

// Transfer a valid amount of funds to compute the cost of a successful
Expand Down
13 changes: 12 additions & 1 deletion crates/blockifier/src/transaction/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::sync::Arc;

use rstest::fixture;
use starknet_api::core::{ClassHash, ContractAddress, Nonce};
use starknet_api::test_utils::deploy_account::DeployAccountTxArgs;
Expand Down Expand Up @@ -292,7 +294,16 @@ pub fn run_invoke_tx(
block_context: &BlockContext,
invoke_args: InvokeTxArgs,
) -> TransactionExecutionResult<TransactionExecutionInfo> {
account_invoke_tx(invoke_args).execute(state, block_context, true, true)
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,
)
}

/// Creates a `ResourceBoundsMapping` with the given `max_amount` and `max_price` for L1 gas limits.
Expand Down
43 changes: 27 additions & 16 deletions crates/blockifier/src/transaction/transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ use crate::transaction::objects::{
StarknetResources,
TransactionExecutionInfo,
TransactionInfo,
TransactionInfoCreator,
TransactionResources,
};
use crate::transaction::test_utils::{
Expand Down Expand Up @@ -787,7 +788,8 @@ fn test_state_get_fee_token_balance(
version: tx_version,
nonce: Nonce::default(),
});
account_tx.execute(state, block_context, true, true).unwrap();
let charge_fee = account_tx.create_tx_info().enforce_fee();
account_tx.execute(state, block_context, charge_fee, true).unwrap();

// Get balance from state, and validate.
let (low, high) =
Expand All @@ -802,10 +804,13 @@ fn assert_failure_if_resource_bounds_exceed_balance(
block_context: &BlockContext,
invalid_tx: AccountTransaction,
) {
match block_context.to_tx_context(&invalid_tx).tx_info {
let tx_info = invalid_tx.create_tx_info();
let charge_fee = tx_info.enforce_fee();

match tx_info {
TransactionInfo::Deprecated(context) => {
assert_matches!(
invalid_tx.execute(state, block_context, true, true).unwrap_err(),
invalid_tx.execute(state, block_context, charge_fee, true).unwrap_err(),
TransactionExecutionError::TransactionPreValidationError(
TransactionPreValidationError::TransactionFeeError(
TransactionFeeError::MaxFeeExceedsBalance{ max_fee, .. }))
Expand All @@ -815,7 +820,7 @@ fn assert_failure_if_resource_bounds_exceed_balance(
TransactionInfo::Current(context) => {
let l1_bounds = context.l1_resource_bounds();
assert_matches!(
invalid_tx.execute(state, block_context, true, true).unwrap_err(),
invalid_tx.execute(state, block_context, charge_fee, true).unwrap_err(),
TransactionExecutionError::TransactionPreValidationError(
TransactionPreValidationError::TransactionFeeError(
TransactionFeeError::GasBoundsExceedBalance{resource, max_amount, max_price, .. }))
Expand Down Expand Up @@ -1603,7 +1608,11 @@ fn test_validate_accounts_tx(
additional_data: None,
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();

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

let error = account_tx.execute(state, block_context, default_charge_fee, true).unwrap_err();
check_transaction_execution_error_for_invalid_scenario!(
cairo_version,
error,
Expand All @@ -1620,7 +1629,8 @@ fn test_validate_accounts_tx(
resource_bounds: ValidResourceBounds::create_for_testing(),
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();

let error = account_tx.execute(state, block_context, default_charge_fee, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall call_contract in execution mode Validate.",
Expand All @@ -1636,7 +1646,8 @@ fn test_validate_accounts_tx(
resource_bounds: ValidResourceBounds::create_for_testing(),
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();

let error = account_tx.execute(state, block_context, default_charge_fee, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_block_hash in execution mode Validate.",
Expand All @@ -1651,7 +1662,7 @@ fn test_validate_accounts_tx(
resource_bounds: ValidResourceBounds::create_for_testing(),
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
let error = account_tx.execute(state, block_context, default_charge_fee, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_sequencer_address in execution mode Validate.",
Expand All @@ -1675,7 +1686,7 @@ fn test_validate_accounts_tx(
..default_args
},
);
let result = account_tx.execute(state, block_context, true, true);
let result = account_tx.execute(state, block_context, default_charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());

if tx_type != TransactionType::DeployAccount {
Expand All @@ -1692,7 +1703,7 @@ fn test_validate_accounts_tx(
..default_args
},
);
let result = account_tx.execute(state, block_context, true, true);
let result = account_tx.execute(state, block_context, default_charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}

Expand All @@ -1712,7 +1723,7 @@ fn test_validate_accounts_tx(
..default_args
},
);
let result = account_tx.execute(state, block_context, true, true);
let result = account_tx.execute(state, block_context, default_charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());

// Call the syscall get_block_timestamp and assert the returned timestamp was modified
Expand All @@ -1728,7 +1739,7 @@ fn test_validate_accounts_tx(
..default_args
},
);
let result = account_tx.execute(state, block_context, true, true);
let result = account_tx.execute(state, block_context, default_charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}

Expand All @@ -1750,7 +1761,7 @@ fn test_validate_accounts_tx(
..default_args
},
);
let result = account_tx.execute(state, block_context, true, true);
let result = account_tx.execute(state, block_context, default_charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}
}
Expand Down Expand Up @@ -1894,8 +1905,8 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) {
let key = calldata.0[1];
let value = calldata.0[2];
let payload_size = tx.payload_size();

let actual_execution_info = tx.execute(state, block_context, true, true).unwrap();
let charge_fee = tx.create_tx_info().enforce_fee();
let actual_execution_info = tx.execute(state, block_context, charge_fee, true).unwrap();

// Build the expected call info.
let accessed_storage_key = StorageKey::try_from(key).unwrap();
Expand Down Expand Up @@ -2016,7 +2027,7 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) {
// always uptade the storage instad.
state.set_storage_at(contract_address, StorageKey::try_from(key).unwrap(), Felt::ZERO).unwrap();
let tx_no_fee = L1HandlerTransaction::create_for_testing(Fee(0), contract_address);
let error = tx_no_fee.execute(state, block_context, true, true).unwrap_err();
let error = tx_no_fee.execute(state, block_context, charge_fee, true).unwrap_err();
// Today, we check that the paid_fee is positive, no matter what was the actual fee.
let expected_actual_fee =
(expected_execution_info.receipt.resources.calculate_tx_fee(block_context, &FeeType::Eth))
Expand Down

0 comments on commit c1309cb

Please sign in to comment.