fix: use div_ceil and assert max fee geq actual fee (#1764)

crates/blockifier/src/fee/gas_usage.rs

@@ -12,7 +12,7 @@ use crate::transaction::objects::{
     GasVector, HasRelatedFeeType, ResourcesMapping, TransactionExecutionResult,
     TransactionPreValidationResult,
 };
-use crate::utils::{u128_from_usize, usize_from_u128};
+use crate::utils::{u128_div_ceil, u128_from_usize, usize_from_u128};
 use crate::versioned_constants::VersionedConstants;
 
 #[cfg(test)]
@@ -332,5 +332,5 @@ pub fn compute_discounted_gas_from_gas_vector(
     let fee_type = tx_context.tx_info.fee_type();
     let gas_price = gas_prices.get_gas_price_by_fee_type(&fee_type);
     let data_gas_price = gas_prices.get_data_gas_price_by_fee_type(&fee_type);
-    gas_usage + (blob_gas_usage * u128::from(data_gas_price)) / gas_price
+    gas_usage + u128_div_ceil(blob_gas_usage * u128::from(data_gas_price), gas_price)
 }

crates/blockifier/src/fee/gas_usage_test.rs

@@ -1,15 +1,25 @@
+use std::num::NonZeroU128;
+
 use pretty_assertions::assert_eq;
 use rstest::{fixture, rstest};
 use starknet_api::hash::StarkFelt;
-use starknet_api::transaction::{EventContent, EventData, EventKey};
+use starknet_api::transaction::{EventContent, EventData, EventKey, Fee};
 
 use crate::abi::constants;
+use crate::context::BlockContext;
 use crate::execution::call_info::{CallExecution, CallInfo, OrderedEvent};
 use crate::fee::eth_gas_constants;
-use crate::fee::gas_usage::{get_da_gas_cost, get_message_segment_length, get_tx_events_gas_cost};
+use crate::fee::fee_utils::get_fee_by_gas_vector;
+use crate::fee::gas_usage::{
+    compute_discounted_gas_from_gas_vector, get_da_gas_cost, get_message_segment_length,
+    get_tx_events_gas_cost,
+};
+use crate::invoke_tx_args;
 use crate::state::cached_state::StateChangesCount;
-use crate::transaction::objects::GasVector;
-use crate::utils::u128_from_usize;
+use crate::test_utils::{DEFAULT_ETH_L1_DATA_GAS_PRICE, DEFAULT_ETH_L1_GAS_PRICE};
+use crate::transaction::objects::{FeeType, GasVector};
+use crate::transaction::test_utils::account_invoke_tx;
+use crate::utils::{u128_div_ceil, u128_from_usize};
 use crate::versioned_constants::VersionedConstants;
 #[fixture]
 fn versioned_constants() -> &'static VersionedConstants {
@@ -186,3 +196,26 @@ fn test_get_message_segment_length(
 
     assert_eq!(result, expected_result);
 }
+
+#[rstest]
+fn test_compute_discounted_gas_from_gas_vector() {
+    let tx_context =
+        BlockContext::create_for_testing().to_tx_context(&account_invoke_tx(invoke_tx_args! {}));
+    let gas_usage = GasVector { l1_gas: 100, l1_data_gas: 2 };
+    let actual_result = compute_discounted_gas_from_gas_vector(&gas_usage, &tx_context);
+
+    let result_div_ceil = gas_usage.l1_gas
+        + u128_div_ceil(
+            gas_usage.l1_data_gas * DEFAULT_ETH_L1_DATA_GAS_PRICE,
+            NonZeroU128::new(DEFAULT_ETH_L1_GAS_PRICE).unwrap(),
+        );
+    let result_div_floor = gas_usage.l1_gas
+        + (gas_usage.l1_data_gas * DEFAULT_ETH_L1_DATA_GAS_PRICE) / DEFAULT_ETH_L1_GAS_PRICE;
+
+    assert_eq!(actual_result, result_div_ceil);
+    assert_eq!(actual_result, result_div_floor + 1);
+    assert!(
+        get_fee_by_gas_vector(&tx_context.block_context.block_info, gas_usage, &FeeType::Eth)
+            <= Fee(actual_result * DEFAULT_ETH_L1_GAS_PRICE)
+    );
+}

crates/blockifier/src/transaction/account_transaction.rs

@@ -25,8 +25,9 @@ use crate::transaction::errors::{
     TransactionExecutionError, TransactionFeeError, TransactionPreValidationError,
 };
 use crate::transaction::objects::{
-    HasRelatedFeeType, ResourcesMapping, TransactionExecutionInfo, TransactionExecutionResult,
-    TransactionInfo, TransactionInfoCreator, TransactionPreValidationResult,
+    DeprecatedTransactionInfo, HasRelatedFeeType, ResourcesMapping, TransactionExecutionInfo,
+    TransactionExecutionResult, TransactionInfo, TransactionInfoCreator,
+    TransactionPreValidationResult,
 };
 use crate::transaction::transaction_types::TransactionType;
 use crate::transaction::transaction_utils::update_remaining_gas;
@@ -270,6 +271,36 @@ impl AccountTransaction {
         }
     }
 
+    fn assert_actual_fee_in_bounds(
+        tx_context: &Arc<TransactionContext>,
+        actual_fee: Fee,
+    ) -> TransactionExecutionResult<()> {
+        match &tx_context.tx_info {
+            TransactionInfo::Current(context) => {
+                let ResourceBounds {
+                    max_amount: max_l1_gas_amount,
+                    max_price_per_unit: max_l1_gas_price,
+                } = context.l1_resource_bounds()?;
+                if actual_fee > Fee(u128::from(max_l1_gas_amount) * max_l1_gas_price) {
+                    panic!(
+                        "Actual fee {:#?} exceeded bounds; max amount is {:#?}, max price is 
+                         {:#?}.",
+                        actual_fee, max_l1_gas_amount, max_l1_gas_price
+                    );
+                }
+            }
+            TransactionInfo::Deprecated(DeprecatedTransactionInfo { max_fee, .. }) => {
+                if actual_fee > *max_fee {
+                    panic!(
+                        "Actual fee {:#?} exceeded bounds; max fee is {:#?}.",
+                        actual_fee, max_fee
+                    );
+                }
+            }
+        }
+        Ok(())
+    }
+
     fn handle_fee(
         &self,
         state: &mut dyn State,
@@ -282,6 +313,9 @@ impl AccountTransaction {
             return Ok(None);
         }
 
+        // TODO(Amos, 8/04/2024): Add test for this assert.
+        Self::assert_actual_fee_in_bounds(&tx_context, actual_fee)?;
+
         // Charge fee.
         let fee_transfer_call_info = Self::execute_fee_transfer(state, tx_context, actual_fee)?;
 

crates/blockifier/src/utils.rs

@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::num::NonZeroU128;
 
 use crate::transaction::errors::NumericConversionError;
 
@@ -33,3 +34,12 @@ pub fn usize_from_u128(val: u128) -> Result<usize, NumericConversionError> {
 pub fn u128_from_usize(val: usize) -> Result<u128, NumericConversionError> {
     val.try_into().map_err(|_| NumericConversionError::UsizeToU128Error(val))
 }
+
+/// Returns the ceiling of the division of two u128 numbers.
+pub fn u128_div_ceil(a: u128, b: NonZeroU128) -> u128 {
+    let mut result = a / b;
+    if result * b.get() < a {
+        result += 1;
+    }
+    result
+}

crates/blockifier/src/utils_test.rs

@@ -1,8 +1,9 @@
 use std::collections::HashMap;
+use std::num::NonZeroU128;
 
 use pretty_assertions::assert_eq;
 
-use crate::utils::subtract_mappings;
+use crate::utils::{subtract_mappings, u128_div_ceil};
 
 #[test]
 fn test_subtract_mappings() {
@@ -18,3 +19,12 @@ fn test_subtract_mappings() {
     let expected = HashMap::from([("red", 1), ("blue", 3)]);
     assert_eq!(expected, subtract_mappings(&map1, &map2));
 }
+
+#[test]
+fn test_u128_div_ceil() {
+    assert_eq!(1, u128_div_ceil(1, NonZeroU128::new(1).unwrap()));
+    assert_eq!(0, u128_div_ceil(0, NonZeroU128::new(1).unwrap()));
+    assert_eq!(1, u128_div_ceil(1, NonZeroU128::new(2).unwrap()));
+    assert_eq!(9, u128_div_ceil(27, NonZeroU128::new(3).unwrap()));
+    assert_eq!(10, u128_div_ceil(28, NonZeroU128::new(3).unwrap()));
+}