From 2b7f89e3fc86497a1602da8e1c95a4ab8e7d813b Mon Sep 17 00:00:00 2001 From: nimrod-starkware <143319383+nimrod-starkware@users.noreply.github.com> Date: Wed, 28 Aug 2024 17:12:32 +0300 Subject: [PATCH] refactor(fee): block context creator may fail (#507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change is [Reviewable](https://reviewable.io/reviews/starkware-libs/sequencer/507) --- .../src/blockifier/stateful_validator.rs | 4 ++-- .../src/concurrency/versioned_state_test.rs | 2 +- .../blockifier/src/concurrency/worker_logic.rs | 5 ++++- .../src/concurrency/worker_logic_test.rs | 4 ++-- crates/blockifier/src/context.rs | 10 ++++++---- crates/blockifier/src/fee/fee_test.rs | 2 +- crates/blockifier/src/fee/gas_usage_test.rs | 5 +++-- crates/blockifier/src/test_utils/prices.rs | 6 +++++- .../src/transaction/account_transaction.rs | 2 +- .../src/transaction/account_transactions_test.rs | 4 ++-- crates/blockifier/src/transaction/errors.rs | 2 ++ .../src/transaction/transaction_execution.rs | 4 ++-- .../src/transaction/transactions_test.rs | 16 ++++++++-------- 13 files changed, 39 insertions(+), 27 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index c93bbef268..392db67885 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -71,7 +71,7 @@ impl StatefulValidator { return Ok(()); } - let tx_context = self.tx_executor.block_context.to_tx_context(&tx); + let tx_context = self.tx_executor.block_context.to_tx_context(&tx)?; self.perform_pre_validation_stage(&tx, &tx_context)?; if skip_validate { @@ -118,7 +118,7 @@ impl StatefulValidator { mut remaining_gas: u64, ) -> StatefulValidatorResult<(Option, TransactionReceipt)> { let mut execution_resources = ExecutionResources::default(); - let tx_context = Arc::new(self.tx_executor.block_context.to_tx_context(tx)); + let tx_context = Arc::new(self.tx_executor.block_context.to_tx_context(tx)?); let limit_steps_by_resources = true; let validate_call_info = tx.validate_tx( diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index a913f60484..0ea94ab04a 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -248,7 +248,7 @@ fn test_run_parallel_txs(max_resource_bounds: DeprecatedResourceBoundsMapping) { let deploy_account_tx_2 = deploy_account_tx(deploy_tx_args, nonce_manager); let account_address = deploy_account_tx_2.contract_address(); let account_tx_2 = AccountTransaction::DeployAccount(deploy_account_tx_2); - let tx_context = block_context.to_tx_context(&account_tx_2); + let tx_context = block_context.to_tx_context(&account_tx_2).unwrap(); let fee_type = tx_context.tx_info.fee_type(); let deployed_account_balance_key = get_fee_token_var_address(account_address); diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index 3029236f6f..ed22913bfe 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/crates/blockifier/src/concurrency/worker_logic.rs @@ -230,7 +230,10 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { &mut execution_output.as_mut().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR).result; if let Ok(tx_execution_info) = tx_result.as_mut() { - let tx_context = self.block_context.to_tx_context(&self.chunk[tx_index]); + let tx_context = self + .block_context + .to_tx_context(&self.chunk[tx_index]) + .expect("Failed to create tx context."); // Add the deleted sequencer balance key to the storage keys. let concurrency_mode = true; tx_state_changes_keys.update_sequencer_key_in_storage( diff --git a/crates/blockifier/src/concurrency/worker_logic_test.rs b/crates/blockifier/src/concurrency/worker_logic_test.rs index 0833734a66..3f9a9ebcac 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -174,7 +174,7 @@ pub fn test_commit_tx() { assert_eq!(felt!(expected_sequencer_storage_read), actual_sequencer_storage_read,); } } - let tx_context = executor.block_context.to_tx_context(&txs[commit_idx]); + let tx_context = executor.block_context.to_tx_context(&txs[commit_idx]).unwrap(); expected_sequencer_balance_low += actual_fee; // Check that the sequencer balance was updated correctly in the state. verify_sequencer_balance_update( @@ -228,7 +228,7 @@ fn test_commit_tx_when_sender_is_sequencer() { let read_values_before_commit = fee_transfer_call_info.storage_read_values.clone(); drop(execution_task_outputs); - let tx_context = &executor.block_context.to_tx_context(&sequencer_tx[0]); + let tx_context = &executor.block_context.to_tx_context(&sequencer_tx[0]).unwrap(); let fee_token_address = executor.block_context.chain_info.fee_token_address(&tx_context.tx_info.fee_type()); let sequencer_balance_high_before = diff --git a/crates/blockifier/src/context.rs b/crates/blockifier/src/context.rs index 7f7864fffb..50ea1444d1 100644 --- a/crates/blockifier/src/context.rs +++ b/crates/blockifier/src/context.rs @@ -7,6 +7,7 @@ use starknet_api::core::{ChainId, ContractAddress}; use crate::blockifier::block::BlockInfo; use crate::bouncer::BouncerConfig; +use crate::transaction::errors::TransactionInfoCreationError; use crate::transaction::objects::{ FeeType, HasRelatedFeeType, @@ -62,14 +63,15 @@ impl BlockContext { &self.versioned_constants } + // TODO(Nimrod): Don't return `Result`. pub fn to_tx_context( &self, tx_info_creator: &impl TransactionInfoCreator, - ) -> TransactionContext { - TransactionContext { + ) -> Result { + Ok(TransactionContext { block_context: self.clone(), - tx_info: tx_info_creator.create_tx_info().expect("todo"), - } + tx_info: tx_info_creator.create_tx_info()?, + }) } } diff --git a/crates/blockifier/src/fee/fee_test.rs b/crates/blockifier/src/fee/fee_test.rs index 9c86e5f9d3..f782a54e42 100644 --- a/crates/blockifier/src/fee/fee_test.rs +++ b/crates/blockifier/src/fee/fee_test.rs @@ -162,7 +162,7 @@ fn test_discounted_gas_overdraft( let charge_fee = true; let report = PostExecutionReport::new( &mut state, - &block_context.to_tx_context(&tx), + &block_context.to_tx_context(&tx).unwrap(), &tx_receipt, charge_fee, ) diff --git a/crates/blockifier/src/fee/gas_usage_test.rs b/crates/blockifier/src/fee/gas_usage_test.rs index 1d89c913db..b83517acd8 100644 --- a/crates/blockifier/src/fee/gas_usage_test.rs +++ b/crates/blockifier/src/fee/gas_usage_test.rs @@ -205,8 +205,9 @@ fn test_get_message_segment_length( #[rstest] fn test_compute_discounted_gas_from_gas_vector() { - let tx_context = - BlockContext::create_for_testing().to_tx_context(&account_invoke_tx(invoke_tx_args! {})); + let tx_context = BlockContext::create_for_testing() + .to_tx_context(&account_invoke_tx(invoke_tx_args! {})) + .unwrap(); let gas_usage = GasVector { l1_gas: 100, l1_data_gas: 2, ..Default::default() }; let actual_result = compute_discounted_gas_from_gas_vector(&gas_usage, &tx_context); diff --git a/crates/blockifier/src/test_utils/prices.rs b/crates/blockifier/src/test_utils/prices.rs index f605d42ff4..0cd544c4a8 100644 --- a/crates/blockifier/src/test_utils/prices.rs +++ b/crates/blockifier/src/test_utils/prices.rs @@ -72,7 +72,11 @@ fn fee_transfer_resources( state, &mut ExecutionResources::default(), &mut EntryPointExecutionContext::new( - Arc::new(block_context.to_tx_context(&account_invoke_tx(InvokeTxArgs::default()))), + Arc::new( + block_context + .to_tx_context(&account_invoke_tx(InvokeTxArgs::default())) + .unwrap(), + ), ExecutionMode::Execute, false, ), diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 204659db3a..28cd17aa68 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -681,7 +681,7 @@ impl ExecutableTransaction for AccountTransaction { block_context: &BlockContext, execution_flags: ExecutionFlags, ) -> TransactionExecutionResult { - let tx_context = Arc::new(block_context.to_tx_context(self)); + let tx_context = Arc::new(block_context.to_tx_context(self)?); self.verify_tx_version(tx_context.tx_info.version())?; // Nonce and fee check should be done before running user code. diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 7652e251bd..d47cc3650d 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -932,7 +932,7 @@ fn test_max_fee_to_max_steps_conversion( resource_bounds: l1_resource_bounds(actual_gas_used, actual_strk_gas_price.into()), nonce: nonce_manager.next(account_address), }); - let tx_context1 = Arc::new(block_context.to_tx_context(&account_tx1)); + let tx_context1 = Arc::new(block_context.to_tx_context(&account_tx1).unwrap()); let execution_context1 = EntryPointExecutionContext::new_invoke(tx_context1, true); 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(); @@ -952,7 +952,7 @@ fn test_max_fee_to_max_steps_conversion( resource_bounds: l1_resource_bounds(2 * actual_gas_used, actual_strk_gas_price.into()), nonce: nonce_manager.next(account_address), }); - let tx_context2 = Arc::new(block_context.to_tx_context(&account_tx2)); + let tx_context2 = Arc::new(block_context.to_tx_context(&account_tx2).unwrap()); let execution_context2 = EntryPointExecutionContext::new_invoke(tx_context2, true); 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(); diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index 1a1ab31029..693c7c6508 100644 --- a/crates/blockifier/src/transaction/errors.rs +++ b/crates/blockifier/src/transaction/errors.rs @@ -112,6 +112,8 @@ pub enum TransactionExecutionError { InvalidSegmentStructure(usize, usize), #[error(transparent)] ProgramError(#[from] ProgramError), + #[error(transparent)] + TransactionInfoCreationError(#[from] TransactionInfoCreationError), } #[derive(Debug, Error)] diff --git a/crates/blockifier/src/transaction/transaction_execution.rs b/crates/blockifier/src/transaction/transaction_execution.rs index 9ef166b854..f11915267b 100644 --- a/crates/blockifier/src/transaction/transaction_execution.rs +++ b/crates/blockifier/src/transaction/transaction_execution.rs @@ -115,7 +115,7 @@ impl ExecutableTransaction for L1HandlerTransaction { block_context: &BlockContext, _execution_flags: ExecutionFlags, ) -> TransactionExecutionResult { - let tx_context = Arc::new(block_context.to_tx_context(self)); + let tx_context = Arc::new(block_context.to_tx_context(self)?); let mut execution_resources = ExecutionResources::default(); let mut context = EntryPointExecutionContext::new_invoke(tx_context.clone(), true); @@ -184,7 +184,7 @@ impl ExecutableTransaction for Transaction { let tx_execution_summary = tx_execution_info.summarize(); let mut tx_state_changes_keys = state.get_actual_state_changes()?.into_keys(); tx_state_changes_keys.update_sequencer_key_in_storage( - &block_context.to_tx_context(self), + &block_context.to_tx_context(self)?, &tx_execution_info, concurrency_mode, ); diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index a522ec9b26..f52e744438 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -422,7 +422,7 @@ fn test_invoke_tx( let sender_address = invoke_tx.sender_address(); let account_tx = AccountTransaction::Invoke(invoke_tx); - let tx_context = block_context.to_tx_context(&account_tx); + let tx_context = block_context.to_tx_context(&account_tx).unwrap(); let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); @@ -792,7 +792,7 @@ fn assert_failure_if_resource_bounds_exceed_balance( block_context: &BlockContext, invalid_tx: AccountTransaction, ) { - match block_context.to_tx_context(&invalid_tx).tx_info { + match block_context.to_tx_context(&invalid_tx).unwrap().tx_info { TransactionInfo::Deprecated(context) => { assert_matches!( invalid_tx.execute(state, block_context, true, true).unwrap_err(), @@ -1040,7 +1040,7 @@ fn test_invalid_nonce( let invalid_nonce = nonce!(1_u8); let invalid_tx = account_invoke_tx(invoke_tx_args! { nonce: invalid_nonce, ..valid_invoke_tx_args.clone() }); - let invalid_tx_context = block_context.to_tx_context(&invalid_tx); + let invalid_tx_context = block_context.to_tx_context(&invalid_tx).unwrap(); let pre_validation_err = invalid_tx .perform_pre_validation_stage(&mut transactional_state, &invalid_tx_context, false, true) .unwrap_err(); @@ -1060,7 +1060,7 @@ fn test_invalid_nonce( let valid_tx = account_invoke_tx(invoke_tx_args! { nonce: valid_nonce, ..valid_invoke_tx_args.clone() }); - let valid_tx_context = block_context.to_tx_context(&valid_tx); + let valid_tx_context = block_context.to_tx_context(&valid_tx).unwrap(); valid_tx .perform_pre_validation_stage(&mut transactional_state, &valid_tx_context, false, false) .unwrap(); @@ -1069,7 +1069,7 @@ fn test_invalid_nonce( let invalid_nonce = nonce!(0_u8); let invalid_tx = account_invoke_tx(invoke_tx_args! { nonce: invalid_nonce, ..valid_invoke_tx_args.clone() }); - let invalid_tx_context = block_context.to_tx_context(&invalid_tx); + let invalid_tx_context = block_context.to_tx_context(&invalid_tx).unwrap(); let pre_validation_err = invalid_tx .perform_pre_validation_stage(&mut transactional_state, &invalid_tx_context, false, false) .unwrap_err(); @@ -1185,7 +1185,7 @@ fn test_declare_tx( undeclared_class_hash == class_hash ); let fee_type = &account_tx.fee_type(); - let tx_context = &block_context.to_tx_context(&account_tx); + let tx_context = &block_context.to_tx_context(&account_tx).unwrap(); let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); // Build expected validate call info. @@ -1328,7 +1328,7 @@ fn test_deploy_account_tx( let account_tx = AccountTransaction::DeployAccount(deploy_account); let fee_type = &account_tx.fee_type(); - let tx_context = &block_context.to_tx_context(&account_tx); + let tx_context = &block_context.to_tx_context(&account_tx).unwrap(); let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); // Build expected validate call info. @@ -1470,7 +1470,7 @@ fn test_fail_deploy_account_undeclared_class_hash( deploy_account_tx_args! {resource_bounds: max_resource_bounds, class_hash: undeclared_hash }, &mut nonce_manager, ); - let tx_context = block_context.to_tx_context(&deploy_account); + let tx_context = block_context.to_tx_context(&deploy_account).unwrap(); let fee_type = tx_context.tx_info.fee_type(); // Fund account, so as not to fail pre-validation.