From f2914323b9f0bd51710473862a76dd1aa5197d52 Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Wed, 9 Oct 2024 16:35:11 +0300 Subject: [PATCH] feat(blockifier): panic on GasVector arithmetic overflow --- Cargo.lock | 1 - crates/blockifier/src/fee/fee_test.rs | 12 ++-- crates/blockifier/src/fee/fee_utils.rs | 2 +- crates/blockifier/src/fee/gas_usage.rs | 27 ++++++-- crates/blockifier/src/fee/resources.rs | 66 +++++++++++++++---- crates/starknet_api/Cargo.toml | 1 - .../starknet_api/src/execution_resources.rs | 54 +++++++-------- 7 files changed, 108 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8a82bd6b5..1b3ae0c680 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10113,7 +10113,6 @@ dependencies = [ "hex", "indexmap 2.6.0", "itertools 0.12.1", - "log", "num-bigint 0.4.6", "pretty_assertions", "primitive-types", diff --git a/crates/blockifier/src/fee/fee_test.rs b/crates/blockifier/src/fee/fee_test.rs index 6d0ac55566..82f9be0a3c 100644 --- a/crates/blockifier/src/fee/fee_test.rs +++ b/crates/blockifier/src/fee/fee_test.rs @@ -322,11 +322,13 @@ fn test_get_fee_by_gas_vector_regression( } #[rstest] -#[case::l1_saturates(u64::MAX, 0, 0)] -#[case::l1_data_saturates(0, u64::MAX, 0)] -#[case::l2_gas_saturates(0, 0, u64::MAX)] -// TODO: Add L2 saturation case once `get_fee_by_gas_vector` implements L2 collection. -fn test_get_fee_by_gas_vector_saturation( +#[should_panic(expected = "L1Gas cost overflowed")] +#[case::l1_overflows(u64::MAX, 0, 0)] +#[should_panic(expected = "L1DataGas cost overflowed")] +#[case::l1_data_overflows(0, u64::MAX, 0)] +#[should_panic(expected = "L2Gas cost overflowed")] +#[case::l2_gas_overflows(0, 0, u64::MAX)] +fn test_get_fee_by_gas_vector_overflow( #[case] l1_gas: u64, #[case] l1_data_gas: u64, #[case] l2_gas: u64, diff --git a/crates/blockifier/src/fee/fee_utils.rs b/crates/blockifier/src/fee/fee_utils.rs index 426447bca9..37cb1cb4e6 100644 --- a/crates/blockifier/src/fee/fee_utils.rs +++ b/crates/blockifier/src/fee/fee_utils.rs @@ -80,7 +80,7 @@ pub fn get_fee_by_gas_vector( gas_vector: GasVector, fee_type: &FeeType, ) -> Fee { - gas_vector.saturated_cost(block_info.gas_prices.get_gas_prices_by_fee_type(fee_type)) + gas_vector.cost(block_info.gas_prices.get_gas_prices_by_fee_type(fee_type)) } /// Returns the current fee balance and a boolean indicating whether the balance covers the fee. diff --git a/crates/blockifier/src/fee/gas_usage.rs b/crates/blockifier/src/fee/gas_usage.rs index 1c3fb4de88..bf5cfa5e8a 100644 --- a/crates/blockifier/src/fee/gas_usage.rs +++ b/crates/blockifier/src/fee/gas_usage.rs @@ -131,7 +131,14 @@ pub fn get_log_message_to_l1_emissions_cost(l2_to_l1_payload_lengths: &[usize]) constants::LOG_MSG_TO_L1_ENCODED_DATA_SIZE + *length, ) }) - .sum() + .fold(GasVector::ZERO, |accumulator, cost| { + accumulator.checked_add(cost).unwrap_or_else(|| { + panic!( + "Overflow in message emission gas costs; attempted to add {accumulator:?} to \ + {cost:?}" + ) + }) + }) } fn get_event_emission_cost(n_topics: usize, data_length: usize) -> GasVector { @@ -184,11 +191,17 @@ pub fn estimate_minimal_gas_vector( + versioned_constants.os_kzg_da_resources(data_segment_length).n_steps; let resources = ExecutionResources { n_steps: os_steps_for_type, ..Default::default() }; - get_da_gas_cost(&state_changes_by_account_tx, block_info.use_kzg_da) - + get_vm_resources_cost( - versioned_constants, - &resources, - 0, - gas_usage_vector_computation_mode, + let da_gas_cost = get_da_gas_cost(&state_changes_by_account_tx, block_info.use_kzg_da); + let vm_resources_cost = get_vm_resources_cost( + versioned_constants, + &resources, + 0, + gas_usage_vector_computation_mode, + ); + da_gas_cost.checked_add(vm_resources_cost).unwrap_or_else(|| { + panic!( + "Overflow in minimal gas estimation; attempted to add {da_gas_cost:?} to \ + {vm_resources_cost:?}" ) + }) } diff --git a/crates/blockifier/src/fee/resources.rs b/crates/blockifier/src/fee/resources.rs index 2cb3cb1290..3d8b2c3498 100644 --- a/crates/blockifier/src/fee/resources.rs +++ b/crates/blockifier/src/fee/resources.rs @@ -36,8 +36,18 @@ impl TransactionResources { use_kzg_da: bool, computation_mode: &GasVectorComputationMode, ) -> GasVector { - self.starknet_resources.to_gas_vector(versioned_constants, use_kzg_da, computation_mode) - + self.computation.to_gas_vector(versioned_constants, computation_mode) + let starknet_gas = self.starknet_resources.to_gas_vector( + versioned_constants, + use_kzg_da, + computation_mode, + ); + let computation_gas = self.computation.to_gas_vector(versioned_constants, computation_mode); + starknet_gas.checked_add(computation_gas).unwrap_or_else(|| { + panic!( + "Transaction resources to gas vector overflowed: starknet gas cost is \ + {starknet_gas:?}, computation gas is {computation_gas:?}", + ) + }) } } @@ -112,9 +122,20 @@ impl StarknetResources { use_kzg_da: bool, mode: &GasVectorComputationMode, ) -> GasVector { - self.archival_data.to_gas_vector(versioned_constants, mode) - + self.state.to_gas_vector(use_kzg_da) - + self.messages.to_gas_vector() + [ + self.archival_data.to_gas_vector(versioned_constants, mode), + self.state.to_gas_vector(use_kzg_da), + self.messages.to_gas_vector(), + ] + .iter() + .fold(GasVector::ZERO, |accumulator, cost| { + accumulator.checked_add(*cost).unwrap_or_else(|| { + panic!( + "Starknet resources to gas vector overflowed: tried to add {accumulator:?} to \ + {cost:?}", + ) + }) + }) } } @@ -248,7 +269,12 @@ impl MessageResources { .into(), ); - starknet_gas_usage + sharp_gas_usage + starknet_gas_usage.checked_add(sharp_gas_usage).unwrap_or_else(|| { + panic!( + "Message resources to gas vector overflowed: starknet gas cost is \ + {starknet_gas_usage:?}, SHARP gas cost is {sharp_gas_usage:?}.", + ) + }) } /// Returns an estimation of the gas usage for processing L1<>L2 messages on L1. Accounts for @@ -257,10 +283,11 @@ impl MessageResources { let n_l2_to_l1_messages = self.l2_to_l1_payload_lengths.len(); let n_l1_to_l2_messages = usize::from(self.l1_handler_payload_size.is_some()); - GasVector::from_l1_gas( - // Starknet's updateState gets the message segment as an argument. - u64_from_usize( - self.message_segment_length * eth_gas_constants::GAS_PER_MEMORY_WORD + [ + GasVector::from_l1_gas( + // Starknet's updateState gets the message segment as an argument. + u64_from_usize( + self.message_segment_length * eth_gas_constants::GAS_PER_MEMORY_WORD // Starknet's updateState increases a (storage) counter for each L2-to-L1 message. + n_l2_to_l1_messages * eth_gas_constants::GAS_PER_ZERO_TO_NONZERO_STORAGE_SET // Starknet's updateState decreases a (storage) counter for each L1-to-L2 consumed @@ -268,9 +295,20 @@ impl MessageResources { // message but we ignore it since refunded gas cannot be used for the current // transaction execution). + n_l1_to_l2_messages * eth_gas_constants::GAS_PER_COUNTER_DECREASE, - ) - .into(), - ) + get_consumed_message_to_l2_emissions_cost(self.l1_handler_payload_size) - + get_log_message_to_l1_emissions_cost(&self.l2_to_l1_payload_lengths) + ) + .into(), + ), + get_consumed_message_to_l2_emissions_cost(self.l1_handler_payload_size), + get_log_message_to_l1_emissions_cost(&self.l2_to_l1_payload_lengths), + ] + .iter() + .fold(GasVector::ZERO, |accumulator, cost| { + accumulator.checked_add(*cost).unwrap_or_else(|| { + panic!( + "Message resources to starknet gas cost overflowed: tried to add \ + {accumulator:?} to {cost:?}" + ) + }) + }) } } diff --git a/crates/starknet_api/Cargo.toml b/crates/starknet_api/Cargo.toml index e6fa5200bb..4672191a41 100644 --- a/crates/starknet_api/Cargo.toml +++ b/crates/starknet_api/Cargo.toml @@ -16,7 +16,6 @@ derive_more.workspace = true hex.workspace = true indexmap = { workspace = true, features = ["serde"] } itertools.workspace = true -log.workspace = true num-bigint.workspace = true pretty_assertions.workspace = true primitive-types = { workspace = true, features = ["serde"] } diff --git a/crates/starknet_api/src/execution_resources.rs b/crates/starknet_api/src/execution_resources.rs index 0e71baaf92..3e2a53df10 100644 --- a/crates/starknet_api/src/execution_resources.rs +++ b/crates/starknet_api/src/execution_resources.rs @@ -74,19 +74,11 @@ impl GasAmount { } } -#[derive( - derive_more::Add, - derive_more::Sum, - derive_more::AddAssign, - Clone, - Copy, - Debug, - Default, - Eq, - PartialEq, - Deserialize, - Serialize, +#[cfg_attr( + any(test, feature = "testing"), + derive(derive_more::Add, derive_more::Sum, derive_more::AddAssign) )] +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Deserialize, Serialize)] pub struct GasVector { pub l1_gas: GasAmount, pub l1_data_gas: GasAmount, @@ -95,6 +87,9 @@ pub struct GasVector { } impl GasVector { + pub const ZERO: GasVector = + GasVector { l1_gas: GasAmount(0), l1_data_gas: GasAmount(0), l2_gas: GasAmount(0) }; + pub fn from_l1_gas(l1_gas: GasAmount) -> Self { Self { l1_gas, ..Default::default() } } @@ -107,8 +102,21 @@ impl GasVector { Self { l2_gas, ..Default::default() } } - /// Computes the cost (in fee token units) of the gas vector (saturating on overflow). - pub fn saturated_cost(&self, gas_prices: &GasPriceVector) -> Fee { + pub fn checked_add(self, rhs: Self) -> Option { + match ( + self.l1_gas.checked_add(rhs.l1_gas), + self.l1_data_gas.checked_add(rhs.l1_data_gas), + self.l2_gas.checked_add(rhs.l2_gas), + ) { + (Some(l1_gas), Some(l1_data_gas), Some(l2_gas)) => { + Some(Self { l1_gas, l1_data_gas, l2_gas }) + } + _ => None, + } + } + + /// Computes the cost (in fee token units) of the gas vector (panicking on overflow). + pub fn cost(&self, gas_prices: &GasPriceVector) -> Fee { let mut sum = Fee(0); for (gas, price, resource) in [ (self.l1_gas, gas_prices.l1_gas_price, Resource::L1Gas), @@ -116,24 +124,18 @@ impl GasVector { (self.l2_gas, gas_prices.l2_gas_price, Resource::L2Gas), ] { let cost = gas.checked_mul(price.get()).unwrap_or_else(|| { - log::warn!( + panic!( "{} cost overflowed: multiplication of gas amount ({}) by price per unit ({}) \ resulted in overflow.", - resource, - gas, - price - ); - Fee(u128::MAX) + resource, gas, price + ) }); sum = sum.checked_add(cost).unwrap_or_else(|| { - log::warn!( + panic!( "Total cost overflowed: addition of current sum ({}) and cost of {} ({}) \ resulted in overflow.", - sum, - resource, - cost - ); - Fee(u128::MAX) + sum, resource, cost + ) }); } sum