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 Oct 13, 2024
1 parent db8da61 commit 30144eb
Show file tree
Hide file tree
Showing 10 changed files with 50 additions and 79 deletions.
2 changes: 1 addition & 1 deletion crates/blockifier/src/blockifier/stateful_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,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.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 @@ -16,7 +16,7 @@ use crate::state::cached_state::{CachedState, CommitmentStateDiff, Transactional
use crate::state::errors::StateError;
use crate::state::state_api::{StateReader, StateResult};
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 @@ -98,9 +98,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
9 changes: 5 additions & 4 deletions crates/blockifier/src/concurrency/versioned_state_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use crate::test_utils::dict_state_reader::DictStateReader;
use crate::test_utils::initial_test_state::test_state;
use crate::test_utils::{CairoVersion, BALANCE, DEFAULT_STRK_L1_GAS_PRICE};
use crate::transaction::account_transaction::AccountTransaction;
use crate::transaction::objects::{HasRelatedFeeType, TransactionInfoCreator};
use crate::transaction::objects::HasRelatedFeeType;
use crate::transaction::test_utils::{l1_resource_bounds, max_l1_resource_bounds};
use crate::transaction::transactions::ExecutableTransaction;

Expand Down Expand Up @@ -246,7 +246,7 @@ fn test_run_parallel_txs(max_l1_resource_bounds: ValidResourceBounds) {
&mut NonceManager::default(),
);
let account_tx_1 = AccountTransaction::DeployAccount(deploy_account_tx_1);
let enforce_fee = account_tx_1.create_tx_info().enforce_fee();
let enforce_fee = account_tx_1.enforce_fee();

let class_hash = grindy_account.get_class_hash();
let ctor_storage_arg = felt!(1_u8);
Expand All @@ -272,11 +272,12 @@ fn test_run_parallel_txs(max_l1_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);
assert_eq!(result.is_err(), enforce_fee);
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).
});
s.spawn(move || {
account_tx_2.execute(&mut state_2, &block_context_2, true, true).unwrap();
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 @@ -128,10 +132,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
4 changes: 3 additions & 1 deletion crates/blockifier/src/execution/stack_trace_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,8 @@ An ASSERT_EQ instruction failed: 1 != 0.
// 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.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 All @@ -617,6 +618,7 @@ fn test_account_ctor_frame_stack_trace(
scenario: INVALID,
class_hash,
max_fee: BALANCE,
resource_bounds: max_l1_resource_bounds(),
validate_constructor: true,
..Default::default()
});
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 @@ -268,7 +268,7 @@ impl PostExecutionReport {
let TransactionReceipt { fee, gas, .. } = 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
6 changes: 5 additions & 1 deletion crates/blockifier/src/transaction/account_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ impl AccountTransaction {
}
}

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

fn verify_tx_version(&self, version: TransactionVersion) -> TransactionExecutionResult<()> {
let allowed_versions: Vec<TransactionVersion> = match self {
// Support `Declare` of version 0 in order to allow bootstrapping of a new system.
Expand Down Expand Up @@ -292,7 +296,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
54 changes: 1 addition & 53 deletions crates/blockifier/src/transaction/account_transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ use crate::state::cached_state::{StateChangesCount, TransactionalState};
use crate::state::state_api::{State, StateReader};
use crate::test_utils::contracts::FeatureContract;
use crate::test_utils::declare::declare_tx;
use crate::test_utils::deploy_account::deploy_account_tx;
use crate::test_utils::initial_test_state::{fund_account, test_state};
use crate::test_utils::{
create_calldata,
Expand All @@ -65,7 +64,6 @@ use crate::test_utils::{
get_tx_resources,
CairoVersion,
BALANCE,
DEFAULT_STRK_L1_GAS_PRICE,
DEFAULT_STRK_L2_GAS_PRICE,
MAX_FEE,
};
Expand Down Expand Up @@ -164,33 +162,6 @@ fn test_rc96_holes(block_context: BlockContext, max_l1_resource_bounds: ValidRes
);
}

#[rstest]
fn test_fee_enforcement(
block_context: BlockContext,
#[values(TransactionVersion::ONE, TransactionVersion::THREE)] version: TransactionVersion,
#[values(true, false)] zero_bounds: bool,
) {
let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo0);
let state = &mut test_state(&block_context.chain_info, BALANCE, &[(account, 1)]);
let deploy_account_tx = deploy_account_tx(
deploy_account_tx_args! {
class_hash: account.get_class_hash(),
max_fee: Fee(u128::from(!zero_bounds)),
resource_bounds: l1_resource_bounds(
u8::from(!zero_bounds).into(),
DEFAULT_STRK_L1_GAS_PRICE.into()
),
version,
},
&mut NonceManager::default(),
);

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);
}

#[rstest]
#[case::positive_case_deprecated_tx(true, true)]
#[case::positive_case_new_tx(true, false)]
Expand Down Expand Up @@ -240,30 +211,6 @@ fn test_assert_actual_fee_in_bounds(
}
}

#[rstest]
#[case(TransactionVersion::ZERO)]
#[case(TransactionVersion::ONE)]
#[case(TransactionVersion::THREE)]
fn test_enforce_fee_false_works(block_context: BlockContext, #[case] version: TransactionVersion) {
let TestInitData { mut state, account_address, contract_address, mut nonce_manager } =
create_test_init_data(&block_context.chain_info, CairoVersion::Cairo0);
let tx_execution_info = run_invoke_tx(
&mut state,
&block_context,
invoke_tx_args! {
max_fee: Fee(0),
resource_bounds: l1_resource_bounds(0_u8.into(), DEFAULT_STRK_L1_GAS_PRICE.into()),
sender_address: account_address,
calldata: create_trivial_calldata(contract_address),
version,
nonce: nonce_manager.next(account_address),
},
)
.unwrap();
assert!(!tx_execution_info.is_reverted());
assert_eq!(tx_execution_info.receipt.fee, Fee(0));
}

// TODO(Dori, 15/9/2023): Convert version variance to attribute macro.
#[rstest]
fn test_account_flow_test(
Expand Down Expand Up @@ -688,6 +635,7 @@ fn test_fail_deploy_account(
scenario: INVALID,
class_hash: faulty_account_feature_contract.get_class_hash(),
max_fee: BALANCE,
resource_bounds: max_l1_resource_bounds(),
..Default::default()
});
let fee_token_address = chain_info.fee_token_address(&deploy_account_tx.fee_type());
Expand Down
5 changes: 4 additions & 1 deletion crates/blockifier/src/transaction/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,10 @@ 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);
let charge_fee = tx.enforce_fee();

tx.execute(state, block_context, charge_fee, true)
}

/// Creates a `ResourceBoundsMapping` with the given `max_amount` and `max_price` for L1 gas limits.
Expand Down
32 changes: 19 additions & 13 deletions crates/blockifier/src/transaction/transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ use crate::transaction::objects::{
HasRelatedFeeType,
TransactionExecutionInfo,
TransactionInfo,
TransactionInfoCreator,
};
use crate::transaction::test_utils::{
account_invoke_tx,
Expand Down Expand Up @@ -803,6 +804,7 @@ fn test_state_get_fee_token_balance(
create_calldata(fee_token_address, "permissionedMint", &[recipient, mint_low, mint_high]);
let account_tx = account_invoke_tx(invoke_tx_args! {
max_fee: MAX_FEE,
resource_bounds: max_l1_resource_bounds(),
sender_address: account_address,
calldata: execute_calldata,
version: tx_version,
Expand All @@ -823,7 +825,9 @@ 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();

match tx_info {
TransactionInfo::Deprecated(context) => {
assert_matches!(
invalid_tx.execute(state, block_context, true, true).unwrap_err(),
Expand Down Expand Up @@ -1755,7 +1759,10 @@ fn test_validate_accounts_tx(
additional_data: None,
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
// We test `__validate__`, and don't care about the cahrge fee flow.
let charge_fee = false;

let error = account_tx.execute(state, block_context, charge_fee, true).unwrap_err();
check_tx_execution_error_for_invalid_scenario!(cairo_version, error, validate_constructor,);

// Try to call another contract (forbidden).
Expand All @@ -1768,7 +1775,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, charge_fee, true).unwrap_err();
check_tx_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall call_contract in execution mode Validate.",
Expand All @@ -1784,7 +1791,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, charge_fee, true).unwrap_err();
check_tx_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_block_hash in execution mode Validate.",
Expand All @@ -1799,7 +1806,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, charge_fee, true).unwrap_err();
check_tx_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_sequencer_address in execution mode Validate.",
Expand All @@ -1823,7 +1830,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, charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());

if tx_type != TransactionType::DeployAccount {
Expand All @@ -1840,7 +1847,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, charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}

Expand All @@ -1860,7 +1867,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, 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 @@ -1876,7 +1883,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, charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}

Expand All @@ -1898,7 +1905,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, charge_fee, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}
}
Expand Down Expand Up @@ -2042,8 +2049,7 @@ 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 actual_execution_info = tx.execute(state, block_context, false, true).unwrap(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0.

// Build the expected call info.
let accessed_storage_key = StorageKey::try_from(key).unwrap();
Expand Down Expand Up @@ -2173,7 +2179,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, false, true).unwrap_err(); // Do not charge fee as L1Handler's resource bounds (/max fee) is 0.
// Today, we check that the paid_fee is positive, no matter what was the actual fee.
let expected_actual_fee =
get_fee_by_gas_vector(&block_context.block_info, total_gas, &FeeType::Eth);
Expand Down

0 comments on commit 30144eb

Please sign in to comment.