diff --git a/crates/op-rbuilder/src/builders/builder_tx.rs b/crates/op-rbuilder/src/builders/builder_tx.rs index d8d99adb..f99bd79b 100644 --- a/crates/op-rbuilder/src/builders/builder_tx.rs +++ b/crates/op-rbuilder/src/builders/builder_tx.rs @@ -235,6 +235,7 @@ pub trait BuilderTransactions OpPayloadBuilderCtx { best_txs: &mut impl PayloadTxsBounds, block_gas_limit: u64, block_da_limit: Option, + block_da_footprint_limit: Option, ) -> Result, PayloadBuilderError> { let execute_txs_start_time = Instant::now(); let mut num_txs_considered = 0; @@ -470,6 +471,7 @@ impl OpPayloadBuilderCtx { 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 diff --git a/crates/op-rbuilder/src/builders/flashblocks/payload.rs b/crates/op-rbuilder/src/builders/flashblocks/payload.rs index 02753bf5..b314b9e6 100644 --- a/crates/op-rbuilder/src/builders/flashblocks/payload.rs +++ b/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, + /// Total DA footprint left for the current flashblock + target_da_footprint_for_batch: Option, /// Gas limit per flashblock gas_per_batch: u64, /// DA bytes limit per flashblock da_per_batch: Option, + /// DA footprint limit per flashblock + da_footprint_per_batch: Option, /// 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) -> Self { + fn next( + self, + target_gas_for_batch: u64, + target_da_for_batch: Option, + target_da_footprint_for_batch: Option, + ) -> Self { Self { flashblock_index: self.flashblock_index + 1, target_gas_for_batch, target_da_for_batch, + target_da_footprint_for_batch, ..self } } @@ -298,7 +308,10 @@ where // We log only every 100th block to reduce usage let span = if cfg!(feature = "telemetry") - && config.parent_header.number % self.config.sampling_ratio == 0 + && config + .parent_header + .number + .is_multiple_of(self.config.sampling_ratio) { span!(Level::INFO, "build_payload") } else { @@ -341,29 +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); - info.cumulative_da_bytes_used += builder_tx_da_size; - let (payload, fb_payload) = build_block( &mut state, &ctx, @@ -436,7 +438,6 @@ 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() @@ -444,26 +445,26 @@ where // 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(); @@ -611,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", @@ -621,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(); @@ -637,9 +640,16 @@ 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); - info.cumulative_da_bytes_used += builder_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 @@ -647,6 +657,13 @@ where *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( @@ -670,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 @@ -778,10 +796,19 @@ 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; + } + + 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", diff --git a/crates/op-rbuilder/src/builders/standard/payload.rs b/crates/op-rbuilder/src/builders/standard/payload.rs index 4907c5c2..84bb84d0 100644 --- a/crates/op-rbuilder/src/builders/standard/payload.rs +++ b/crates/op-rbuilder/src/builders/standard/payload.rs @@ -375,7 +375,6 @@ impl OpBuilder<'_, Txs> { } // Save some space in the block_da_limit for builder tx let builder_tx_da_size = builder_txs.iter().fold(0, |acc, tx| acc + tx.da_size); - info.cumulative_da_bytes_used += builder_tx_da_size; let block_da_limit = ctx .da_config .max_da_block_size() @@ -386,6 +385,14 @@ impl 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(); @@ -405,6 +412,7 @@ impl OpBuilder<'_, Txs> { &mut best_txs, block_gas_limit, block_da_limit, + block_da_footprint, )? .is_some() { diff --git a/crates/op-rbuilder/src/primitives/reth/execution.rs b/crates/op-rbuilder/src/primitives/reth/execution.rs index 9ce998d1..7865a1c8 100644 --- a/crates/op-rbuilder/src/primitives/reth/execution.rs +++ b/crates/op-rbuilder/src/primitives/reth/execution.rs @@ -65,6 +65,7 @@ impl ExecutionInfo { /// 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,12 @@ impl ExecutionInfo { block_data_limit: Option, tx_gas_limit: u64, da_footprint_gas_scalar: Option, + block_da_footprint_limit: Option, ) -> 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); - 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 +92,7 @@ impl ExecutionInfo { 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, diff --git a/crates/op-rbuilder/src/tests/data_availability.rs b/crates/op-rbuilder/src/tests/data_availability.rs index 0e856c9f..9621f398 100644 --- a/crates/op-rbuilder/src/tests/data_availability.rs +++ b/crates/op-rbuilder/src/tests/data_availability.rs @@ -1,4 +1,4 @@ -use crate::tests::{BlockTransactionsExt, ChainDriverExt, LocalInstance}; +use crate::tests::{BlockTransactionsExt, ChainDriverExt, LocalInstance, TransactionBuilderExt}; use alloy_provider::Provider; use macros::{if_flashblocks, if_standard, rb_test}; @@ -56,7 +56,7 @@ async fn block_size_limit(rbuilder: LocalInstance) -> eyre::Result<()> { } /// This test ensures that block will fill up to the limit. -/// Size of each transaction is 100000000 +/// Size of each transaction is 100 /// We will set limit to 3 txs and see that the builder will include 3 transactions. /// We should not forget about builder transaction so we will spawn only 2 regular txs. #[rb_test] @@ -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,7 +82,12 @@ 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?; @@ -91,6 +95,7 @@ async fn block_fill(rbuilder: LocalInstance) -> eyre::Result<()> { // 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! { @@ -98,17 +103,116 @@ async fn block_fill(rbuilder: LocalInstance) -> eyre::Result<()> { // 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(()) } + +/// This test ensures that the DA footprint limit (Jovian) is respected and the block fills +/// to the DA footprint limit. The DA footprint is calculated as: +/// total_da_bytes_used * da_footprint_gas_scalar (stored in blob_gas_used). +/// This must not exceed the block gas limit, accounting for the builder transaction. +#[rb_test] +async fn da_footprint_fills_to_limit(rbuilder: LocalInstance) -> eyre::Result<()> { + let driver = rbuilder.driver().await?; + + // DA footprint scalar from JOVIAN_DATA is 400 + // Set a constrained gas limit so DA footprint becomes the limiting factor + // With gas limit = 200_000 and scalar = 400: + // Max DA bytes = 200_000 / 400 = 500 bytes + let gas_limit = 400_000u64; + let call = driver + .provider() + .raw_request::<(u64,), bool>("miner_setGasLimit".into(), (gas_limit,)) + .await?; + assert!(call, "miner_setGasLimit should be executed successfully"); + + // Set DA size limit to be permissive (not the constraint) + let call = driver + .provider() + .raw_request::<(i32, i32), bool>("miner_setMaxDASize".into(), (0, 100_000)) + .await?; + assert!(call, "miner_setMaxDASize should be executed successfully"); + + let mut tx_hashes = Vec::new(); + for _ in 0..10 { + // 400 * 100 = 400_000 total da footprint + let tx = driver + .create_transaction() + .random_valid_transfer() + .with_gas_limit(21000) + .send() + .await?; + tx_hashes.push(tx.tx_hash().clone()); + } + + let block = driver.build_new_block_with_current_timestamp(None).await?; + + // Verify that blob_gas_used (DA footprint) is set and respects limits + assert!( + block.header.blob_gas_used.is_some(), + "blob_gas_used should be set in Jovian" + ); + + let blob_gas = block.header.blob_gas_used.unwrap(); + + // The DA footprint must not exceed the block gas limit + assert!( + blob_gas == gas_limit, + "DA footprint (blob_gas_used={}) must not exceed block gas limit ({})", + blob_gas, + gas_limit + ); + + // Verify the block fills up to the DA footprint limit + // accounting for builder tx DA contribution + if_standard! { + for i in 0..8 { + assert!( + block.includes(&tx_hashes[i]), + "tx {} should be included in the block", + i + ); + } + + // Verify the last tx doesn't fit due to DA footprint limit + assert!( + !block.includes(&tx_hashes[9]), + "tx 9 should not fit in the block due to DA footprint limit" + ); + } + + if_flashblocks! { + for i in 0..7 { + assert!( + block.includes(&tx_hashes[i]), + "tx {} should be included in the block", + i + ); + } + + // Verify the last 2 tx doesn't fit due to DA footprint limit + assert!( + !block.includes(&tx_hashes[8]), + "tx 8 should not fit in the block due to DA footprint limit" + ); + + assert!( + !block.includes(&tx_hashes[9]), + "tx 9 should not fit in the block due to DA footprint limit" + ); + } + + Ok(()) +}