From 512e8efa030aab208a4c46ee530a228bc7362d91 Mon Sep 17 00:00:00 2001 From: dorimedini-starkware Date: Wed, 30 Oct 2024 14:17:38 +0200 Subject: [PATCH] test(blockifier): update test_insufficient_max_fee_reverts to include new resource bounds (#1313) 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/1313) --- .../transaction/account_transactions_test.rs | 54 +++++++++++++------ crates/starknet_api/src/transaction.rs | 21 ++++++++ 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 3f5fd009c6..a47d31dcdb 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -1164,9 +1164,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! { @@ -1178,7 +1180,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() @@ -1186,32 +1188,51 @@ 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(); - // 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). + // Invoke the `recurse` function with depth of 2 and the actual gas usage of depth 1 as the + // resource bounds. This call should fail in post-execution due to insufficient max gas (steps + // bound based on the resource bounds are not so tight as to stop execution between iterations 1 + // and 2). + let resource_used_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_used_depth1, nonce: nonce_manager.next(account_address), calldata: recursive_function_calldata(&contract_address, 2, false), ..recursion_base_args.clone() }, ) .unwrap(); + // In the L1 gas 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). + let overdraft_resource = match gas_mode { + GasVectorComputationMode::NoL2Gas => Resource::L1Gas, + GasVectorComputationMode::All => Resource::L2Gas, + }; assert!(tx_execution_info2.is_reverted()); - assert!(tx_execution_info2.receipt.fee == actual_fee_depth1); + // DA costs should be identical, regardless of bounds; as should the final fee (computed by + // snapping to bounds). + assert_eq!(tx_execution_info2.receipt.da_gas, tx_execution_info1.receipt.da_gas); + assert_eq!(tx_execution_info2.receipt.fee, tx_execution_info1.receipt.fee); assert!( tx_execution_info2 .revert_error .unwrap() - .starts_with(&format!("Insufficient max {resource}", resource = Resource::L1Gas)) + .contains(&format!("Insufficient max {overdraft_resource}")) ); // Invoke the `recurse` function with depth of 824 and the actual fee of depth 1 as max_fee. @@ -1221,7 +1242,7 @@ 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_used_depth1, nonce: nonce_manager.next(account_address), calldata: recursive_function_calldata(&contract_address, 824, false), ..recursion_base_args @@ -1229,10 +1250,9 @@ fn test_insufficient_max_fee_reverts( ) .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.") - ); + assert_eq!(tx_execution_info3.receipt.da_gas, tx_execution_info1.receipt.da_gas); + assert_eq!(tx_execution_info3.receipt.fee, tx_execution_info1.receipt.fee); + assert!(tx_execution_info3.revert_error.unwrap().contains("no remaining steps")); } #[rstest] diff --git a/crates/starknet_api/src/transaction.rs b/crates/starknet_api/src/transaction.rs index 7a88857833..c4212ef428 100644 --- a/crates/starknet_api/src/transaction.rs +++ b/crates/starknet_api/src/transaction.rs @@ -1153,6 +1153,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(