Block producer selects da height to never exceed u64::MAX - 1 transactions from L1

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 - [2135](https://github.com/FuelLabs/fuel-core/pull/2135): Added metrics logging for number of blocks served over the p2p req/res protocol.
 - [2151](https://github.com/FuelLabs/fuel-core/pull/2151): Added limitations on gas used during dry_run in API.
+- [2189](https://github.com/FuelLabs/fuel-core/pull/2151): Select next DA height to never include more than u16::MAX -1 transactions from L1.
 - [2188](https://github.com/FuelLabs/fuel-core/pull/2188): Added the new variant `V2` for the `ConsensusParameters` which contains the new `block_transaction_size_limit` parameter.
 - [2163](https://github.com/FuelLabs/fuel-core/pull/2163): Added runnable task for fetching block committer data.
 

crates/fuel-core/src/service/adapters/producer.rs

@@ -139,13 +139,16 @@ impl fuel_core_producer::ports::Relayer for MaybeRelayerAdapter {
         }
     }
 
-    async fn get_cost_for_block(&self, height: &DaBlockHeight) -> anyhow::Result<u64> {
+    async fn get_cost_and_transactions_number_for_block(
+        &self,
+        height: &DaBlockHeight,
+    ) -> anyhow::Result<(u64, u64)> {
         #[cfg(feature = "relayer")]
         {
             if let Some(sync) = self.relayer_synced.as_ref() {
-                get_gas_cost_for_height(**height, sync)
+                get_gas_cost_and_transactions_number_for_height(**height, sync)
             } else {
-                Ok(0)
+                Ok((0, 0))
             }
         }
         #[cfg(not(feature = "relayer"))]
@@ -155,29 +158,38 @@ impl fuel_core_producer::ports::Relayer for MaybeRelayerAdapter {
                 "Cannot have a da height above zero without a relayer"
             );
             // If the relayer is not enabled, then all blocks are zero.
-            Ok(0)
+            Ok((0, 0))
         }
     }
 }
 
 #[cfg(feature = "relayer")]
-fn get_gas_cost_for_height(
+fn get_gas_cost_and_transactions_number_for_height(
     height: u64,
     sync: &fuel_core_relayer::SharedState<
         crate::database::Database<
             crate::database::database_description::relayer::Relayer,
         >,
     >,
-) -> anyhow::Result<u64> {
+) -> anyhow::Result<(u64, u64)> {
     let da_height = DaBlockHeight(height);
-    let cost = sync
+    let cost_with_transactions = sync
         .database()
         .storage::<fuel_core_relayer::storage::EventsHistory>()
         .get(&da_height)?
         .unwrap_or_default()
         .iter()
-        .fold(0u64, |acc, event| acc.saturating_add(event.cost()));
-    Ok(cost)
+        .fold((0u64, 0u64), |(cost, transactions), event| {
+            let cost = cost.saturating_add(event.cost());
+            let transactions = match event {
+                fuel_core_types::services::relayer::Event::Message(_) => transactions,
+                fuel_core_types::services::relayer::Event::Transaction(_) => {
+                    transactions.saturating_add(1)
+                }
+            };
+            (cost, transactions)
+        });
+    Ok(cost_with_transactions)
 }
 
 impl fuel_core_producer::ports::BlockProducerDatabase for OnChainIterableKeyValueView {

crates/services/executor/src/executor.rs

@@ -974,7 +974,7 @@ where
             return Err(ForcedTransactionFailure::InsufficientMaxGas {
                 claimed_max_gas,
                 actual_max_gas,
-            });
+            })
         }
         Ok(())
     }

crates/services/producer/src/block_producer.rs

@@ -352,8 +352,10 @@ where
             .consensus_parameters_provider
             .consensus_params_at_version(&block_header.consensus_parameters_version)?
             .block_gas_limit();
+        // We have a hard limit of u16::MAX transactions per block, including the final mint transactions.
+        // Therefore we choose the `new_da_height` to never include more than u16::MAX - 1 transactions in a block.
         let new_da_height = self
-            .select_new_da_height(gas_limit, previous_da_height)
+            .select_new_da_height(gas_limit, previous_da_height, u16::MAX - 1)
             .await?;
 
         block_header.application.da_height = new_da_height;
@@ -375,9 +377,12 @@ where
         &self,
         gas_limit: u64,
         previous_da_height: DaBlockHeight,
+        transactions_limit: u16,
     ) -> anyhow::Result<DaBlockHeight> {
         let mut new_best = previous_da_height;
         let mut total_cost: u64 = 0;
+        let transactions_limit: u64 = transactions_limit as u64;
+        let mut total_transactions: u64 = 0;
         let highest = self
             .relayer
             .wait_for_at_least_height(&previous_da_height)
@@ -396,17 +401,19 @@ where
 
         let next_da_height = previous_da_height.saturating_add(1);
         for height in next_da_height..=highest.0 {
-            let cost = self
+            let (cost, transactions_number) = self
                 .relayer
-                .get_cost_for_block(&DaBlockHeight(height))
+                .get_cost_and_transactions_number_for_block(&DaBlockHeight(height))
                 .await?;
             total_cost = total_cost.saturating_add(cost);
-            if total_cost > gas_limit {
+            total_transactions = total_transactions.saturating_add(transactions_number);
+            if total_cost > gas_limit || total_transactions > transactions_limit {
                 break;
-            } else {
-                new_best = DaBlockHeight(height);
             }
+
+            new_best = DaBlockHeight(height);
         }
+
         if new_best == previous_da_height {
             Err(anyhow!(NO_NEW_DA_HEIGHT_FOUND))
         } else {

crates/services/producer/src/block_producer/tests.rs

@@ -390,6 +390,52 @@ mod produce_and_execute_block_txpool {
         assert_eq!(expected, actual);
     }
 
+    #[tokio::test]
+    async fn will_only_advance_da_height_if_enough_transactions_remaining() {
+        // given
+        let prev_da_height = 100;
+        let prev_height = 1u32.into();
+        // 0 + 15_000 + 15_000 + 15_000 + 21_000 = 66_000 > 65_535
+        let latest_blocks_with_transaction_numbers = vec![
+            (prev_da_height, 0u64),
+            (prev_da_height + 1, 15_000),
+            (prev_da_height + 2, 15_000),
+            (prev_da_height + 3, 15_000),
+            (prev_da_height + 4, 21_000),
+        ]
+        .into_iter()
+        .map(|(height, gas_cost)| (DaBlockHeight(height), gas_cost));
+
+        let ctx = TestContextBuilder::new()
+            .with_latest_block_height((prev_da_height + 4u64).into())
+            .with_latest_blocks_with_transactions(latest_blocks_with_transaction_numbers)
+            .with_prev_da_height(prev_da_height.into())
+            .with_prev_height(prev_height)
+            .build();
+
+        let producer = ctx.producer();
+        let next_height = prev_height
+            .succ()
+            .expect("The block height should be valid");
+
+        // when
+        let res = producer
+            .produce_and_execute_block_txpool(next_height, Tai64::now())
+            .await
+            .unwrap();
+
+        // then
+        let expected = prev_da_height + 3;
+        let actual: u64 = res
+            .into_result()
+            .block
+            .header()
+            .application()
+            .da_height
+            .into();
+        assert_eq!(expected, actual);
+    }
+
     #[tokio::test]
     async fn if_each_block_is_full_then_only_advance_one_at_a_time() {
         // given
@@ -800,7 +846,7 @@ impl<Executor> TestContext<Executor> {
 
 struct TestContextBuilder {
     latest_block_height: DaBlockHeight,
-    blocks_with_gas_costs: HashMap<DaBlockHeight, u64>,
+    blocks_with_gas_costs_and_transactions_number: HashMap<DaBlockHeight, (u64, u64)>,
     prev_da_height: DaBlockHeight,
     block_gas_limit: Option<u64>,
     prev_height: BlockHeight,
@@ -810,7 +856,7 @@ impl TestContextBuilder {
     fn new() -> Self {
         Self {
             latest_block_height: 0u64.into(),
-            blocks_with_gas_costs: HashMap::new(),
+            blocks_with_gas_costs_and_transactions_number: HashMap::new(),
             prev_da_height: 1u64.into(),
             block_gas_limit: None,
             prev_height: 0u32.into(),
@@ -822,12 +868,47 @@ impl TestContextBuilder {
         self
     }
 
+    fn with_latest_blocks_with_gas_costs_and_transactions_number(
+        mut self,
+        latest_blocks_with_gas_costs_and_transactions: impl Iterator<
+            Item = (DaBlockHeight, (u64, u64)),
+        >,
+    ) -> Self {
+        self.blocks_with_gas_costs_and_transactions_number
+            .extend(latest_blocks_with_gas_costs_and_transactions);
+        self
+    }
+
+    // Helper function that can be used in tests where transaction numbers in a da block are irrelevant
     fn with_latest_blocks_with_gas_costs(
         mut self,
         latest_blocks_with_gas_costs: impl Iterator<Item = (DaBlockHeight, u64)>,
     ) -> Self {
-        self.blocks_with_gas_costs
-            .extend(latest_blocks_with_gas_costs);
+        let latest_blocks_with_gas_costs_and_transactions_number =
+            latest_blocks_with_gas_costs
+                .into_iter()
+                .map(|(da_block_height, gas_costs)| (da_block_height, (gas_costs, 0)));
+        // Assigning `self` necessary to avoid the compiler complaining about the mutability of `self`
+        self = self.with_latest_blocks_with_gas_costs_and_transactions_number(
+            latest_blocks_with_gas_costs_and_transactions_number,
+        );
+        self
+    }
+
+    // Helper function that can be used in tests where gas costs in a da block are irrelevant
+    fn with_latest_blocks_with_transactions(
+        mut self,
+        latest_blocks_with_transactions: impl Iterator<Item = (DaBlockHeight, u64)>,
+    ) -> Self {
+        let latest_blocks_with_gas_costs_and_transactions_number =
+            latest_blocks_with_transactions.into_iter().map(
+                |(da_block_height, transactions)| (da_block_height, (0, transactions)),
+            );
+
+        // Assigning `self` necessary to avoid the compiler complaining about the mutability of `self`
+        self = self.with_latest_blocks_with_gas_costs_and_transactions_number(
+            latest_blocks_with_gas_costs_and_transactions_number,
+        );
         self
     }
 
@@ -877,7 +958,9 @@ impl TestContextBuilder {
 
         let mock_relayer = MockRelayer {
             latest_block_height: self.latest_block_height,
-            latest_da_blocks_with_costs: self.blocks_with_gas_costs.clone(),
+            latest_da_blocks_with_costs_and_transactions_number: self
+                .blocks_with_gas_costs_and_transactions_number
+                .clone(),
             ..Default::default()
         };
 
@@ -919,7 +1002,9 @@ impl TestContextBuilder {
 
         let mock_relayer = MockRelayer {
             latest_block_height: self.latest_block_height,
-            latest_da_blocks_with_costs: self.blocks_with_gas_costs.clone(),
+            latest_da_blocks_with_costs_and_transactions_number: self
+                .blocks_with_gas_costs_and_transactions_number
+                .clone(),
             ..Default::default()
         };
 

crates/services/producer/src/mocks.rs

@@ -57,7 +57,8 @@ use std::{
 pub struct MockRelayer {
     pub block_production_key: Address,
     pub latest_block_height: DaBlockHeight,
-    pub latest_da_blocks_with_costs: HashMap<DaBlockHeight, u64>,
+    pub latest_da_blocks_with_costs_and_transactions_number:
+        HashMap<DaBlockHeight, (u64, u64)>,
 }
 
 #[async_trait::async_trait]
@@ -70,13 +71,16 @@ impl Relayer for MockRelayer {
         Ok(highest)
     }
 
-    async fn get_cost_for_block(&self, height: &DaBlockHeight) -> anyhow::Result<u64> {
-        let cost = self
-            .latest_da_blocks_with_costs
+    async fn get_cost_and_transactions_number_for_block(
+        &self,
+        height: &DaBlockHeight,
+    ) -> anyhow::Result<(u64, u64)> {
+        let cost_with_transactions_number = self
+            .latest_da_blocks_with_costs_and_transactions_number
             .get(height)
             .cloned()
             .unwrap_or_default();
-        Ok(cost)
+        Ok(cost_with_transactions_number)
     }
 }
 

crates/services/producer/src/ports.rs

@@ -71,7 +71,10 @@ pub trait Relayer: Send + Sync {
     ) -> anyhow::Result<DaBlockHeight>;
 
     /// Get the total Forced Transaction gas cost for the block at the given height.
-    async fn get_cost_for_block(&self, height: &DaBlockHeight) -> anyhow::Result<u64>;
+    async fn get_cost_and_transactions_number_for_block(
+        &self,
+        height: &DaBlockHeight,
+    ) -> anyhow::Result<(u64, u64)>;
 }
 
 pub trait BlockProducer<TxSource>: Send + Sync {