From ab670f8216f08bc1e3bb9fa6327f1d57252b03fa Mon Sep 17 00:00:00 2001 From: Aner Ben Efraim Date: Wed, 4 Dec 2024 17:50:30 +0200 Subject: [PATCH] fix(blockifier): fix tests for sierra_gas usage --- .../src/blockifier/stateful_validator.rs | 2 +- crates/blockifier/src/context.rs | 14 ++-- crates/blockifier/src/fee/fee_test.rs | 4 +- .../src/transaction/account_transaction.rs | 46 +++++----- .../transaction/account_transactions_test.rs | 13 ++- .../src/transaction/transaction_execution.rs | 2 +- .../src/transaction/transactions_test.rs | 83 ++++++++++++++----- crates/blockifier/src/versioned_constants.rs | 12 ++- 8 files changed, 113 insertions(+), 63 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index 1d2a12ae3e..47bae3c173 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -74,7 +74,7 @@ impl StatefulValidator { // `__validate__` call. let (_optional_call_info, actual_cost) = - self.validate(&tx, tx_context.initial_sierra_gas())?; + self.validate(&tx, tx_context.initial_sierra_gas().0)?; // Post validations. PostValidationReport::verify(&tx_context, &actual_cost)?; diff --git a/crates/blockifier/src/context.rs b/crates/blockifier/src/context.rs index a6c6ea31e2..5135744c66 100644 --- a/crates/blockifier/src/context.rs +++ b/crates/blockifier/src/context.rs @@ -44,8 +44,7 @@ impl TransactionContext { /// Returns the initial Sierra gas of the transaction. /// This value is used to limit the transaction's run. - // TODO(tzahi): replace returned value from u64 to GasAmount. - pub fn initial_sierra_gas(&self) -> u64 { + pub fn initial_sierra_gas(&self) -> GasAmount { match &self.tx_info { TransactionInfo::Deprecated(_) | TransactionInfo::Current(CurrentTransactionInfo { @@ -55,7 +54,7 @@ impl TransactionContext { TransactionInfo::Current(CurrentTransactionInfo { resource_bounds: ValidResourceBounds::AllResources(AllResourceBounds { l2_gas, .. }), .. - }) => l2_gas.max_amount.0, + }) => l2_gas.max_amount, } } } @@ -66,11 +65,11 @@ pub(crate) struct GasCounter { } impl GasCounter { - pub(crate) fn new(initial_gas: u64) -> Self { - GasCounter { spent_gas: GasAmount(0), remaining_gas: GasAmount(initial_gas) } + pub(crate) fn new(initial_gas: GasAmount) -> Self { + GasCounter { spent_gas: GasAmount(0), remaining_gas: initial_gas } } - pub(crate) fn spend(&mut self, amount: GasAmount) { + fn spend(&mut self, amount: GasAmount) { self.spent_gas = self.spent_gas.checked_add(amount).expect("Gas overflow"); self.remaining_gas = self .remaining_gas @@ -78,7 +77,8 @@ impl GasCounter { .expect("Overuse of gas; should have been caught earlier"); } - pub(crate) fn cap_usage(&self, amount: GasAmount) -> u64 { + /// Limits the amount of gas that can be used (in validate\execute) by the given global limit. + pub(crate) fn limit_usage(&self, amount: GasAmount) -> u64 { self.remaining_gas.min(amount).0 } diff --git a/crates/blockifier/src/fee/fee_test.rs b/crates/blockifier/src/fee/fee_test.rs index 98ad761ef4..88ee0262fa 100644 --- a/crates/blockifier/src/fee/fee_test.rs +++ b/crates/blockifier/src/fee/fee_test.rs @@ -361,7 +361,7 @@ fn test_get_fee_by_gas_vector_overflow( #[rstest] #[case::default( - VersionedConstants::create_for_account_testing().default_initial_gas_cost(), + VersionedConstants::create_for_account_testing().default_initial_gas_amount().0, GasVectorComputationMode::NoL2Gas )] #[case::from_l2_gas(4321, GasVectorComputationMode::All)] @@ -384,6 +384,6 @@ fn test_initial_sierra_gas( }), }; let account_tx = account_invoke_tx(invoke_tx_args!(resource_bounds)); - let actual = block_context.to_tx_context(&account_tx).initial_sierra_gas(); + let actual = block_context.to_tx_context(&account_tx).initial_sierra_gas().0; assert_eq!(actual, expected) } diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 0726f1ec48..cbf28dd6f8 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -354,21 +354,13 @@ impl AccountTransaction { if validate { // TODO(Aner): cap the gas for validation. let remaining_validation_gas = &mut remaining_gas - .cap_usage(tx_context.block_context.versioned_constants.validate_max_sierra_gas); - let call_info = self.validate_tx( - state, - tx_context, - remaining_validation_gas, - limit_steps_by_resources, - )?; - match call_info { - // TODO(Aner): Update the gas counter. - Some(call_info) => { - remaining_gas.subtract_used_gas(&call_info); - Ok(Some(call_info)) - } - None => Ok(None), - } + .limit_usage(tx_context.block_context.versioned_constants.validate_max_sierra_gas); + Ok(self + .validate_tx(state, tx_context, remaining_validation_gas, limit_steps_by_resources)? + .inspect(|call_info| { + // TODO(Aner): Update the gas counter. + remaining_gas.subtract_used_gas(call_info); + })) } else { Ok(None) } @@ -498,24 +490,24 @@ impl AccountTransaction { remaining_gas: &mut GasCounter, ) -> TransactionExecutionResult> { // TODO(Aner): cap the gas usage for execution. - let remaining_execution_gas = &mut remaining_gas - .cap_usage(context.tx_context.block_context.versioned_constants.execute_max_sierra_gas); - - let call_info = match &self.tx { + let remaining_execution_gas = &mut remaining_gas.limit_usage( + context + .tx_context + .block_context + .versioned_constants + .sierra_gas_limit(&context.execution_mode), + ); + Ok(match &self.tx { Transaction::Declare(tx) => tx.run_execute(state, context, remaining_execution_gas), Transaction::DeployAccount(tx) => { tx.run_execute(state, context, remaining_execution_gas) } Transaction::Invoke(tx) => tx.run_execute(state, context, remaining_execution_gas), - }?; - match call_info { + }? + .inspect(|call_info| { // TODO(Aner): Update the gas counter. - Some(call_info) => { - remaining_gas.subtract_used_gas(&call_info); - Ok(Some(call_info)) - } - None => Ok(None), - } + remaining_gas.subtract_used_gas(call_info); + })) } fn run_non_revertible( diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index a016105508..56c2c52bad 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -1684,13 +1684,22 @@ fn test_initial_gas( let validate_call_info = &transaction_ex_info.validate_call_info.unwrap(); let validate_initial_gas = validate_call_info.call.initial_gas; - assert_eq!(validate_initial_gas, DEFAULT_L2_GAS_MAX_AMOUNT.0); + assert_eq!(validate_initial_gas, block_context.versioned_constants.validate_max_sierra_gas.0); let validate_gas_consumed = validate_call_info.execution.gas_consumed; assert!(validate_gas_consumed > 0, "New Cairo1 contract should consume gas."); let default_call_info = CallInfo::default(); - let mut prev_initial_gas = validate_initial_gas; let mut execute_call_info = &transaction_ex_info.execute_call_info.unwrap(); + // Initial gas for execution is the minimum between the max execution gas and the initial gas + // minus the gas consumed by validate. + let mut prev_initial_gas = block_context + .versioned_constants + .max_execution_sierra_gas() + .min( + block_context.versioned_constants.default_initial_gas_amount() + - GasAmount(validate_gas_consumed), + ) + .0; let mut curr_initial_gas; let mut started_vm_mode = false; // The __validate__ call of a the account contract. diff --git a/crates/blockifier/src/transaction/transaction_execution.rs b/crates/blockifier/src/transaction/transaction_execution.rs index d305e0bdb8..2529fbc7fa 100644 --- a/crates/blockifier/src/transaction/transaction_execution.rs +++ b/crates/blockifier/src/transaction/transaction_execution.rs @@ -143,7 +143,7 @@ impl ExecutableTransaction for L1HandlerTransaction { let limit_steps_by_resources = false; let mut context = EntryPointExecutionContext::new_invoke(tx_context.clone(), limit_steps_by_resources); - let mut remaining_gas = tx_context.initial_sierra_gas(); + let mut remaining_gas = tx_context.initial_sierra_gas().0; let execute_call_info = self.run_execute(state, &mut context, &mut remaining_gas)?; let l1_handler_payload_size = self.payload_size(); let TransactionReceipt { diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index 73c64c5d83..494ccedcfc 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -1,3 +1,4 @@ +use std::cmp::min; use std::collections::{HashMap, HashSet}; use std::sync::{Arc, LazyLock}; @@ -160,6 +161,11 @@ use crate::{ static VERSIONED_CONSTANTS: LazyLock = LazyLock::new(VersionedConstants::create_for_testing); +#[fixture] +fn default_initial_gas_amount() -> GasAmount { + VERSIONED_CONSTANTS.default_initial_gas_amount() +} + #[fixture] fn default_initial_gas_cost() -> u64 { VERSIONED_CONSTANTS.default_initial_gas_cost() @@ -179,7 +185,7 @@ struct ExpectedResultTestInvokeTx { fn user_initial_gas_from_bounds(bounds: ValidResourceBounds) -> Option { match bounds { - ValidResourceBounds::L1Gas(_) => None, + ValidResourceBounds::L1Gas(_) => Some(default_initial_gas_amount()), ValidResourceBounds::AllResources(bounds) => Some(bounds.l2_gas.max_amount), } } @@ -229,11 +235,19 @@ fn expected_validate_call_info( builtin_instance_counter: HashMap::from([(BuiltinName::range_check, n_range_checks)]), } .filter_unused_builtins(); - let initial_gas = match tracked_resource { - TrackedResource::CairoSteps => default_initial_gas_cost(), - TrackedResource::SierraGas => { - user_initial_gas.unwrap_or(GasAmount(default_initial_gas_cost())).0 - } + let initial_gas = match cairo_version { + CairoVersion::Cairo0 => default_initial_gas_cost(), + CairoVersion::Cairo1 => match tracked_resource { + TrackedResource::CairoSteps => { + default_initial_gas_amount().min(VERSIONED_CONSTANTS.max_validation_sierra_gas()).0 + } + TrackedResource::SierraGas => { + user_initial_gas + .unwrap_or(default_initial_gas_amount()) + .min(VERSIONED_CONSTANTS.max_validation_sierra_gas()) + .0 + } + }, }; Some(CallInfo { @@ -487,14 +501,10 @@ fn test_invoke_tx( let tracked_resource = account_contract .get_runnable_class() .tracked_resource(&versioned_constants.min_compiler_version_for_sierra_gas, None); - if tracked_resource == TrackedResource::CairoSteps { - // In CairoSteps mode, the initial gas is set to the default once before the validate call. - expected_arguments.inner_call_initial_gas -= - expected_arguments.validate_gas_consumed + expected_arguments.execute_gas_consumed - } // Build expected validate call info. let expected_account_class_hash = account_contract.get_class_hash(); + let initial_gas = user_initial_gas_from_bounds(resource_bounds); let expected_validate_call_info = expected_validate_call_info( expected_account_class_hash, constants::VALIDATE_ENTRY_POINT_NAME, @@ -503,11 +513,39 @@ fn test_invoke_tx( sender_address, account_cairo_version, tracked_resource, - user_initial_gas_from_bounds(resource_bounds), + initial_gas.min(Some(VERSIONED_CONSTANTS.max_validation_sierra_gas())), ); // Build expected execute call info. let expected_return_result_calldata = vec![felt!(2_u8)]; + + let expected_validated_call = expected_validate_call_info.as_ref().unwrap().call.clone(); + let expected_initial_execution_gas = VERSIONED_CONSTANTS + .max_execution_sierra_gas() + .min(initial_gas.unwrap() - GasAmount(expected_arguments.validate_gas_consumed)) + .0; + if account_cairo_version == CairoVersion::Cairo0 { + expected_arguments.inner_call_initial_gas = VERSIONED_CONSTANTS.default_initial_gas_cost(); + } + let expected_execute_call = CallEntryPoint { + entry_point_selector: selector_from_name(constants::EXECUTE_ENTRY_POINT_NAME), + initial_gas: if account_cairo_version == CairoVersion::Cairo0 { + VERSIONED_CONSTANTS.default_initial_gas_cost() + } else { + expected_initial_execution_gas + }, // expected_validated_call.initial_gas - expected_arguments.validate_gas_consumed, + ..expected_validated_call + }; + if tracked_resource == TrackedResource::CairoSteps { + // else { + expected_arguments.inner_call_initial_gas -= expected_arguments.validate_gas_consumed; + expected_arguments.inner_call_initial_gas = min( + expected_arguments.inner_call_initial_gas, + VERSIONED_CONSTANTS.max_execution_sierra_gas().0, + ); + expected_arguments.inner_call_initial_gas -= expected_arguments.execute_gas_consumed; + // } + } let expected_return_result_call = CallEntryPoint { entry_point_selector: selector_from_name("return_result"), class_hash: Some(test_contract.get_class_hash()), @@ -517,13 +555,11 @@ fn test_invoke_tx( storage_address: test_contract_address, caller_address: sender_address, call_type: CallType::Call, - initial_gas: expected_arguments.inner_call_initial_gas, - }; - let expected_validated_call = expected_validate_call_info.as_ref().unwrap().call.clone(); - let expected_execute_call = CallEntryPoint { - entry_point_selector: selector_from_name(constants::EXECUTE_ENTRY_POINT_NAME), - initial_gas: expected_validated_call.initial_gas - expected_arguments.validate_gas_consumed, - ..expected_validated_call + initial_gas: if account_cairo_version == CairoVersion::Cairo0 { + VERSIONED_CONSTANTS.default_initial_gas_cost() + } else { + expected_arguments.inner_call_initial_gas + }, }; let expected_return_result_retdata = Retdata(expected_return_result_calldata); let expected_execute_call_info = Some(CallInfo { @@ -1772,6 +1808,11 @@ fn test_deploy_account_tx( ); // Build expected execute call info. + let expected_execute_initial_gas = user_initial_gas + .unwrap_or(default_initial_gas_amount()) + // Note that in the case of deploy account, the initial gas in "execute" is limited by + // max_validation_sierra_gas. + .min(VERSIONED_CONSTANTS.max_validation_sierra_gas()); let expected_execute_call_info = Some(CallInfo { call: CallEntryPoint { class_hash: Some(account_class_hash), @@ -1779,7 +1820,7 @@ fn test_deploy_account_tx( entry_point_type: EntryPointType::Constructor, entry_point_selector: selector_from_name(CONSTRUCTOR_ENTRY_POINT_NAME), storage_address: deployed_account_address, - initial_gas: user_initial_gas.unwrap_or(GasAmount(default_initial_gas_cost())).0, + initial_gas: expected_execute_initial_gas.0, ..Default::default() }, ..Default::default() @@ -2289,7 +2330,7 @@ fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) { storage_address: contract_address, caller_address: ContractAddress::default(), call_type: CallType::Call, - initial_gas: default_initial_gas_cost(), + initial_gas: default_initial_gas_amount().0, }, execution: CallExecution { retdata: Retdata(vec![value]), diff --git a/crates/blockifier/src/versioned_constants.rs b/crates/blockifier/src/versioned_constants.rs index 49a475433f..27f5a9662f 100644 --- a/crates/blockifier/src/versioned_constants.rs +++ b/crates/blockifier/src/versioned_constants.rs @@ -22,6 +22,7 @@ use starknet_api::transaction::fields::GasVectorComputationMode; use strum::IntoEnumIterator; use thiserror::Error; +use crate::execution::common_hints::ExecutionMode; use crate::execution::deprecated_syscalls::hint_processor::SyscallCounter; use crate::execution::execution_utils::poseidon_hash_many_cost; use crate::execution::syscalls::SyscallSelector; @@ -249,10 +250,17 @@ impl VersionedConstants { } /// Default initial gas amount when L2 gas is not provided. - pub fn default_initial_gas_amount(&self) -> u64 { + pub fn default_initial_gas_amount(&self) -> GasAmount { (self.execute_max_sierra_gas.checked_add(self.validate_max_sierra_gas)) .expect("The default initial gas cost should be less than the maximum gas amount.") - .0 + } + + /// Returns the maximum gas amount according to the given mode. + pub fn sierra_gas_limit(&self, mode: &ExecutionMode) -> GasAmount { + match mode { + ExecutionMode::Validate => self.validate_max_sierra_gas, + ExecutionMode::Execute => self.execute_max_sierra_gas, + } } /// Returns the maximum gas amount for validation.