From e569b59e945c8e18a477e162e98e49bbcb3990cb Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Thu, 10 Oct 2024 16:47:41 +0300 Subject: [PATCH] test(blockifier): update test_insufficient_max_fee_reverts to include new resource bounds --- .../transaction/account_transactions_test.rs | 74 ++++++++++++++----- crates/starknet_api/src/transaction.rs | 21 ++++++ 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 78568ca227..4743a29191 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -1123,9 +1123,11 @@ fn test_max_fee_to_max_steps_conversion( /// recorded and max_fee is charged. fn test_insufficient_max_fee_reverts( block_context: BlockContext, - default_l1_resource_bounds: ValidResourceBounds, + #[values(default_l1_resource_bounds(), default_all_resource_bounds())] + resource_bounds: ValidResourceBounds, #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] cairo_version: CairoVersion, ) { + let gas_mode = resource_bounds.get_gas_vector_computation_mode(); let TestInitData { mut state, account_address, contract_address, mut nonce_manager } = create_test_init_data(&block_context.chain_info, cairo_version); let recursion_base_args = invoke_tx_args! { @@ -1137,7 +1139,7 @@ fn test_insufficient_max_fee_reverts( &mut state, &block_context, invoke_tx_args! { - resource_bounds: default_l1_resource_bounds, + resource_bounds, nonce: nonce_manager.next(account_address), calldata: recursive_function_calldata(&contract_address, 1, false), ..recursion_base_args.clone() @@ -1145,32 +1147,66 @@ fn test_insufficient_max_fee_reverts( ) .unwrap(); assert!(!tx_execution_info1.is_reverted()); - let actual_fee_depth1 = tx_execution_info1.receipt.fee; - let gas_price = - block_context.block_info.gas_prices.get_l1_gas_price_by_fee_type(&FeeType::Strk); - let gas_ammount = actual_fee_depth1.checked_div(gas_price).unwrap(); + + // Utility macro to verify the error and final fee is as expected, in each case of insufficient + // resource bounds. + macro_rules! verify_insufficient_bounds_result_by_gas_mode { + ($execution_info:expr, $expected_error_in_l1_bounds_mode:expr) => { + assert!($execution_info.is_reverted()); + // DA costs should be identical, regardless of bounds. + assert_eq!($execution_info.receipt.da_gas, tx_execution_info1.receipt.da_gas); + match gas_mode { + GasVectorComputationMode::NoL2Gas => { + assert!( + $execution_info + .revert_error + .unwrap() + .contains($expected_error_in_l1_bounds_mode) + ); + // See usages of the macro for documentation on why the NoL2Gas case should + // assert equality. + assert_eq!($execution_info.receipt.fee, tx_execution_info1.receipt.fee); + } + GasVectorComputationMode::All => { + assert!($execution_info.revert_error.unwrap().contains("no remaining steps")); + assert!($execution_info.receipt.fee <= tx_execution_info1.receipt.fee); + } + }; + }; + } // Invoke the `recurse` function with depth of 2 and the actual fee of depth 1 as max_fee. // This call should fail due to insufficient max fee (steps bound based on max_fee is not so // tight as to stop execution between iterations 1 and 2). + let resource_bounds_depth1 = match gas_mode { + GasVectorComputationMode::NoL2Gas => l1_resource_bounds( + tx_execution_info1.receipt.gas.l1_gas, + block_context.block_info.gas_prices.get_l1_gas_price_by_fee_type(&FeeType::Strk).into(), + ), + GasVectorComputationMode::All => ValidResourceBounds::all_bounds_from_vectors( + &tx_execution_info1.receipt.gas, + block_context.block_info.gas_prices.get_gas_prices_by_fee_type(&FeeType::Strk), + ), + }; let tx_execution_info2 = run_invoke_tx( &mut state, &block_context, invoke_tx_args! { - resource_bounds: l1_resource_bounds(gas_ammount, gas_price.into()), + resource_bounds: resource_bounds_depth1, nonce: nonce_manager.next(account_address), calldata: recursive_function_calldata(&contract_address, 2, false), ..recursion_base_args.clone() }, ) .unwrap(); - assert!(tx_execution_info2.is_reverted()); - assert!(tx_execution_info2.receipt.fee == actual_fee_depth1); - assert!( - tx_execution_info2 - .revert_error - .unwrap() - .starts_with(&format!("Insufficient max {resource}", resource = Resource::L1Gas)) + // In the deprecated bounds case, due to resource limit being estimated by steps, the execution + // will not fail due to insufficient resources; there are not enough steps in execution to hit + // the bound. Post-execution will fail due to insufficient max fee. + // The expected revert fee is therefore the same as the original fee (snap to bounds on + // post-execution error). + verify_insufficient_bounds_result_by_gas_mode!( + tx_execution_info2, + &format!("Insufficient max {resource}", resource = Resource::L1Gas) ); // Invoke the `recurse` function with depth of 824 and the actual fee of depth 1 as max_fee. @@ -1180,18 +1216,16 @@ fn test_insufficient_max_fee_reverts( &mut state, &block_context, invoke_tx_args! { - resource_bounds: l1_resource_bounds(gas_ammount, gas_price.into()), + resource_bounds: resource_bounds_depth1, nonce: nonce_manager.next(account_address), calldata: recursive_function_calldata(&contract_address, 824, false), ..recursion_base_args }, ) .unwrap(); - assert!(tx_execution_info3.is_reverted()); - assert!(tx_execution_info3.receipt.fee == actual_fee_depth1); - assert!( - tx_execution_info3.revert_error.unwrap().contains("RunResources has no remaining steps.") - ); + // In the deprecated bounds case, the entire step limit is derived from 100% of all the + // bounds (only L1 gas), so expected revert fee is identical to the original fee. + verify_insufficient_bounds_result_by_gas_mode!(tx_execution_info3, "no remaining steps"); } #[rstest] diff --git a/crates/starknet_api/src/transaction.rs b/crates/starknet_api/src/transaction.rs index 8164c3c797..2c0eeeb853 100644 --- a/crates/starknet_api/src/transaction.rs +++ b/crates/starknet_api/src/transaction.rs @@ -1139,6 +1139,27 @@ impl ValidResourceBounds { pub fn create_for_testing() -> Self { Self::L1Gas(ResourceBounds { max_amount: GasAmount(0), max_price_per_unit: GasPrice(1) }) } + + /// Utility method to "zip" an amount vector and a price vector to get an AllResourceBounds. + #[cfg(any(feature = "testing", test))] + pub fn all_bounds_from_vectors( + gas: &crate::execution_resources::GasVector, + prices: &crate::block::GasPriceVector, + ) -> Self { + let l1_gas = ResourceBounds { + max_amount: gas.l1_gas, + max_price_per_unit: prices.l1_gas_price.into(), + }; + let l2_gas = ResourceBounds { + max_amount: gas.l2_gas, + max_price_per_unit: prices.l2_gas_price.into(), + }; + let l1_data_gas = ResourceBounds { + max_amount: gas.l1_data_gas, + max_price_per_unit: prices.l1_data_gas_price.into(), + }; + Self::AllResources(AllResourceBounds { l1_gas, l2_gas, l1_data_gas }) + } } #[derive(