From 1de7cbccf5f02d452feaea28a112407f6eaed1ee Mon Sep 17 00:00:00 2001 From: Aviv Greenburg Date: Wed, 4 Sep 2024 14:50:32 +0300 Subject: [PATCH] refactor(blockifier): remove enforce fee from actual fee --- .../src/blockifier/stateful_validator.rs | 2 +- .../blockifier/src/concurrency/fee_utils.rs | 14 +++++--- .../src/concurrency/worker_logic.rs | 1 - crates/blockifier/src/fee/actual_cost.rs | 17 +++++++--- crates/blockifier/src/test_utils/prices.rs | 2 +- .../src/transaction/account_transaction.rs | 3 +- .../transaction/account_transactions_test.rs | 4 ++- crates/blockifier/src/transaction/objects.rs | 2 +- .../src/transaction/transactions_test.rs | 34 +++++++++++++------ 9 files changed, 53 insertions(+), 26 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index d472266bc6..634db6d655 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -115,7 +115,7 @@ impl StatefulValidator { let mut execution_resources = ExecutionResources::default(); let tx_context = Arc::new(self.tx_executor.block_context.to_tx_context(tx)); - let limit_steps_by_resources = tx.create_tx_info().enforce_fee(); //aviv: was true; + let limit_steps_by_resources = tx.create_tx_info().enforce_fee(); //aviv: was true; let validate_call_info = tx.validate_tx( self.tx_executor.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR), &mut execution_resources, diff --git a/crates/blockifier/src/concurrency/fee_utils.rs b/crates/blockifier/src/concurrency/fee_utils.rs index e40f492319..160fce4bae 100644 --- a/crates/blockifier/src/concurrency/fee_utils.rs +++ b/crates/blockifier/src/concurrency/fee_utils.rs @@ -56,11 +56,15 @@ pub fn complete_fee_transfer_flow( sequencer_balance, ); } else { - assert_eq!( - tx_execution_info.receipt.fee, - Fee(0), - "Transaction with no fee transfer info must have zero fee." - ) + let charge_fee = tx_context.tx_info.enforce_fee(); //aviv: new + + // assert_eq!( + // //aviv: changes: + // // tx_execution_info.receipt.fee, + // // Fee(0), + // "Transaction with no fee transfer info must have zero fee." + // ) + assert!(!charge_fee, "Transaction with no fee transfer info must have zero fee.") } } diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index 41288d69b4..c432426d59 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/crates/blockifier/src/concurrency/worker_logic.rs @@ -27,7 +27,6 @@ use crate::transaction::objects::{ TransactionExecutionResult, TransactionInfoCreator, }; - use crate::transaction::transaction_execution::Transaction; use crate::transaction::transactions::{ExecutableTransaction, ExecutionFlags}; diff --git a/crates/blockifier/src/fee/actual_cost.rs b/crates/blockifier/src/fee/actual_cost.rs index c91f3dac4c..578a3c1b52 100644 --- a/crates/blockifier/src/fee/actual_cost.rs +++ b/crates/blockifier/src/fee/actual_cost.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use cairo_vm::vm::runners::cairo_runner::ExecutionResources; use starknet_api::core::ContractAddress; -use starknet_api::transaction::Fee; +use starknet_api::transaction::{Fee, TransactionVersion}; use crate::abi::constants as abi_constants; use crate::context::TransactionContext; @@ -99,11 +99,20 @@ impl TransactionReceipt { )?; // L1 handler transactions are not charged an L2 fee but it is compared to the L1 fee. - let fee = if tx_context.tx_info.enforce_fee() || tx_type == TransactionType::L1Handler { - tx_context.tx_info.get_fee_by_gas_vector(&tx_context.block_context.block_info, gas) - } else { + // PREV: + // let fee = if tx_context.tx_info.enforce_fee() || tx_type == TransactionType::L1Handler { + // tx_context.tx_info.get_fee_by_gas_vector(&tx_context.block_context.block_info, gas) + // } else { + // Fee(0) + // }; + let fee = if tx_type == TransactionType::Declare + && tx_context.tx_info.signed_version() == TransactionVersion::ZERO + { Fee(0) + } else { + tx_context.tx_info.get_fee_by_gas_vector(&tx_context.block_context.block_info, gas) }; + let da_gas = tx_resources .starknet_resources .get_state_changes_cost(tx_context.block_context.block_info.use_kzg_da); diff --git a/crates/blockifier/src/test_utils/prices.rs b/crates/blockifier/src/test_utils/prices.rs index e768f4e2f4..343ebe7ad5 100644 --- a/crates/blockifier/src/test_utils/prices.rs +++ b/crates/blockifier/src/test_utils/prices.rs @@ -74,7 +74,7 @@ fn fee_transfer_resources( &mut EntryPointExecutionContext::new( Arc::new(block_context.to_tx_context(&account_invoke_tx(InvokeTxArgs::default()))), ExecutionMode::Execute, - false, // aviv: limit_steps_by_resources: can be enforce_fee() instead of false.? + false, ), ) .unwrap() diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index d59502dee0..ebc6be53b9 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -359,7 +359,8 @@ impl AccountTransaction { charge_fee: bool, concurrency_mode: bool, ) -> TransactionExecutionResult> { - if !charge_fee || actual_fee == Fee(0) { + // aviv: if !charge_fee || actual_fee == Fee(0) { + if !charge_fee { // Fee charging is not enforced in some transaction simulations and tests. return Ok(None); } diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index ed92da9025..c39f59359f 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -205,7 +205,7 @@ fn test_enforce_fee_false_works(block_context: BlockContext, #[case] version: Tr ) .unwrap(); assert!(!tx_execution_info.is_reverted()); - assert_eq!(tx_execution_info.receipt.fee, Fee(0)); + // assert_eq!(tx_execution_info.receipt.fee, Fee(0)); // aviv: to remove? } // TODO(Dori, 15/9/2023): Convert version variance to attribute macro. @@ -526,6 +526,7 @@ fn test_recursion_depth_exceeded( fn test_revert_invoke( block_context: BlockContext, max_fee: Fee, + max_resource_bounds: ValidResourceBounds, #[case] transaction_version: TransactionVersion, #[case] fee_type: FeeType, ) { @@ -544,6 +545,7 @@ fn test_revert_invoke( &block_context, invoke_tx_args! { max_fee, + resource_bounds: max_resource_bounds, sender_address: account_address, calldata: create_calldata( test_contract_address, diff --git a/crates/blockifier/src/transaction/objects.rs b/crates/blockifier/src/transaction/objects.rs index cc918d37ee..d0f1d9a2a5 100644 --- a/crates/blockifier/src/transaction/objects.rs +++ b/crates/blockifier/src/transaction/objects.rs @@ -106,7 +106,7 @@ impl TransactionInfo { TransactionInfo::Current(context) => { let l1_bounds = context.l1_resource_bounds(); let max_amount: u128 = l1_bounds.max_amount.into(); - max_amount * l1_bounds.max_price_per_unit > 0 + max_amount * l1_bounds.max_price_per_unit > 0 //aviv: note : this is max fee } TransactionInfo::Deprecated(context) => context.max_fee != Fee(0), } diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index 961f2eb04e..f1b2b73b62 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -1225,10 +1225,11 @@ fn test_declare_tx( None, std::iter::empty(), ); - + // For V0 transactions, the max fee is always 0. + let max_fee = if tx_version == TransactionVersion::ZERO { Fee(0) } else { Fee(MAX_FEE) }; let account_tx = declare_tx( declare_tx_args! { - max_fee: Fee(MAX_FEE), + max_fee, sender_address, version: tx_version, resource_bounds: max_resource_bounds, @@ -1247,7 +1248,8 @@ fn test_declare_tx( ); let fee_type = &account_tx.fee_type(); let tx_context = &block_context.to_tx_context(&account_tx); - let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); + let actual_execution_info = + account_tx.execute(state, block_context, tx_context.tx_info.enforce_fee(), true).unwrap(); //aviv: changed from true, true // Build expected validate call info. let expected_validate_call_info = declare_validate_callinfo( @@ -1259,14 +1261,24 @@ fn test_declare_tx( ); // Build expected fee transfer call info. - let expected_actual_fee = - actual_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type).unwrap(); - let expected_fee_transfer_call_info = expected_fee_transfer_call_info( - tx_context, - sender_address, - expected_actual_fee, - FeatureContract::ERC20(CairoVersion::Cairo0).get_class_hash(), - ); + // aviv added 'if' condition. not pretty solution: + let expected_actual_fee = if tx_context.tx_info.signed_version() == TransactionVersion::ZERO { + Fee(0) + } else { + actual_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type).unwrap() + }; + // aviv added 'if' condition. not pretty solution: + let expected_fee_transfer_call_info = + if tx_context.tx_info.signed_version() == TransactionVersion::ZERO { + None + } else { + expected_fee_transfer_call_info( + tx_context, + sender_address, + expected_actual_fee, + FeatureContract::ERC20(CairoVersion::Cairo0).get_class_hash(), + ) + }; let da_gas = starknet_resources.get_state_changes_cost(use_kzg_da); let expected_cairo_resources = get_expected_cairo_resources(