From 778073750cd19420f474921a10f4179b6b8f0e14 Mon Sep 17 00:00:00 2001 From: Nimrod Weiss Date: Wed, 14 Aug 2024 14:25:00 +0300 Subject: [PATCH] build(fee): tx_resources depend on resource bounds signature --- crates/blockifier/src/fee/actual_cost.rs | 6 +-- .../blockifier/src/test_utils/struct_impls.rs | 2 + .../transaction/account_transactions_test.rs | 14 +++++- .../src/transaction/execution_flavors_test.rs | 30 ++++++++++-- crates/blockifier/src/transaction/objects.rs | 22 ++++++++- .../src/transaction/post_execution_test.rs | 6 ++- .../src/transaction/transactions_test.rs | 49 +++++++++++++------ crates/starknet_api/src/transaction.rs | 7 +++ 8 files changed, 109 insertions(+), 27 deletions(-) diff --git a/crates/blockifier/src/fee/actual_cost.rs b/crates/blockifier/src/fee/actual_cost.rs index 301c91da38c..21da56177cd 100644 --- a/crates/blockifier/src/fee/actual_cost.rs +++ b/crates/blockifier/src/fee/actual_cost.rs @@ -85,11 +85,7 @@ impl TransactionReceipt { vm_resources: cairo_resources, n_reverted_steps: reverted_steps, }; - - let gas = tx_resources.to_gas_vector( - &tx_context.block_context.versioned_constants, - tx_context.block_context.block_info.use_kzg_da, - )?; + let gas = tx_resources.to_gas_vector_with_context(tx_context)?; // 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 { diff --git a/crates/blockifier/src/test_utils/struct_impls.rs b/crates/blockifier/src/test_utils/struct_impls.rs index da0ad97472f..ecdb0f523d9 100644 --- a/crates/blockifier/src/test_utils/struct_impls.rs +++ b/crates/blockifier/src/test_utils/struct_impls.rs @@ -114,10 +114,12 @@ impl TransactionResources { &self, block_context: &BlockContext, fee_type: &FeeType, + include_l2_gas: bool, ) -> TransactionFeeResult { let gas_vector = self.to_gas_vector( &block_context.versioned_constants, block_context.block_info.use_kzg_da, + include_l2_gas, )?; Ok(get_fee_by_gas_vector(&block_context.block_info, gas_vector, fee_type)) } diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 98c52fab069..f10105b4441 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -930,6 +930,7 @@ 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 has_l2_gas_bounds = tx_context1.tx_info.has_l2_gas_bounds(); 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(); @@ -937,7 +938,11 @@ fn test_max_fee_to_max_steps_conversion( let gas_used_vector1 = tx_execution_info1 .receipt .resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + has_l2_gas_bounds, + ) .unwrap(); // Second invocation of `with_arg` gets twice the pre-calculated actual fee as max_fee. @@ -950,6 +955,7 @@ 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 has_l2_gas_bounds = tx_context2.tx_info.has_l2_gas_bounds(); 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(); @@ -957,7 +963,11 @@ fn test_max_fee_to_max_steps_conversion( let gas_used_vector2 = tx_execution_info2 .receipt .resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + has_l2_gas_bounds, + ) .unwrap(); // Test that steps limit doubles as max_fee doubles, but actual consumed steps and fee remains. diff --git a/crates/blockifier/src/transaction/execution_flavors_test.rs b/crates/blockifier/src/transaction/execution_flavors_test.rs index ca9442bb9bf..189c47787f3 100644 --- a/crates/blockifier/src/transaction/execution_flavors_test.rs +++ b/crates/blockifier/src/transaction/execution_flavors_test.rs @@ -124,12 +124,17 @@ fn check_gas_and_fee( expected_actual_gas: u64, expected_actual_fee: Fee, expected_cost_of_resources: Fee, + include_l2_gas: bool, ) { assert_eq!( tx_execution_info .receipt .resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + include_l2_gas + ) .unwrap() .l1_gas, expected_actual_gas.into() @@ -139,7 +144,11 @@ fn check_gas_and_fee( // Future compatibility: resources other than the L1 gas usage may affect the fee (currently, // `calculate_tx_fee` is simply the result of `calculate_tx_gas_usage_vector` times gas price). assert_eq!( - tx_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type).unwrap(), + tx_execution_info + .receipt + .resources + .calculate_tx_fee(block_context, fee_type, include_l2_gas) + .unwrap(), expected_cost_of_resources ); } @@ -171,6 +180,7 @@ fn test_simulate_validate_charge_fee_pre_validate( // The max resource bounds fixture is not used here because this function already has the // maximum number of arguments. let resource_bounds = l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE); + let has_l2_gas = resource_bounds.has_l2_gas(); let gas_price = block_context.block_info.gas_prices.get_l1_gas_price_by_fee_type(&fee_type); let FlavorTestInitialState { mut state, @@ -238,6 +248,7 @@ fn test_simulate_validate_charge_fee_pre_validate( actual_gas_used, actual_fee, actual_fee, + has_l2_gas, ); } else { nonce_manager.rollback(account_address); @@ -282,6 +293,7 @@ fn test_simulate_validate_charge_fee_pre_validate( actual_gas_used, actual_fee, actual_fee, + has_l2_gas, ); } else { nonce_manager.rollback(account_address); @@ -322,6 +334,7 @@ fn test_simulate_validate_charge_fee_pre_validate( actual_gas_used, actual_fee, actual_fee, + has_l2_gas, ); } else { nonce_manager.rollback(account_address); @@ -351,6 +364,7 @@ fn test_simulate_validate_charge_fee_fail_validate( #[case] fee_type: FeeType, max_resource_bounds: ValidResourceBounds, ) { + let has_l2_gas = max_resource_bounds.has_l2_gas(); let block_context = BlockContext::create_for_account_testing(); let max_fee = Fee(MAX_FEE); @@ -391,6 +405,7 @@ fn test_simulate_validate_charge_fee_fail_validate( actual_gas_used, actual_fee, actual_fee, + has_l2_gas, ); } else { assert!( @@ -413,6 +428,7 @@ fn test_simulate_validate_charge_fee_mid_execution( #[case] fee_type: FeeType, max_resource_bounds: ValidResourceBounds, ) { + let has_l2_gas = max_resource_bounds.has_l2_gas(); let block_context = BlockContext::create_for_account_testing(); let chain_info = &block_context.chain_info; let gas_price = block_context.block_info.gas_prices.get_l1_gas_price_by_fee_type(&fee_type); @@ -462,6 +478,7 @@ fn test_simulate_validate_charge_fee_mid_execution( revert_gas_used, revert_fee, revert_fee, + has_l2_gas, ); let current_balance = check_balance( current_balance, @@ -513,6 +530,7 @@ fn test_simulate_validate_charge_fee_mid_execution( // Complete resources used are reported as receipt.resources; but only the // charged final fee is shown in actual_fee. if charge_fee { limited_fee } else { unlimited_fee }, + has_l2_gas, ); let current_balance = check_balance(current_balance, &state, account_address, chain_info, &fee_type, charge_fee); @@ -553,10 +571,10 @@ fn test_simulate_validate_charge_fee_mid_execution( block_limit_gas, block_limit_fee, block_limit_fee, + has_l2_gas, ); check_balance(current_balance, &state, account_address, chain_info, &fee_type, charge_fee); } - #[rstest] #[case(TransactionVersion::ONE, FeeType::Eth, true)] #[case(TransactionVersion::THREE, FeeType::Strk, false)] @@ -614,9 +632,11 @@ fn test_simulate_validate_charge_fee_post_execution( validate, &fee_type, ); + let resource_bounds = l1_resource_bounds(just_not_enough_gas_bound, gas_price.into()); + let has_l2_gas = resource_bounds.has_l2_gas(); let tx_execution_info = account_invoke_tx(invoke_tx_args! { max_fee: just_not_enough_fee_bound, - resource_bounds: l1_resource_bounds(just_not_enough_gas_bound, gas_price.into()), + resource_bounds, calldata: recurse_calldata(test_contract_address, false, 1000), nonce: nonce_manager.next(account_address), sender_address: account_address, @@ -641,6 +661,7 @@ fn test_simulate_validate_charge_fee_post_execution( if charge_fee { revert_gas_usage } else { unlimited_gas_used }, if charge_fee { just_not_enough_fee_bound } else { unlimited_fee }, if charge_fee { revert_fee } else { unlimited_fee }, + has_l2_gas, ); let current_balance = check_balance(current_balance, &state, account_address, chain_info, &fee_type, charge_fee); @@ -705,6 +726,7 @@ fn test_simulate_validate_charge_fee_post_execution( if charge_fee { fail_actual_gas } else { success_actual_gas }, actual_fee, if charge_fee { fail_actual_fee } else { actual_fee }, + has_l2_gas, ); check_balance( current_balance, diff --git a/crates/blockifier/src/transaction/objects.rs b/crates/blockifier/src/transaction/objects.rs index 95f35ede33c..caffb721406 100644 --- a/crates/blockifier/src/transaction/objects.rs +++ b/crates/blockifier/src/transaction/objects.rs @@ -23,6 +23,7 @@ use strum_macros::EnumIter; use crate::abi::constants as abi_constants; use crate::blockifier::block::{BlockInfo, GasPrices}; +use crate::context::TransactionContext; use crate::execution::call_info::{CallInfo, ExecutionSummary, MessageL1CostInfo, OrderedEvent}; use crate::fee::actual_cost::TransactionReceipt; use crate::fee::eth_gas_constants; @@ -108,6 +109,13 @@ impl TransactionInfo { TransactionInfo::Deprecated(context) => context.max_fee != Fee(0), } } + + pub fn has_l2_gas_bounds(&self) -> bool { + match self { + TransactionInfo::Current(context) => context.resource_bounds.has_l2_gas(), + TransactionInfo::Deprecated(_) => false, + } + } } impl HasRelatedFeeType for TransactionInfo { @@ -489,8 +497,9 @@ impl TransactionResources { &self, versioned_constants: &VersionedConstants, use_kzg_da: bool, + include_l2_gas: bool, ) -> TransactionFeeResult { - Ok(self.starknet_resources.to_gas_vector(versioned_constants, use_kzg_da, false) + Ok(self.starknet_resources.to_gas_vector(versioned_constants, use_kzg_da, include_l2_gas) + calculate_l1_gas_by_vm_usage( versioned_constants, &self.vm_resources, @@ -498,6 +507,17 @@ impl TransactionResources { )?) } + pub fn to_gas_vector_with_context( + &self, + tx_context: &TransactionContext, + ) -> TransactionFeeResult { + self.to_gas_vector( + &tx_context.block_context.versioned_constants, + tx_context.block_context.block_info.use_kzg_da, + tx_context.tx_info.has_l2_gas_bounds(), + ) + } + pub fn to_resources_mapping( &self, versioned_constants: &VersionedConstants, diff --git a/crates/blockifier/src/transaction/post_execution_test.rs b/crates/blockifier/src/transaction/post_execution_test.rs index 95d7f7573de..cdab68f16bb 100644 --- a/crates/blockifier/src/transaction/post_execution_test.rs +++ b/crates/blockifier/src/transaction/post_execution_test.rs @@ -256,7 +256,11 @@ fn test_revert_on_resource_overuse( let actual_gas_usage: u64 = execution_info_measure .receipt .resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + false, // TODO(Nimrod): Derive this value from `max_resource_bounds`. + ) .unwrap() .l1_gas .try_into() diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index b1c1921f846..7020d0c565f 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -475,8 +475,11 @@ fn test_invoke_tx( // Build expected fee transfer call info. let fee_type = &tx_context.tx_info.fee_type(); - let expected_actual_fee = - actual_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type).unwrap(); + let expected_actual_fee = actual_execution_info + .receipt + .resources + .calculate_tx_fee(block_context, fee_type, tx_context.tx_info.has_l2_gas_bounds()) + .unwrap(); let expected_fee_transfer_call_info = expected_fee_transfer_call_info( &tx_context, sender_address, @@ -507,7 +510,11 @@ fn test_invoke_tx( ); let total_gas = expected_actual_resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + tx_context.tx_info.has_l2_gas_bounds(), + ) .unwrap(); let expected_execution_info = TransactionExecutionInfo { @@ -1198,8 +1205,11 @@ 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_actual_fee = actual_execution_info + .receipt + .resources + .calculate_tx_fee(block_context, fee_type, tx_context.tx_info.has_l2_gas_bounds()) + .unwrap(); let expected_fee_transfer_call_info = expected_fee_transfer_call_info( tx_context, sender_address, @@ -1228,8 +1238,9 @@ fn test_declare_tx( use_kzg_da, ); - let expected_total_gas = - expected_actual_resources.to_gas_vector(versioned_constants, use_kzg_da).unwrap(); + let expected_total_gas = expected_actual_resources + .to_gas_vector(versioned_constants, use_kzg_da, tx_context.tx_info.has_l2_gas_bounds()) + .unwrap(); let expected_execution_info = TransactionExecutionInfo { validate_call_info: expected_validate_call_info, @@ -1359,8 +1370,11 @@ fn test_deploy_account_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_actual_fee = actual_execution_info + .receipt + .resources + .calculate_tx_fee(block_context, fee_type, tx_context.tx_info.has_l2_gas_bounds()) + .unwrap(); let expected_fee_transfer_call_info = expected_fee_transfer_call_info( tx_context, deployed_account_address, @@ -1397,7 +1411,11 @@ fn test_deploy_account_tx( ); let expected_total_gas = actual_resources - .to_gas_vector(&block_context.versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector( + &block_context.versioned_constants, + block_context.block_info.use_kzg_da, + tx_context.tx_info.has_l2_gas_bounds(), + ) .unwrap(); let expected_execution_info = TransactionExecutionInfo { @@ -1920,7 +1938,7 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) { ); let total_gas = expected_tx_resources - .to_gas_vector(versioned_constants, block_context.block_info.use_kzg_da) + .to_gas_vector(versioned_constants, block_context.block_info.use_kzg_da, false) .unwrap(); // Build the expected execution info. @@ -1954,9 +1972,12 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) { 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(); // 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)) - .unwrap(); + let expected_actual_fee = (expected_execution_info.receipt.resources.calculate_tx_fee( + block_context, + &FeeType::Eth, + false, + )) + .unwrap(); assert_matches!( error, TransactionExecutionError::TransactionFeeError( diff --git a/crates/starknet_api/src/transaction.rs b/crates/starknet_api/src/transaction.rs index 43095c85c72..324b99187d1 100644 --- a/crates/starknet_api/src/transaction.rs +++ b/crates/starknet_api/src/transaction.rs @@ -970,6 +970,13 @@ impl ValidResourceBounds { } } } + + pub fn has_l2_gas(&self) -> bool { + match self { + ValidResourceBounds::L1Gas(_) => false, + ValidResourceBounds::AllResources(_) => true, + } + } } #[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize)]