From 64cb2546c4ff97be5ae2cd9e906fe1dedf42e6e2 Mon Sep 17 00:00:00 2001 From: aner-starkware <147302140+aner-starkware@users.noreply.github.com> Date: Tue, 24 Sep 2024 18:05:37 +0300 Subject: [PATCH] chore(blockifier): check max price of l1_data_gas and l2_gas (#710) --- crates/blockifier/src/blockifier/block.rs | 15 +++ .../src/transaction/account_transaction.rs | 92 ++++++++++++++----- .../src/transaction/transactions_test.rs | 2 + crates/starknet_api/src/transaction.rs | 2 +- 4 files changed, 85 insertions(+), 26 deletions(-) diff --git a/crates/blockifier/src/blockifier/block.rs b/crates/blockifier/src/blockifier/block.rs index 75e08dbc70..2bf9571fd4 100644 --- a/crates/blockifier/src/blockifier/block.rs +++ b/crates/blockifier/src/blockifier/block.rs @@ -37,6 +37,13 @@ pub struct GasPrices { strk_l2_gas_price: NonZeroU128, // In fri. } +#[derive(Debug)] +pub struct GasPricesForFeeType { + pub l1_gas_price: NonZeroU128, + pub l1_data_gas_price: NonZeroU128, + pub l2_gas_price: NonZeroU128, +} + impl GasPrices { pub fn new( eth_l1_gas_price: NonZeroU128, @@ -96,6 +103,14 @@ impl GasPrices { FeeType::Eth => self.eth_l2_gas_price, } } + + pub fn get_gas_prices_by_fee_type(&self, fee_type: &FeeType) -> GasPricesForFeeType { + GasPricesForFeeType { + l1_gas_price: self.get_l1_gas_price_by_fee_type(fee_type), + l1_data_gas_price: self.get_l1_data_gas_price_by_fee_type(fee_type), + l2_gas_price: self.get_l2_gas_price_by_fee_type(fee_type), + } + } } // Block pre-processing. diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 15d30dd554..c03304bde0 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -4,17 +4,20 @@ use cairo_vm::vm::runners::cairo_runner::ExecutionResources; use starknet_api::calldata; use starknet_api::core::{ContractAddress, EntryPointSelector}; use starknet_api::deprecated_contract_class::EntryPointType; +use starknet_api::transaction::Resource::{L1DataGas, L1Gas, L2Gas}; use starknet_api::transaction::{ + AllResourceBounds, Calldata, Fee, - Resource, ResourceBounds, TransactionHash, TransactionVersion, + ValidResourceBounds, }; use starknet_types_core::felt::Felt; use crate::abi::abi_utils::selector_from_name; +use crate::blockifier::block::GasPricesForFeeType; use crate::context::{BlockContext, TransactionContext}; use crate::execution::call_info::{CallInfo, Retdata}; use crate::execution::contract_class::ContractClass; @@ -241,35 +244,74 @@ impl AccountTransaction { let block_info = &block_context.block_info; let fee_type = &tx_info.fee_type(); match tx_info { - TransactionInfo::Current(context) => { - let ResourceBounds { + TransactionInfo::Current(context) => match &context.resource_bounds { + ValidResourceBounds::L1Gas(ResourceBounds { max_amount: max_l1_gas_amount, max_price_per_unit: max_l1_gas_price, - } = context.l1_resource_bounds(); + }) => { + let max_l1_gas_amount_as_u128: u128 = (*max_l1_gas_amount).into(); + if max_l1_gas_amount_as_u128 < minimal_l1_gas_amount { + return Err(TransactionFeeError::MaxGasAmountTooLow { + resource: L1Gas, + max_gas_amount: *max_l1_gas_amount, + // TODO(Ori, 1/2/2024): Write an indicative expect message + // explaining why the conversion + // works. + minimal_gas_amount: (minimal_l1_gas_amount + .try_into() + .expect("Failed to convert u128 to u64.")), + })?; + } - let max_l1_gas_amount_as_u128: u128 = max_l1_gas_amount.into(); - if max_l1_gas_amount_as_u128 < minimal_l1_gas_amount { - return Err(TransactionFeeError::MaxGasAmountTooLow { - resource: Resource::L1Gas, - max_gas_amount: max_l1_gas_amount, - // TODO(Ori, 1/2/2024): Write an indicative expect message explaining why - // the convertion works. - minimal_gas_amount: (minimal_l1_gas_amount - .try_into() - .expect("Failed to convert u128 to u64.")), - })?; + let actual_l1_gas_price = + block_info.gas_prices.get_l1_gas_price_by_fee_type(fee_type); + if *max_l1_gas_price < actual_l1_gas_price.into() { + return Err(TransactionFeeError::MaxGasPriceTooLow { + resource: L1Gas, + max_gas_price: *max_l1_gas_price, + actual_gas_price: actual_l1_gas_price.into(), + })?; + } } - - let actual_l1_gas_price = - block_info.gas_prices.get_l1_gas_price_by_fee_type(fee_type); - if max_l1_gas_price < actual_l1_gas_price.into() { - return Err(TransactionFeeError::MaxGasPriceTooLow { - resource: Resource::L1Gas, - max_gas_price: max_l1_gas_price, - actual_gas_price: actual_l1_gas_price.into(), - })?; + ValidResourceBounds::AllResources(AllResourceBounds { + l1_gas, + l2_gas, + l1_data_gas, + }) => { + let max_l1_gas_amount_as_u128: u128 = l1_gas.max_amount.into(); + if max_l1_gas_amount_as_u128 < minimal_l1_gas_amount { + return Err(TransactionFeeError::MaxGasAmountTooLow { + resource: L1Gas, + max_gas_amount: l1_gas.max_amount, + // TODO(Ori, 1/2/2024): Write an indicative expect message + // explaining why the conversion + // works. + minimal_gas_amount: (minimal_l1_gas_amount + .try_into() + .expect("Failed to convert u128 to u64.")), + })?; + } + // TODO(Aner): Add checks for minimal_l1_data_gas and minimal_l2_gas. + + let GasPricesForFeeType { l1_gas_price, l1_data_gas_price, l2_gas_price } = + block_info.gas_prices.get_gas_prices_by_fee_type(fee_type); + // TODO!(Aner): Add tests for l1_data_gas_price and l2_gas_price. + for (resource, max_gas_price, actual_gas_price) in [ + (L1Gas, l1_gas.max_price_per_unit, l1_gas_price.into()), + (L1DataGas, l1_data_gas.max_price_per_unit, l1_data_gas_price.into()), + (L2Gas, l2_gas.max_price_per_unit, l2_gas_price.into()), + ] { + // TODO(Aner): refactor to return all prices that are too low. + if max_gas_price < actual_gas_price { + return Err(TransactionFeeError::MaxGasPriceTooLow { + resource, + max_gas_price, + actual_gas_price, + })?; + } + } } - } + }, TransactionInfo::Deprecated(context) => { let max_fee = context.max_fee; let min_fee = diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index f348039070..fa2112d279 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -1021,6 +1021,8 @@ fn test_insufficient_resource_bounds( actual_gas_price == actual_strk_l1_gas_price.into() && resource == Resource::L1Gas ); + + // TODO(Aner): Add test for low max L1 data gas price and L2 gas price. } // TODO(Aner, 21/01/24) modify test for 4844. diff --git a/crates/starknet_api/src/transaction.rs b/crates/starknet_api/src/transaction.rs index 500e73414f..fb4f33db44 100644 --- a/crates/starknet_api/src/transaction.rs +++ b/crates/starknet_api/src/transaction.rs @@ -870,6 +870,7 @@ impl From for Felt { Copy, Debug, Deserialize, + Display, EnumIter, Eq, Hash, @@ -877,7 +878,6 @@ impl From for Felt { PartialEq, PartialOrd, Serialize, - Display, )] pub enum Resource { #[serde(rename = "L1_GAS")]