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

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 - [2131](https://github.com/FuelLabs/fuel-core/pull/2131): Add flow in TxPool in order to ask to newly connected peers to share their transaction pool
 - [2182](https://github.com/FuelLabs/fuel-core/pull/2151): Limit number of transactions that can be fetched via TxSource::next
+- [2189](https://github.com/FuelLabs/fuel-core/pull/2151): Select next DA height to never include more than u16::MAX -1 transactions from L1.
+
 
 ### Changed
 

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

@@ -18,7 +18,10 @@ use fuel_core_producer::{
         ConsensusParametersProvider as ConsensusParametersProviderTrait,
         GasPriceProvider,
     },
-    ports::TxPool,
+    ports::{
+        RelayerBlockInfo,
+        TxPool,
+    },
 };
 use fuel_core_storage::{
     iter::{
@@ -139,13 +142,19 @@ 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<RelayerBlockInfo> {
         #[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(RelayerBlockInfo {
+                    gas_cost: 0,
+                    tx_count: 0,
+                })
             }
         }
         #[cfg(not(feature = "relayer"))]
@@ -155,29 +164,41 @@ 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(RelayerBlockInfo {
+                gas_cost: 0,
+                tx_count: 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<RelayerBlockInfo> {
     let da_height = DaBlockHeight(height);
-    let cost = sync
+    let (gas_cost, tx_count) = 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), |(gas_cost, tx_count), event| {
+            let gas_cost = gas_cost.saturating_add(event.cost());
+            let tx_count = match event {
+                fuel_core_types::services::relayer::Event::Message(_) => tx_count,
+                fuel_core_types::services::relayer::Event::Transaction(_) => {
+                    tx_count.saturating_add(1)
+                }
+            };
+            (gas_cost, tx_count)
+        });
+    Ok(RelayerBlockInfo { gas_cost, tx_count })
 }
 
 impl fuel_core_producer::ports::BlockProducerDatabase for OnChainIterableKeyValueView {

crates/services/producer/src/block_producer.rs

@@ -3,8 +3,11 @@ use crate::{
         ConsensusParametersProvider,
         GasPriceProvider as GasPriceProviderConstraint,
     },
-    ports,
-    ports::BlockProducerDatabase,
+    ports::{
+        self,
+        BlockProducerDatabase,
+        RelayerBlockInfo,
+    },
     Config,
 };
 use anyhow::{
@@ -361,8 +364,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;
@@ -384,9 +389,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)
@@ -405,17 +413,19 @@ where
 
         let next_da_height = previous_da_height.saturating_add(1);
         for height in next_da_height..=highest.0 {
-            let cost = self
+            let RelayerBlockInfo { gas_cost, tx_count } = 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_cost = total_cost.saturating_add(gas_cost);
+            total_transactions = total_transactions.saturating_add(tx_count);
+            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

@@ -285,7 +285,7 @@ mod produce_and_execute_block_txpool {
         let da_height = DaBlockHeight(100u64);
         let prev_height = 1u32.into();
         let ctx = TestContextBuilder::new()
-            .with_latest_block_height(da_height)
+            .with_latest_da_block_height_from_relayer(da_height)
             .with_prev_da_height(da_height)
             .with_prev_height(prev_height)
             .build();
@@ -311,7 +311,7 @@ mod produce_and_execute_block_txpool {
         let prev_da_height = DaBlockHeight(100u64);
         let prev_height = 1u32.into();
         let ctx = TestContextBuilder::new()
-            .with_latest_block_height(prev_da_height - 1u64.into())
+            .with_latest_da_block_height_from_relayer(prev_da_height - 1u64.into())
             .with_prev_da_height(prev_da_height)
             .with_prev_height(prev_height)
             .build();
@@ -360,7 +360,7 @@ mod produce_and_execute_block_txpool {
         .map(|(height, gas_cost)| (DaBlockHeight(height), gas_cost));
 
         let ctx = TestContextBuilder::new()
-            .with_latest_block_height((prev_da_height + 4u64).into())
+            .with_latest_da_block_height_from_relayer((prev_da_height + 4u64).into())
             .with_latest_blocks_with_gas_costs(latest_blocks_with_gas_costs)
             .with_prev_da_height(prev_da_height.into())
             .with_block_gas_limit(block_gas_limit)
@@ -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_da_block_height_from_relayer((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
@@ -407,7 +453,7 @@ mod produce_and_execute_block_txpool {
         .map(|(height, gas_cost)| (DaBlockHeight(height), gas_cost));
 
         let ctx = TestContextBuilder::new()
-            .with_latest_block_height((prev_da_height + 4u64).into())
+            .with_latest_da_block_height_from_relayer((prev_da_height + 4u64).into())
             .with_latest_blocks_with_gas_costs(latest_blocks_with_gas_costs)
             .with_prev_da_height(prev_da_height.into())
             .with_block_gas_limit(block_gas_limit)
@@ -455,7 +501,7 @@ mod produce_and_execute_block_txpool {
                 .map(|(height, gas_cost)| (DaBlockHeight(height), gas_cost));
 
         let ctx = TestContextBuilder::new()
-            .with_latest_block_height((prev_da_height + 1u64).into())
+            .with_latest_da_block_height_from_relayer((prev_da_height + 1u64).into())
             .with_latest_blocks_with_gas_costs(latest_blocks_with_gas_costs)
             .with_prev_da_height(prev_da_height.into())
             .with_block_gas_limit(block_gas_limit)
@@ -898,7 +944,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,
@@ -909,25 +955,63 @@ 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(),
             prev_time: Tai64::UNIX_EPOCH,
         }
     }
 
-    fn with_latest_block_height(mut self, latest_block_height: DaBlockHeight) -> Self {
+    fn with_latest_da_block_height_from_relayer(
+        mut self,
+        latest_block_height: DaBlockHeight,
+    ) -> Self {
         self.latest_block_height = latest_block_height;
         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
     }
 
@@ -982,7 +1066,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()
         };
 
@@ -1004,7 +1090,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

@@ -3,6 +3,7 @@ use crate::ports::{
     BlockProducerDatabase,
     DryRunner,
     Relayer,
+    RelayerBlockInfo,
     TxPool,
 };
 use fuel_core_storage::{
@@ -59,7 +60,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]
@@ -72,13 +74,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<RelayerBlockInfo> {
+        let (gas_cost, tx_count) = self
+            .latest_da_blocks_with_costs_and_transactions_number
             .get(height)
             .cloned()
             .unwrap_or_default();
-        Ok(cost)
+        Ok(RelayerBlockInfo { gas_cost, tx_count })
     }
 }