fix builder gas and da calculation

Author: avalonche

crates/op-rbuilder/src/builders/context.rs

@@ -385,6 +385,7 @@ impl<ExtraCtx: Debug + Default> OpPayloadBuilderCtx<ExtraCtx> {
         best_txs: &mut impl PayloadTxsBounds,
         block_gas_limit: u64,
         block_da_limit: Option<u64>,
+        block_da_footprint_limit: Option<u64>,
     ) -> Result<Option<()>, PayloadBuilderError> {
         let execute_txs_start_time = Instant::now();
         let mut num_txs_considered = 0;
@@ -470,6 +471,7 @@ impl<ExtraCtx: Debug + Default> OpPayloadBuilderCtx<ExtraCtx> {
                 block_da_limit,
                 tx.gas_limit(),
                 info.da_footprint_scalar,
+                block_da_footprint_limit,
             ) {
                 // we can't fit this transaction into the block, so we need to mark it as
                 // invalid which also removes all dependent transaction from

crates/op-rbuilder/src/builders/flashblocks/payload.rs

@@ -85,20 +85,30 @@ pub struct FlashblocksExtraCtx {
     target_gas_for_batch: u64,
     /// Total DA bytes left for the current flashblock
     target_da_for_batch: Option<u64>,
+    /// Total DA footprint left for the current flashblock
+    target_da_footprint_for_batch: Option<u64>,
     /// Gas limit per flashblock
     gas_per_batch: u64,
     /// DA bytes limit per flashblock
     da_per_batch: Option<u64>,
+    /// DA footprint limit per flashblock
+    da_footprint_per_batch: Option<u64>,
     /// Whether to disable state root calculation for each flashblock
     disable_state_root: bool,
 }
 
 impl FlashblocksExtraCtx {
-    fn next(self, target_gas_for_batch: u64, target_da_for_batch: Option<u64>) -> Self {
+    fn next(
+        self,
+        target_gas_for_batch: u64,
+        target_da_for_batch: Option<u64>,
+        target_da_footprint_for_batch: Option<u64>,
+    ) -> Self {
         Self {
             flashblock_index: self.flashblock_index + 1,
             target_gas_for_batch,
             target_da_for_batch,
+            target_da_footprint_for_batch,
             ..self
         }
     }
@@ -344,28 +354,18 @@ where
         ctx.metrics.sequencer_tx_gauge.set(sequencer_tx_time);
 
         // We add first builder tx right after deposits
-        let builder_txs = if ctx.attributes().no_tx_pool {
-            vec![]
-        } else {
-            match self.builder_tx.add_builder_txs(
-                &state_provider,
-                &mut info,
-                &ctx,
-                &mut state,
-                false,
-            ) {
-                Ok(builder_txs) => builder_txs,
-                Err(e) => {
-                    error!(target: "payload_builder", "Error adding builder txs to fallback block: {}", e);
-                    vec![]
-                }
-            }
+        if !ctx.attributes().no_tx_pool
+            && let Err(e) =
+                self.builder_tx
+                    .add_builder_txs(&state_provider, &mut info, &ctx, &mut state, false)
+        {
+            error!(
+                target: "payload_builder",
+                "Error adding builder txs to fallback block: {}",
+                e
+            );
         };
 
-        // We subtract gas limit and da limit for builder transaction from the whole limit
-        let builder_tx_gas = builder_txs.iter().fold(0, |acc, tx| acc + tx.gas_used);
-        let builder_tx_da_size: u64 = builder_txs.iter().fold(0, |acc, tx| acc + tx.da_size);
-
         let (payload, fb_payload) = build_block(
             &mut state,
             &ctx,
@@ -438,34 +438,33 @@ where
             .first_flashblock_time_offset
             .record(first_flashblock_offset.as_millis() as f64);
         let gas_per_batch = ctx.block_gas_limit() / flashblocks_per_block;
-        let target_gas_for_batch = gas_per_batch;
         let da_per_batch = ctx
             .da_config
             .max_da_block_size()
             .map(|da_limit| da_limit / flashblocks_per_block);
         // Check that builder tx won't affect fb limit too much
         if let Some(da_limit) = da_per_batch {
             // We error if we can't insert any tx aside from builder tx in flashblock
-            if da_limit / 2 < builder_tx_da_size {
+            if info.cumulative_da_bytes_used >= da_limit {
                 error!(
                     "Builder tx da size subtraction caused max_da_block_size to be 0. No transaction would be included."
                 );
             }
         }
-        let mut target_da_for_batch = da_per_batch;
+        let da_footprint_per_batch = info
+            .da_footprint_scalar
+            .map(|_| ctx.block_gas_limit() / flashblocks_per_block);
 
-        // Account for already included builder tx
-        if let Some(da_limit) = target_da_for_batch.as_mut() {
-            *da_limit = da_limit.saturating_sub(builder_tx_da_size);
-        }
         let extra_ctx = FlashblocksExtraCtx {
             flashblock_index: 1,
             target_flashblock_count: flashblocks_per_block,
-            target_gas_for_batch: target_gas_for_batch.saturating_sub(builder_tx_gas),
-            target_da_for_batch,
+            target_gas_for_batch: gas_per_batch,
+            target_da_for_batch: da_per_batch,
             gas_per_batch,
             da_per_batch,
+            da_footprint_per_batch,
             disable_state_root,
+            target_da_footprint_for_batch: da_footprint_per_batch,
         };
 
         let mut fb_cancel = block_cancel.child_token();
@@ -613,6 +612,7 @@ where
         let flashblock_index = ctx.flashblock_index();
         let mut target_gas_for_batch = ctx.extra_ctx.target_gas_for_batch;
         let mut target_da_for_batch = ctx.extra_ctx.target_da_for_batch;
+        let mut target_da_footprint_for_batch = ctx.extra_ctx.target_da_footprint_for_batch;
 
         info!(
             target: "payload_builder",
@@ -623,6 +623,7 @@ where
             target_da = target_da_for_batch,
             da_used = info.cumulative_da_bytes_used,
             block_gas_used = ctx.block_gas_limit(),
+            target_da_footprint = target_da_footprint_for_batch,
             "Building flashblock",
         );
         let flashblock_build_start_time = Instant::now();
@@ -639,15 +640,30 @@ where
                 }
             };
 
-        let builder_tx_gas = builder_txs.iter().fold(0, |acc, tx| acc + tx.gas_used);
-        let builder_tx_da_size: u64 = builder_txs.iter().fold(0, |acc, tx| acc + tx.da_size);
+        // only reserve builder tx gas / da size that has not been committed yet
+        // committed builder txs would have counted towards the gas / da used
+        let builder_tx_gas = builder_txs
+            .iter()
+            .filter(|tx| !tx.is_top_of_block)
+            .fold(0, |acc, tx| acc + tx.gas_used);
+        let builder_tx_da_size: u64 = builder_txs
+            .iter()
+            .filter(|tx| !tx.is_top_of_block)
+            .fold(0, |acc, tx| acc + tx.da_size);
         target_gas_for_batch = target_gas_for_batch.saturating_sub(builder_tx_gas);
 
         // saturating sub just in case, we will log an error if da_limit too small for builder_tx_da_size
         if let Some(da_limit) = target_da_for_batch.as_mut() {
             *da_limit = da_limit.saturating_sub(builder_tx_da_size);
         }
 
+        if let (Some(footprint), Some(scalar)) = (
+            target_da_footprint_for_batch.as_mut(),
+            info.da_footprint_scalar,
+        ) {
+            *footprint = footprint.saturating_sub(builder_tx_da_size.saturating_mul(scalar as u64));
+        }
+
         let best_txs_start_time = Instant::now();
         best_txs.refresh_iterator(
             BestPayloadTransactions::new(
@@ -671,6 +687,7 @@ where
             best_txs,
             target_gas_for_batch.min(ctx.block_gas_limit()),
             target_da_for_batch,
+            target_da_footprint_for_batch,
         )
         .wrap_err("failed to execute best transactions")?;
         // Extract last transactions
@@ -779,10 +796,21 @@ where
 
                 let target_gas_for_batch =
                     ctx.extra_ctx.target_gas_for_batch + ctx.extra_ctx.gas_per_batch;
-                let next_extra_ctx = ctx
-                    .extra_ctx
-                    .clone()
-                    .next(target_gas_for_batch, target_da_for_batch);
+
+                if let (Some(footprint), Some(da_footprint_limit)) = (
+                    target_da_footprint_for_batch.as_mut(),
+                    ctx.extra_ctx.da_footprint_per_batch,
+                ) {
+                    *footprint += da_footprint_limit;
+                }
+
+                info!(target_da_for_batch, "next da batch");
+
+                let next_extra_ctx = ctx.extra_ctx.clone().next(
+                    target_gas_for_batch,
+                    target_da_for_batch,
+                    target_da_footprint_for_batch,
+                );
 
                 info!(
                     target: "payload_builder",

crates/op-rbuilder/src/builders/standard/payload.rs

@@ -385,6 +385,14 @@ impl<Txs: PayloadTxsBounds> OpBuilder<'_, Txs> {
                 }
                 da_limit
             });
+        let block_da_footprint = info.da_footprint_scalar
+        .map(|da_footprint_scalar| {
+            let da_footprint_limit = ctx.block_gas_limit().saturating_sub(builder_tx_da_size.saturating_mul(da_footprint_scalar as u64));
+            if da_footprint_limit == 0 {
+                error!("Builder tx da size subtraction caused max_da_footprint to be 0. No transaction would be included.");
+            }
+            da_footprint_limit
+        });
 
         if !ctx.attributes().no_tx_pool {
             let best_txs_start_time = Instant::now();
@@ -404,6 +412,7 @@ impl<Txs: PayloadTxsBounds> OpBuilder<'_, Txs> {
                     &mut best_txs,
                     block_gas_limit,
                     block_da_limit,
+                    block_da_footprint,
                 )?
                 .is_some()
             {

crates/op-rbuilder/src/primitives/reth/execution.rs

@@ -65,6 +65,7 @@ impl<T: Debug + Default> ExecutionInfo<T> {
     ///   per tx.
     /// - block DA limit: if configured, ensures the transaction's DA size does not exceed the
     ///   maximum allowed DA limit per block.
+    #[allow(clippy::too_many_arguments)]
     pub fn is_tx_over_limits(
         &self,
         tx_da_size: u64,
@@ -73,12 +74,20 @@ impl<T: Debug + Default> ExecutionInfo<T> {
         block_data_limit: Option<u64>,
         tx_gas_limit: u64,
         da_footprint_gas_scalar: Option<u16>,
+        block_da_footprint_limit: Option<u64>,
     ) -> Result<(), TxnExecutionResult> {
         if tx_data_limit.is_some_and(|da_limit| tx_da_size > da_limit) {
             return Err(TxnExecutionResult::TransactionDALimitExceeded);
         }
         let total_da_bytes_used = self.cumulative_da_bytes_used.saturating_add(tx_da_size);
-
+        tracing::info!(
+            tx_da_size,
+            tx_data_limit,
+            block_data_limit,
+            block_da_footprint_limit,
+            total_da_bytes_used,
+            "da limits"
+        );
         if block_data_limit.is_some_and(|da_limit| total_da_bytes_used > da_limit) {
             return Err(TxnExecutionResult::BlockDALimitExceeded(
                 self.cumulative_da_bytes_used,
@@ -91,7 +100,7 @@ impl<T: Debug + Default> ExecutionInfo<T> {
         if let Some(da_footprint_gas_scalar) = da_footprint_gas_scalar {
             let tx_da_footprint =
                 total_da_bytes_used.saturating_mul(da_footprint_gas_scalar as u64);
-            if tx_da_footprint > block_gas_limit {
+            if tx_da_footprint > block_da_footprint_limit.unwrap_or(block_gas_limit) {
                 return Err(TxnExecutionResult::BlockDALimitExceeded(
                     total_da_bytes_used,
                     tx_da_size,

crates/op-rbuilder/src/tests/data_availability.rs

@@ -64,7 +64,6 @@ async fn block_fill(rbuilder: LocalInstance) -> eyre::Result<()> {
     let driver = rbuilder.driver().await?;
 
     // Set block big enough so it could fit 3 transactions without tx size limit
-    // Deposit transactions also count towards DA and there is one deposit txn in this block too
     let call = driver
         .provider()
         .raw_request::<(i32, i32), bool>("miner_setMaxDASize".into(), (0, 100 * 4))
@@ -83,31 +82,38 @@ async fn block_fill(rbuilder: LocalInstance) -> eyre::Result<()> {
         .with_max_priority_fee_per_gas(50)
         .send()
         .await?;
-    let unfit_tx_3 = driver.create_transaction().send().await?;
+    let fit_tx_3 = driver
+        .create_transaction()
+        .with_max_priority_fee_per_gas(50)
+        .send()
+        .await?;
+    let unfit_tx_4 = driver.create_transaction().send().await?;
 
     let block = driver.build_new_block_with_current_timestamp(None).await?;
 
     if_standard! {
         // Now the first 2 txs will fit into the block
         assert!(block.includes(fit_tx_1.tx_hash()), "tx should be in block");
         assert!(block.includes(fit_tx_2.tx_hash()), "tx should be in block");
+        assert!(block.includes(fit_tx_3.tx_hash()), "tx should be in block");
     }
 
     if_flashblocks! {
         // in flashblocks the DA quota is divided by the number of flashblocks
         // so we will include only one tx in the block because not all of them
         // will fit within DA quote / flashblocks count.
         assert!(block.includes(fit_tx_1.tx_hash()), "tx should be in block");
-        assert!(!block.includes(fit_tx_2.tx_hash()), "tx should not be in block");
+        assert!(block.includes(fit_tx_2.tx_hash()), "tx should be in block");
+        assert!(!block.includes(fit_tx_3.tx_hash()), "tx should not be in block");
     }
 
     assert!(
-        !block.includes(unfit_tx_3.tx_hash()),
+        !block.includes(unfit_tx_4.tx_hash()),
         "unfit tx should not be in block"
     );
     assert!(
-        driver.latest_full().await?.transactions.len() == 4,
-        "builder + deposit + 2 valid txs should be in the block"
+        driver.latest_full().await?.transactions.len() == 5,
+        "builder + deposit + 3 valid txs should be in the block"
     );
 
     Ok(())