From 2aae70ec341cf5847f904308760937418c4423a8 Mon Sep 17 00:00:00 2001 From: Andrea Cerone Date: Fri, 20 Sep 2024 18:18:20 +0100 Subject: [PATCH] Block producer selects da height to never exceed u64::MAX - 1 transactions from L1 (#2189) ## Linked Issues/PRs Part of #2114 ## Description When producing a block, we want to make sure that we never include more than 65_536 transactions. To this end, we select the DA height for including Forced transactions so that no more than 65_535 transactions will ever be included. Changes: - A new function for the RelayerPort to get the number of forced transactions at a given level - Refactor the function `select_new_da_height` to return early if more than 65_535 transactions will be included. - Test the changes to the `select_new_da_height` function. - ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Green Baneling --- CHANGELOG.md | 2 + .../src/service/adapters/producer.rs | 41 +++++-- .../services/producer/src/block_producer.rs | 28 +++-- .../producer/src/block_producer/tests.rs | 112 ++++++++++++++++-- crates/services/producer/src/mocks.rs | 15 ++- crates/services/producer/src/ports.rs | 10 +- 6 files changed, 171 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a784749090..4dcd90f58b8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crates/fuel-core/src/service/adapters/producer.rs b/crates/fuel-core/src/service/adapters/producer.rs index 0449de9e841..d7ac7590106 100644 --- a/crates/fuel-core/src/service/adapters/producer.rs +++ b/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 { + async fn get_cost_and_transactions_number_for_block( + &self, + height: &DaBlockHeight, + ) -> anyhow::Result { #[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 { +) -> anyhow::Result { let da_height = DaBlockHeight(height); - let cost = sync + let (gas_cost, tx_count) = sync .database() .storage::() .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 { diff --git a/crates/services/producer/src/block_producer.rs b/crates/services/producer/src/block_producer.rs index c6da9767bd3..253f845d422 100644 --- a/crates/services/producer/src/block_producer.rs +++ b/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 { 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 { diff --git a/crates/services/producer/src/block_producer/tests.rs b/crates/services/producer/src/block_producer/tests.rs index b0ab390d37b..8a532361ede 100644 --- a/crates/services/producer/src/block_producer/tests.rs +++ b/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 TestContext { struct TestContextBuilder { latest_block_height: DaBlockHeight, - blocks_with_gas_costs: HashMap, + blocks_with_gas_costs_and_transactions_number: HashMap, prev_da_height: DaBlockHeight, block_gas_limit: Option, prev_height: BlockHeight, @@ -909,7 +955,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(), @@ -917,17 +963,55 @@ impl TestContextBuilder { } } - 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, ) -> 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, + ) -> 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() }; diff --git a/crates/services/producer/src/mocks.rs b/crates/services/producer/src/mocks.rs index 912796185b0..b2c21f0971e 100644 --- a/crates/services/producer/src/mocks.rs +++ b/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, + pub latest_da_blocks_with_costs_and_transactions_number: + HashMap, } #[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 { - let cost = self - .latest_da_blocks_with_costs + async fn get_cost_and_transactions_number_for_block( + &self, + height: &DaBlockHeight, + ) -> anyhow::Result { + 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 }) } } diff --git a/crates/services/producer/src/ports.rs b/crates/services/producer/src/ports.rs index a50b2ebe33c..af11d1648e9 100644 --- a/crates/services/producer/src/ports.rs +++ b/crates/services/producer/src/ports.rs @@ -62,6 +62,11 @@ pub trait TxPool: Send + Sync { ) -> Self::TxSource; } +pub struct RelayerBlockInfo { + pub gas_cost: u64, + pub tx_count: u64, +} + #[async_trait::async_trait] pub trait Relayer: Send + Sync { /// Wait for the relayer to reach at least this height and return the latest height. @@ -71,7 +76,10 @@ pub trait Relayer: Send + Sync { ) -> anyhow::Result; /// 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; + async fn get_cost_and_transactions_number_for_block( + &self, + height: &DaBlockHeight, + ) -> anyhow::Result; } pub trait BlockProducer: Send + Sync {