Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(blockifier): remove enforce fee from pre-validation checks #366

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/blockifier/src/blockifier/stateful_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ 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;
// TODO (Meshi, 1/10/2024): When resource bound is supported in FaultyAccountTxCreatorArgs
// Change charge_fee to true.
let charge_fee = tx_context.tx_info.enforce_fee().unwrap();
tx.perform_pre_validation_stage(
self.tx_executor.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
tx_context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ fn test_transaction_validator(
// Test the stateful validator.
let mut stateful_validator = StatefulValidator::create(state, block_context);

let charge_fee = true;
let charge_fee = false;
let skip_validation = false;
let result = stateful_validator.perform_validations(tx, skip_validation, charge_fee);
assert!(result.is_ok(), "Validation failed: {:?}", result.unwrap_err());
Expand Down
1 change: 1 addition & 0 deletions crates/blockifier/src/blockifier/transaction_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ impl<S: StateReader> TransactionExecutor<S> {
let mut transactional_state = TransactionalState::create_transactional(
self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR),
);

// Executing a single transaction cannot be done in a concurrent mode.
let execution_flags =
ExecutionFlags { charge_fee, validate: true, concurrency_mode: false };
Expand Down
6 changes: 3 additions & 3 deletions crates/blockifier/src/blockifier/transaction_executor_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn tx_executor_test_body<S: StateReader>(
// TODO(Arni, 30/03/2024): Consider adding a test for the transaction execution info. If A test
// should not be added, rename the test to `test_bouncer_info`.
// TODO(Arni, 30/03/2024): Test all bouncer weights.
let charge_fee = true;
let charge_fee = false;
let _tx_execution_info = tx_executor.execute(&tx, charge_fee).unwrap();
let bouncer_weights = tx_executor.bouncer.get_accumulated_weights();
assert_eq!(bouncer_weights.state_diff_size, expected_bouncer_weights.state_diff_size);
Expand Down Expand Up @@ -272,7 +272,7 @@ fn test_bouncing(#[case] initial_bouncer_weights: BouncerWeights, #[case] n_even
contract_address,
nonce_manager.next(account_address),
)),
true, // charge_fee
false, // charge_fee
)
.map_err(|error| panic!("{error:?}: {error}"))
.unwrap();
Expand Down Expand Up @@ -309,7 +309,7 @@ fn test_execute_txs_bouncing() {
.collect();

// Run.
let charge_fee = true;
let charge_fee = false;
let results = tx_executor.execute_txs(&txs, charge_fee);

// Check execution results.
Expand Down
6 changes: 2 additions & 4 deletions crates/blockifier/src/concurrency/versioned_state_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use crate::test_utils::dict_state_reader::DictStateReader;
use crate::test_utils::initial_test_state::test_state;
use crate::test_utils::{CairoVersion, NonceManager, 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_resource_bounds};
use crate::transaction::transactions::ExecutableTransaction;
use crate::{compiled_class_hash, deploy_account_tx_args, nonce, storage_key};
Expand Down Expand Up @@ -233,7 +233,6 @@ fn test_run_parallel_txs(max_resource_bounds: ResourceBoundsMapping) {
&mut NonceManager::default(),
);
let account_tx_1 = AccountTransaction::DeployAccount(deploy_account_tx_1);
let enforce_fee = account_tx_1.create_tx_info().enforce_fee().unwrap();

let class_hash = grindy_account.get_class_hash();
let ctor_storage_arg = felt!(1_u8);
Expand Down Expand Up @@ -262,8 +261,7 @@ fn test_run_parallel_txs(max_resource_bounds: ResourceBoundsMapping) {
// 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);
account_tx_1.execute(&mut state_1, &block_context_1, false, true).unwrap();
});
s.spawn(move || {
account_tx_2.execute(&mut state_2, &block_context_2, true, true).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion crates/blockifier/src/concurrency/worker_logic_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ fn test_worker_commit_phase_with_halt() {
// `ReadyToExecute` and not `Executing` as expected).
assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(0));
assert_eq!(worker_executor.scheduler.next_task(), Task::ExecutionTask(1));
let charge_fee = true;
let charge_fee = false;
// Execute both transactions.
worker_executor.execute(0, charge_fee);
worker_executor.execute(1, charge_fee);
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 @@ -79,6 +79,7 @@ fn test_stack_trace(
invoke_tx_args! {
sender_address: account_address,
calldata,
max_fee: Fee(BALANCE),
version: TransactionVersion::ZERO,
},
)
Expand Down Expand Up @@ -198,6 +199,7 @@ fn test_trace_callchain_ends_with_regular_call(
sender_address: account_address,
calldata,
version: TransactionVersion::ZERO,
max_fee: Fee(BALANCE),
},
)
.unwrap_err();
Expand Down Expand Up @@ -334,6 +336,7 @@ fn test_trace_call_chain_with_syscalls(
sender_address: account_address,
calldata,
version: TransactionVersion::ZERO,
max_fee: Fee(BALANCE),
},
)
.unwrap_err();
Expand Down Expand Up @@ -526,7 +529,7 @@ 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 actual_error = account_tx.execute(state, block_context, false, 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 @@ -589,7 +592,7 @@ 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 error = deploy_account_tx.execute(state, &block_context, false, 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 @@ -192,7 +192,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/test_utils/transfers_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ impl TransfersGenerator {
let account_tx = self.generate_transfer(sender_address, recipient_address);
txs.push(Transaction::AccountTransaction(account_tx));
}
let charge_fee = true;
let charge_fee = false;
let results = self.executor.execute_txs(&txs, charge_fee);
assert_eq!(results.len(), self.config.n_txs);
for result in results {
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 @@ -178,7 +178,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: 3 additions & 51 deletions crates/blockifier/src/transaction/account_transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,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::invoke::InvokeTxArgs;
use crate::test_utils::{
Expand All @@ -47,7 +46,6 @@ use crate::test_utils::{
CairoVersion,
NonceManager,
BALANCE,
DEFAULT_STRK_L1_GAS_PRICE,
MAX_FEE,
};
use crate::transaction::account_transaction::AccountTransaction;
Expand Down Expand Up @@ -152,54 +150,6 @@ fn test_rc96_holes(block_context: BlockContext, max_resource_bounds: ResourceBou
assert_eq!(tx_execution_info.receipt.gas, GasVector::from_l1_gas(6598));
}

#[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(u64::from(!zero_bounds), DEFAULT_STRK_L1_GAS_PRICE),
version,
},
&mut NonceManager::default(),
);

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

#[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, DEFAULT_STRK_L1_GAS_PRICE),
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 @@ -517,6 +467,7 @@ fn test_recursion_depth_exceeded(
fn test_revert_invoke(
block_context: BlockContext,
max_fee: Fee,
max_resource_bounds: ResourceBoundsMapping,
#[case] transaction_version: TransactionVersion,
#[case] fee_type: FeeType,
) {
Expand All @@ -536,6 +487,7 @@ fn test_revert_invoke(
invoke_tx_args! {
max_fee,
sender_address: account_address,
resource_bounds: max_resource_bounds,
calldata: create_calldata(
test_contract_address,
"write_and_revert",
Expand Down Expand Up @@ -606,7 +558,7 @@ 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 error = deploy_account_tx.execute(state, &block_context, false, 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
1 change: 1 addition & 0 deletions crates/blockifier/src/transaction/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ pub fn create_test_init_data(chain_info: &ChainInfo, cairo_version: CairoVersion
}
}

// TODO(Meshi, 1/10/2024): add resource bounds to the argumaents.
pub struct FaultyAccountTxCreatorArgs {
pub tx_type: TransactionType,
pub tx_version: TransactionVersion,
Expand Down
20 changes: 10 additions & 10 deletions crates/blockifier/src/transaction/transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ fn test_state_get_fee_token_balance(
version: tx_version,
nonce: Nonce::default(),
});
account_tx.execute(state, block_context, true, true).unwrap();
account_tx.execute(state, block_context, false, true).unwrap();

// Get balance from state, and validate.
let (low, high) =
Expand Down Expand Up @@ -1538,7 +1538,7 @@ fn test_validate_accounts_tx(
additional_data: None,
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
let error = account_tx.execute(state, block_context, false, true).unwrap_err();
check_transaction_execution_error_for_invalid_scenario!(
cairo_version,
error,
Expand All @@ -1554,7 +1554,7 @@ fn test_validate_accounts_tx(
contract_address_salt: salt_manager.next_salt(),
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
let error = account_tx.execute(state, block_context, false, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall call_contract in execution mode Validate.",
Expand All @@ -1569,7 +1569,7 @@ fn test_validate_accounts_tx(
additional_data: None,
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
let error = account_tx.execute(state, block_context, false, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_block_hash in execution mode Validate.",
Expand All @@ -1583,7 +1583,7 @@ fn test_validate_accounts_tx(
contract_address_salt: salt_manager.next_salt(),
..default_args
});
let error = account_tx.execute(state, block_context, true, true).unwrap_err();
let error = account_tx.execute(state, block_context, false, true).unwrap_err();
check_transaction_execution_error_for_custom_hint!(
&error,
"Unauthorized syscall get_sequencer_address in execution mode Validate.",
Expand All @@ -1606,7 +1606,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, false, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());

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

Expand All @@ -1641,7 +1641,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, false, 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 @@ -1656,7 +1656,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, false, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}

Expand All @@ -1677,7 +1677,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, false, true);
assert!(result.is_ok(), "Execution failed: {:?}", result.unwrap_err());
}
}
Expand Down
Loading