From ac17f85135074e1a26d6eaa250378727ecdb5110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Fri, 6 Sep 2024 18:36:16 +0200 Subject: [PATCH 01/11] Add `block_transaction_size_limit` to `ConsensusParameters` --- fuel-tx/src/tests/valid_cases.rs | 1 + fuel-tx/src/transaction/consensus_parameters.rs | 13 +++++++++++++ fuel-vm/src/checked_transaction.rs | 1 + fuel-vm/src/tests/outputs.rs | 1 + fuel-vm/src/util.rs | 4 ++++ 5 files changed, 20 insertions(+) diff --git a/fuel-tx/src/tests/valid_cases.rs b/fuel-tx/src/tests/valid_cases.rs index cf3e1e79ce..4c4fd3d5de 100644 --- a/fuel-tx/src/tests/valid_cases.rs +++ b/fuel-tx/src/tests/valid_cases.rs @@ -40,6 +40,7 @@ pub fn test_params() -> ConsensusParameters { Default::default(), Default::default(), Default::default(), + Default::default(), ) } diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 391d0ee575..6decf78c4f 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -60,6 +60,7 @@ impl ConsensusParameters { gas_costs: GasCosts, base_asset_id: AssetId, block_gas_limit: u64, + block_transaction_size_limit: u32, privileged_address: Address, ) -> Self { Self::V1(ConsensusParametersV1 { @@ -72,6 +73,7 @@ impl ConsensusParameters { gas_costs, base_asset_id, block_gas_limit, + block_transaction_size_limit, privileged_address, }) } @@ -139,6 +141,13 @@ impl ConsensusParameters { } } + /// Get the block transaction size limit + pub const fn block_transaction_size_limit(&self) -> u32 { + match self { + Self::V1(params) => params.block_transaction_size_limit, + } + } + /// Get the privileged address pub const fn privileged_address(&self) -> &Address { match self { @@ -231,6 +240,7 @@ pub struct ConsensusParametersV1 { pub gas_costs: GasCosts, pub base_asset_id: AssetId, pub block_gas_limit: u64, + pub block_transaction_size_limit: u32, /// The privileged address(user or predicate) that can perform permissioned /// operations(like upgrading the network). pub privileged_address: Address, @@ -255,6 +265,9 @@ impl ConsensusParametersV1 { gas_costs: GasCosts::default(), base_asset_id: Default::default(), block_gas_limit: TxParameters::DEFAULT.max_gas_per_tx(), + // TODO[RC]: Should not be u64::default(), but some other "default" value, + // like 126kb? + block_transaction_size_limit: Default::default(), privileged_address: Default::default(), } } diff --git a/fuel-vm/src/checked_transaction.rs b/fuel-vm/src/checked_transaction.rs index 36c33dc0c1..a77975b746 100644 --- a/fuel-vm/src/checked_transaction.rs +++ b/fuel-vm/src/checked_transaction.rs @@ -920,6 +920,7 @@ mod tests { Default::default(), Default::default(), Default::default(), + Default::default(), ) } diff --git a/fuel-vm/src/tests/outputs.rs b/fuel-vm/src/tests/outputs.rs index e1c40aa372..498bc500e5 100644 --- a/fuel-vm/src/tests/outputs.rs +++ b/fuel-vm/src/tests/outputs.rs @@ -166,6 +166,7 @@ fn correct_change_is_provided_for_coin_outputs_create() { context.get_gas_costs().to_owned(), *context.get_base_asset_id(), context.get_block_gas_limit(), + context.get_block_transaction_size_limit(), *context.get_privileged_address(), ); let create = create diff --git a/fuel-vm/src/util.rs b/fuel-vm/src/util.rs index 841f1c42e2..bbb7fc4d22 100644 --- a/fuel-vm/src/util.rs +++ b/fuel-vm/src/util.rs @@ -380,6 +380,10 @@ pub mod test_helpers { self.consensus_params.block_gas_limit() } + pub fn get_block_transaction_size_limit(&self) -> u32 { + self.consensus_params.block_transaction_size_limit() + } + pub fn get_privileged_address(&self) -> &Address { self.consensus_params.privileged_address() } From 6c29921df479bf58d259b5b62415c0714c924447 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Tue, 10 Sep 2024 13:17:05 +0200 Subject: [PATCH 02/11] Satisfy clippy --- fuel-vm/src/interpreter/diff/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuel-vm/src/interpreter/diff/tests.rs b/fuel-vm/src/interpreter/diff/tests.rs index 003bb7ffc4..599fd9c9cd 100644 --- a/fuel-vm/src/interpreter/diff/tests.rs +++ b/fuel-vm/src/interpreter/diff/tests.rs @@ -49,7 +49,7 @@ use crate::interpreter::InterpreterParams; fn record_and_invert_storage() { let arb_gas_price = 1; let interpreter_params = - InterpreterParams::new(arb_gas_price, &ConsensusParameters::standard()); + InterpreterParams::new(arb_gas_price, ConsensusParameters::standard()); let a = Interpreter::<_, _, Script>::with_storage( crate::interpreter::MemoryInstance::new(), From 1fa9ee4276ebc7c384b99327806b366d02c4afbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Wed, 11 Sep 2024 10:31:34 +0200 Subject: [PATCH 03/11] Add support for using `ConsensusParametersV2` --- fuel-tx/src/tests/valid_cases.rs | 1 - .../src/transaction/consensus_parameters.rs | 178 +++++++++++++++++- fuel-vm/src/checked_transaction.rs | 1 - fuel-vm/src/tests/outputs.rs | 1 - fuel-vm/src/util.rs | 2 +- 5 files changed, 171 insertions(+), 12 deletions(-) diff --git a/fuel-tx/src/tests/valid_cases.rs b/fuel-tx/src/tests/valid_cases.rs index 4c4fd3d5de..cf3e1e79ce 100644 --- a/fuel-tx/src/tests/valid_cases.rs +++ b/fuel-tx/src/tests/valid_cases.rs @@ -40,7 +40,6 @@ pub fn test_params() -> ConsensusParameters { Default::default(), Default::default(), Default::default(), - Default::default(), ) } diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 6decf78c4f..b26ce144ec 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -22,11 +22,15 @@ const MAX_GAS: u64 = 100_000_000; #[cfg(feature = "test-helpers")] const MAX_SIZE: u64 = 110 * 1024; +#[derive(Debug)] +pub struct SettingBlockTransactionSizeLimitNotSupported; + /// A versioned set of consensus parameters. #[derive(Clone, Debug, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] pub enum ConsensusParameters { /// Version 1 of the consensus parameters V1(ConsensusParametersV1), + V2(ConsensusParametersV2), } #[cfg(feature = "test-helpers")] @@ -37,19 +41,28 @@ impl Default for ConsensusParameters { } impl ConsensusParameters { + const DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT: u64 = 126 * 1024; + #[cfg(feature = "test-helpers")] /// Constructor for the `ConsensusParameters` with Standard values. pub fn standard() -> Self { ConsensusParametersV1::standard().into() } + #[cfg(feature = "test-helpers")] + /// Constructor for the `ConsensusParameters` with Standard values. + pub fn standard_v2() -> Self { + ConsensusParametersV2::standard().into() + } + #[cfg(feature = "test-helpers")] /// Constructor for the `ConsensusParameters` with Standard values around `ChainId`. pub fn standard_with_id(chain_id: ChainId) -> Self { ConsensusParametersV1::standard_with_id(chain_id).into() } - /// Constructor for the `ConsensusParameters` + // TODO[RC]: Shall we get rid of these constructors? + /// Constructor for the version 1 of `ConsensusParameters` pub const fn new( tx_params: TxParameters, predicate_params: PredicateParameters, @@ -60,10 +73,37 @@ impl ConsensusParameters { gas_costs: GasCosts, base_asset_id: AssetId, block_gas_limit: u64, - block_transaction_size_limit: u32, privileged_address: Address, ) -> Self { Self::V1(ConsensusParametersV1 { + tx_params, + predicate_params, + script_params, + contract_params, + fee_params, + chain_id, + gas_costs, + base_asset_id, + block_gas_limit, + privileged_address, + }) + } + + /// Constructor for the version 2 of `ConsensusParameters` + pub const fn new_v2( + tx_params: TxParameters, + predicate_params: PredicateParameters, + script_params: ScriptParameters, + contract_params: ContractParameters, + fee_params: FeeParameters, + chain_id: ChainId, + gas_costs: GasCosts, + base_asset_id: AssetId, + block_gas_limit: u64, + block_transaction_size_limit: u64, + privileged_address: Address, + ) -> Self { + Self::V2(ConsensusParametersV2 { tx_params, predicate_params, script_params, @@ -82,6 +122,7 @@ impl ConsensusParameters { pub const fn tx_params(&self) -> &TxParameters { match self { Self::V1(params) => ¶ms.tx_params, + Self::V2(params) => ¶ms.tx_params, } } @@ -89,6 +130,7 @@ impl ConsensusParameters { pub const fn predicate_params(&self) -> &PredicateParameters { match self { Self::V1(params) => ¶ms.predicate_params, + Self::V2(params) => ¶ms.predicate_params, } } @@ -96,6 +138,7 @@ impl ConsensusParameters { pub const fn script_params(&self) -> &ScriptParameters { match self { Self::V1(params) => ¶ms.script_params, + Self::V2(params) => ¶ms.script_params, } } @@ -103,6 +146,7 @@ impl ConsensusParameters { pub const fn contract_params(&self) -> &ContractParameters { match self { Self::V1(params) => ¶ms.contract_params, + Self::V2(params) => ¶ms.contract_params, } } @@ -110,6 +154,7 @@ impl ConsensusParameters { pub const fn fee_params(&self) -> &FeeParameters { match self { Self::V1(params) => ¶ms.fee_params, + Self::V2(params) => ¶ms.fee_params, } } @@ -117,6 +162,7 @@ impl ConsensusParameters { pub const fn chain_id(&self) -> ChainId { match self { Self::V1(params) => params.chain_id, + Self::V2(params) => params.chain_id, } } @@ -124,6 +170,7 @@ impl ConsensusParameters { pub const fn gas_costs(&self) -> &GasCosts { match self { Self::V1(params) => ¶ms.gas_costs, + Self::V2(params) => ¶ms.gas_costs, } } @@ -131,6 +178,7 @@ impl ConsensusParameters { pub const fn base_asset_id(&self) -> &AssetId { match self { Self::V1(params) => ¶ms.base_asset_id, + Self::V2(params) => ¶ms.base_asset_id, } } @@ -138,13 +186,18 @@ impl ConsensusParameters { pub const fn block_gas_limit(&self) -> u64 { match self { Self::V1(params) => params.block_gas_limit, + Self::V2(params) => params.block_gas_limit, } } /// Get the block transaction size limit - pub const fn block_transaction_size_limit(&self) -> u32 { + pub fn block_transaction_size_limit(&self) -> u64 { match self { - Self::V1(params) => params.block_transaction_size_limit, + Self::V1(params) => params + .block_gas_limit + .checked_div(self.fee_params().gas_per_byte()) + .unwrap_or(Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT), + Self::V2(params) => params.block_transaction_size_limit, } } @@ -152,6 +205,7 @@ impl ConsensusParameters { pub const fn privileged_address(&self) -> &Address { match self { Self::V1(params) => ¶ms.privileged_address, + Self::V2(params) => ¶ms.privileged_address, } } } @@ -161,6 +215,7 @@ impl ConsensusParameters { pub fn set_tx_params(&mut self, tx_params: TxParameters) { match self { Self::V1(params) => params.tx_params = tx_params, + Self::V2(params) => params.tx_params = tx_params, } } @@ -168,6 +223,7 @@ impl ConsensusParameters { pub fn set_predicate_params(&mut self, predicate_params: PredicateParameters) { match self { Self::V1(params) => params.predicate_params = predicate_params, + Self::V2(params) => params.predicate_params = predicate_params, } } @@ -175,6 +231,7 @@ impl ConsensusParameters { pub fn set_script_params(&mut self, script_params: ScriptParameters) { match self { Self::V1(params) => params.script_params = script_params, + Self::V2(params) => params.script_params = script_params, } } @@ -182,6 +239,7 @@ impl ConsensusParameters { pub fn set_contract_params(&mut self, contract_params: ContractParameters) { match self { Self::V1(params) => params.contract_params = contract_params, + Self::V2(params) => params.contract_params = contract_params, } } @@ -189,6 +247,7 @@ impl ConsensusParameters { pub fn set_fee_params(&mut self, fee_params: FeeParameters) { match self { Self::V1(params) => params.fee_params = fee_params, + Self::V2(params) => params.fee_params = fee_params, } } @@ -196,6 +255,7 @@ impl ConsensusParameters { pub fn set_chain_id(&mut self, chain_id: ChainId) { match self { Self::V1(params) => params.chain_id = chain_id, + Self::V2(params) => params.chain_id = chain_id, } } @@ -203,6 +263,7 @@ impl ConsensusParameters { pub fn set_gas_costs(&mut self, gas_costs: GasCosts) { match self { Self::V1(params) => params.gas_costs = gas_costs, + Self::V2(params) => params.gas_costs = gas_costs, } } @@ -210,6 +271,7 @@ impl ConsensusParameters { pub fn set_base_asset_id(&mut self, base_asset_id: AssetId) { match self { Self::V1(params) => params.base_asset_id = base_asset_id, + Self::V2(params) => params.base_asset_id = base_asset_id, } } @@ -217,6 +279,21 @@ impl ConsensusParameters { pub fn set_block_gas_limit(&mut self, block_gas_limit: u64) { match self { Self::V1(params) => params.block_gas_limit = block_gas_limit, + Self::V2(params) => params.block_gas_limit = block_gas_limit, + } + } + + /// Set the block transaction size limit. + pub fn set_block_transaction_size_limit( + &mut self, + block_transaction_size_limit: u64, + ) -> Result<(), SettingBlockTransactionSizeLimitNotSupported> { + match self { + Self::V1(_) => Err(SettingBlockTransactionSizeLimitNotSupported), + Self::V2(params) => { + params.block_transaction_size_limit = block_transaction_size_limit; + Ok(()) + } } } @@ -224,6 +301,7 @@ impl ConsensusParameters { pub fn set_privileged_address(&mut self, privileged_address: Address) { match self { Self::V1(params) => params.privileged_address = privileged_address, + Self::V2(params) => params.privileged_address = privileged_address, } } } @@ -240,7 +318,6 @@ pub struct ConsensusParametersV1 { pub gas_costs: GasCosts, pub base_asset_id: AssetId, pub block_gas_limit: u64, - pub block_transaction_size_limit: u32, /// The privileged address(user or predicate) that can perform permissioned /// operations(like upgrading the network). pub privileged_address: Address, @@ -265,9 +342,6 @@ impl ConsensusParametersV1 { gas_costs: GasCosts::default(), base_asset_id: Default::default(), block_gas_limit: TxParameters::DEFAULT.max_gas_per_tx(), - // TODO[RC]: Should not be u64::default(), but some other "default" value, - // like 126kb? - block_transaction_size_limit: Default::default(), privileged_address: Default::default(), } } @@ -286,6 +360,63 @@ impl From for ConsensusParameters { } } +/// A collection of parameters for convenience +#[derive(Clone, Debug, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +pub struct ConsensusParametersV2 { + pub tx_params: TxParameters, + pub predicate_params: PredicateParameters, + pub script_params: ScriptParameters, + pub contract_params: ContractParameters, + pub fee_params: FeeParameters, + pub chain_id: ChainId, + pub gas_costs: GasCosts, + pub base_asset_id: AssetId, + pub block_gas_limit: u64, + pub block_transaction_size_limit: u64, + /// The privileged address(user or predicate) that can perform permissioned + /// operations(like upgrading the network). + pub privileged_address: Address, +} + +#[cfg(feature = "test-helpers")] +impl ConsensusParametersV2 { + /// Constructor for the `ConsensusParameters` with Standard values. + pub fn standard() -> Self { + Self::standard_with_id(ChainId::default()) + } + + /// Constructor for the `ConsensusParameters` with Standard values around `ChainId`. + pub fn standard_with_id(chain_id: ChainId) -> Self { + Self { + tx_params: TxParameters::DEFAULT, + predicate_params: PredicateParameters::DEFAULT, + script_params: ScriptParameters::DEFAULT, + contract_params: ContractParameters::DEFAULT, + fee_params: FeeParameters::DEFAULT, + chain_id, + gas_costs: GasCosts::default(), + base_asset_id: Default::default(), + block_gas_limit: TxParameters::DEFAULT.max_gas_per_tx(), + block_transaction_size_limit: + ConsensusParameters::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, + privileged_address: Default::default(), + } + } +} + +#[cfg(feature = "test-helpers")] +impl Default for ConsensusParametersV2 { + fn default() -> Self { + Self::standard() + } +} + +impl From for ConsensusParameters { + fn from(params: ConsensusParametersV2) -> Self { + Self::V2(params) + } +} + /// The versioned fee parameters. #[derive( Copy, Clone, Debug, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize, @@ -929,3 +1060,34 @@ pub mod typescript { } } } + +#[cfg(test)] +mod tests { + use super::{ + ConsensusParameters, + FeeParameters, + }; + + #[test] + fn v1_returns_correct_block_transaction_size_limit_with_non_zero_gas_per_byte() { + let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(2); + let mut consensus_params_v1 = ConsensusParameters::standard(); + consensus_params_v1.set_fee_params(fee_params); + consensus_params_v1.set_block_gas_limit(100); + + assert_eq!(100 / 2, consensus_params_v1.block_transaction_size_limit()) + } + + #[test] + fn v1_returns_correct_block_transaction_size_limit_with_zero_gas_per_byte() { + let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(0); + let mut consensus_params_v1 = ConsensusParameters::standard(); + consensus_params_v1.set_fee_params(fee_params); + consensus_params_v1.set_block_gas_limit(100); + + assert_eq!( + ConsensusParameters::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, + consensus_params_v1.block_transaction_size_limit() + ) + } +} diff --git a/fuel-vm/src/checked_transaction.rs b/fuel-vm/src/checked_transaction.rs index a77975b746..36c33dc0c1 100644 --- a/fuel-vm/src/checked_transaction.rs +++ b/fuel-vm/src/checked_transaction.rs @@ -920,7 +920,6 @@ mod tests { Default::default(), Default::default(), Default::default(), - Default::default(), ) } diff --git a/fuel-vm/src/tests/outputs.rs b/fuel-vm/src/tests/outputs.rs index 498bc500e5..e1c40aa372 100644 --- a/fuel-vm/src/tests/outputs.rs +++ b/fuel-vm/src/tests/outputs.rs @@ -166,7 +166,6 @@ fn correct_change_is_provided_for_coin_outputs_create() { context.get_gas_costs().to_owned(), *context.get_base_asset_id(), context.get_block_gas_limit(), - context.get_block_transaction_size_limit(), *context.get_privileged_address(), ); let create = create diff --git a/fuel-vm/src/util.rs b/fuel-vm/src/util.rs index bbb7fc4d22..1233c479d7 100644 --- a/fuel-vm/src/util.rs +++ b/fuel-vm/src/util.rs @@ -380,7 +380,7 @@ pub mod test_helpers { self.consensus_params.block_gas_limit() } - pub fn get_block_transaction_size_limit(&self) -> u32 { + pub fn get_block_transaction_size_limit(&self) -> u64 { self.consensus_params.block_transaction_size_limit() } From ccd219a425146a7f94b79305259bdfd305c1e053 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Wed, 11 Sep 2024 10:37:31 +0200 Subject: [PATCH 04/11] Update `CHANGELOG.md` --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9d6881dbf..d62b5f3230 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Added +- [#819](https://github.com/FuelLabs/fuel-vm/pull/819): Added `block_transaction_size_limit` to `ConsensusParameters` + ## [Version 0.56.0] ### Added From 1a1f730f4389b95bee9b62fed366a5d953ebce64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Wed, 11 Sep 2024 15:35:11 +0200 Subject: [PATCH 05/11] Remove superfluous constructor `ConsensusParameters::new_v2()` --- fuel-tx/src/tests/valid_cases.rs | 1 + .../src/transaction/consensus_parameters.rs | 30 +------------------ fuel-vm/src/checked_transaction.rs | 1 + fuel-vm/src/tests/outputs.rs | 1 + 4 files changed, 4 insertions(+), 29 deletions(-) diff --git a/fuel-tx/src/tests/valid_cases.rs b/fuel-tx/src/tests/valid_cases.rs index cf3e1e79ce..4c4fd3d5de 100644 --- a/fuel-tx/src/tests/valid_cases.rs +++ b/fuel-tx/src/tests/valid_cases.rs @@ -40,6 +40,7 @@ pub fn test_params() -> ConsensusParameters { Default::default(), Default::default(), Default::default(), + Default::default(), ) } diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index b26ce144ec..abee629dc8 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -61,36 +61,8 @@ impl ConsensusParameters { ConsensusParametersV1::standard_with_id(chain_id).into() } - // TODO[RC]: Shall we get rid of these constructors? - /// Constructor for the version 1 of `ConsensusParameters` + /// Constructor for the `ConsensusParameters` pub const fn new( - tx_params: TxParameters, - predicate_params: PredicateParameters, - script_params: ScriptParameters, - contract_params: ContractParameters, - fee_params: FeeParameters, - chain_id: ChainId, - gas_costs: GasCosts, - base_asset_id: AssetId, - block_gas_limit: u64, - privileged_address: Address, - ) -> Self { - Self::V1(ConsensusParametersV1 { - tx_params, - predicate_params, - script_params, - contract_params, - fee_params, - chain_id, - gas_costs, - base_asset_id, - block_gas_limit, - privileged_address, - }) - } - - /// Constructor for the version 2 of `ConsensusParameters` - pub const fn new_v2( tx_params: TxParameters, predicate_params: PredicateParameters, script_params: ScriptParameters, diff --git a/fuel-vm/src/checked_transaction.rs b/fuel-vm/src/checked_transaction.rs index 36c33dc0c1..a77975b746 100644 --- a/fuel-vm/src/checked_transaction.rs +++ b/fuel-vm/src/checked_transaction.rs @@ -920,6 +920,7 @@ mod tests { Default::default(), Default::default(), Default::default(), + Default::default(), ) } diff --git a/fuel-vm/src/tests/outputs.rs b/fuel-vm/src/tests/outputs.rs index e1c40aa372..498bc500e5 100644 --- a/fuel-vm/src/tests/outputs.rs +++ b/fuel-vm/src/tests/outputs.rs @@ -166,6 +166,7 @@ fn correct_change_is_provided_for_coin_outputs_create() { context.get_gas_costs().to_owned(), *context.get_base_asset_id(), context.get_block_gas_limit(), + context.get_block_transaction_size_limit(), *context.get_privileged_address(), ); let create = create From f92096702c9af710a4b5d8377cfa1c867467ca06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Wed, 11 Sep 2024 15:55:45 +0200 Subject: [PATCH 06/11] Update `CHANGELOG.md` --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6843cd6255..3bcb3762d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,13 +8,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ### Added -- [#819](https://github.com/FuelLabs/fuel-vm/pull/819): Added `block_transaction_size_limit` to `ConsensusParameters` - [#670](https://github.com/FuelLabs/fuel-vm/pull/670): Add DA compression functionality to `Transaction` and any types within - [#733](https://github.com/FuelLabs/fuel-vm/pull/733): Add LibAFL based fuzzer and update `secp256k1` version to 0.29.1. ### Changed #### Breaking +- [#821](https://github.com/FuelLabs/fuel-vm/pull/821): Added `block_transaction_size_limit` to `ConsensusParameters` - [#670](https://github.com/FuelLabs/fuel-vm/pull/670): The `predicate` field of `fuel_tx::input::Coin` is now a wrapper struct `PredicateCode`. ## [Version 0.56.0] From a1ddd0771f2c9d8994e65098d37f0f197a92ac97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Thu, 12 Sep 2024 11:21:49 +0200 Subject: [PATCH 07/11] Remove the superfluous `ConsensusParameters::standard_v2()` constructor and use V2 by default --- .../src/transaction/consensus_parameters.rs | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index abee629dc8..0d4e1b6499 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -46,19 +46,13 @@ impl ConsensusParameters { #[cfg(feature = "test-helpers")] /// Constructor for the `ConsensusParameters` with Standard values. pub fn standard() -> Self { - ConsensusParametersV1::standard().into() - } - - #[cfg(feature = "test-helpers")] - /// Constructor for the `ConsensusParameters` with Standard values. - pub fn standard_v2() -> Self { ConsensusParametersV2::standard().into() } #[cfg(feature = "test-helpers")] /// Constructor for the `ConsensusParameters` with Standard values around `ChainId`. pub fn standard_with_id(chain_id: ChainId) -> Self { - ConsensusParametersV1::standard_with_id(chain_id).into() + ConsensusParametersV2::standard_with_id(chain_id).into() } /// Constructor for the `ConsensusParameters` @@ -1035,6 +1029,8 @@ pub mod typescript { #[cfg(test)] mod tests { + use crate::consensus_parameters::ConsensusParametersV1; + use super::{ ConsensusParameters, FeeParameters, @@ -1042,24 +1038,35 @@ mod tests { #[test] fn v1_returns_correct_block_transaction_size_limit_with_non_zero_gas_per_byte() { - let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(2); - let mut consensus_params_v1 = ConsensusParameters::standard(); - consensus_params_v1.set_fee_params(fee_params); - consensus_params_v1.set_block_gas_limit(100); + const BLOCK_GAS_LIMIT: u64 = 100; + const GAS_PER_BYTE: u64 = 2; - assert_eq!(100 / 2, consensus_params_v1.block_transaction_size_limit()) + let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(GAS_PER_BYTE); + let v1 = ConsensusParametersV1::standard(); + let mut consensus_params: ConsensusParameters = v1.into(); + consensus_params.set_fee_params(fee_params); + consensus_params.set_block_gas_limit(BLOCK_GAS_LIMIT); + + assert_eq!( + BLOCK_GAS_LIMIT / GAS_PER_BYTE, + consensus_params.block_transaction_size_limit() + ) } #[test] fn v1_returns_correct_block_transaction_size_limit_with_zero_gas_per_byte() { - let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(0); - let mut consensus_params_v1 = ConsensusParameters::standard(); - consensus_params_v1.set_fee_params(fee_params); - consensus_params_v1.set_block_gas_limit(100); + const BLOCK_GAS_LIMIT: u64 = 100; + const GAS_PER_BYTE: u64 = 0; + + let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(GAS_PER_BYTE); + let v1 = ConsensusParametersV1::standard(); + let mut consensus_params: ConsensusParameters = v1.into(); + consensus_params.set_fee_params(fee_params); + consensus_params.set_block_gas_limit(BLOCK_GAS_LIMIT); assert_eq!( ConsensusParameters::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, - consensus_params_v1.block_transaction_size_limit() + consensus_params.block_transaction_size_limit() ) } } From 28b3cbb2c2ab973f27affc32e824cca85c7bc60c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= <88321181+rafal-ch@users.noreply.github.com> Date: Thu, 12 Sep 2024 11:22:19 +0200 Subject: [PATCH 08/11] Update CHANGELOG.md Co-authored-by: Green Baneling --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bcb3762d5..9eb3e29a1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed #### Breaking -- [#821](https://github.com/FuelLabs/fuel-vm/pull/821): Added `block_transaction_size_limit` to `ConsensusParameters` +- [#821](https://github.com/FuelLabs/fuel-vm/pull/821): Added `block_transaction_size_limit` to `ConsensusParameters`. It adds a new `ConensusParametersV2` as a variant of the `ConsensusParameters`. - [#670](https://github.com/FuelLabs/fuel-vm/pull/670): The `predicate` field of `fuel_tx::input::Coin` is now a wrapper struct `PredicateCode`. ## [Version 0.56.0] From 65087ba3eb90fc5837ad621affde74f2fa1a7b51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Thu, 12 Sep 2024 11:34:39 +0200 Subject: [PATCH 09/11] Explain the changes in `ConsensusParametersV2` in a doc comment --- fuel-tx/src/transaction/consensus_parameters.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 0d4e1b6499..71783595e1 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -327,6 +327,8 @@ impl From for ConsensusParameters { } /// A collection of parameters for convenience +/// The difference with [`ConsensusParametersV1`]: +/// - `block_transaction_size_limit` has been added. #[derive(Clone, Debug, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] pub struct ConsensusParametersV2 { pub tx_params: TxParameters, From 7b8c9985ccc3271a246064c93c47d71b9252d81d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Thu, 12 Sep 2024 12:51:52 +0200 Subject: [PATCH 10/11] `block_transaction_size_limit` for parametes is set to `u64::MAX` --- .../src/transaction/consensus_parameters.rs | 60 +++---------------- 1 file changed, 8 insertions(+), 52 deletions(-) diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 71783595e1..97abce810c 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -41,8 +41,6 @@ impl Default for ConsensusParameters { } impl ConsensusParameters { - const DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT: u64 = 126 * 1024; - #[cfg(feature = "test-helpers")] /// Constructor for the `ConsensusParameters` with Standard values. pub fn standard() -> Self { @@ -159,10 +157,11 @@ impl ConsensusParameters { /// Get the block transaction size limit pub fn block_transaction_size_limit(&self) -> u64 { match self { - Self::V1(params) => params - .block_gas_limit - .checked_div(self.fee_params().gas_per_byte()) - .unwrap_or(Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT), + Self::V1(_) => { + // In V1 there was no limit on the transaction size. For the sake of + // backwards compatibility we allow for a largest limit possible. + u64::MAX + } Self::V2(params) => params.block_transaction_size_limit, } } @@ -348,6 +347,8 @@ pub struct ConsensusParametersV2 { #[cfg(feature = "test-helpers")] impl ConsensusParametersV2 { + const DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT: u64 = 126 * 1024; + /// Constructor for the `ConsensusParameters` with Standard values. pub fn standard() -> Self { Self::standard_with_id(ChainId::default()) @@ -365,8 +366,7 @@ impl ConsensusParametersV2 { gas_costs: GasCosts::default(), base_asset_id: Default::default(), block_gas_limit: TxParameters::DEFAULT.max_gas_per_tx(), - block_transaction_size_limit: - ConsensusParameters::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, + block_transaction_size_limit: Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, privileged_address: Default::default(), } } @@ -1028,47 +1028,3 @@ pub mod typescript { } } } - -#[cfg(test)] -mod tests { - use crate::consensus_parameters::ConsensusParametersV1; - - use super::{ - ConsensusParameters, - FeeParameters, - }; - - #[test] - fn v1_returns_correct_block_transaction_size_limit_with_non_zero_gas_per_byte() { - const BLOCK_GAS_LIMIT: u64 = 100; - const GAS_PER_BYTE: u64 = 2; - - let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(GAS_PER_BYTE); - let v1 = ConsensusParametersV1::standard(); - let mut consensus_params: ConsensusParameters = v1.into(); - consensus_params.set_fee_params(fee_params); - consensus_params.set_block_gas_limit(BLOCK_GAS_LIMIT); - - assert_eq!( - BLOCK_GAS_LIMIT / GAS_PER_BYTE, - consensus_params.block_transaction_size_limit() - ) - } - - #[test] - fn v1_returns_correct_block_transaction_size_limit_with_zero_gas_per_byte() { - const BLOCK_GAS_LIMIT: u64 = 100; - const GAS_PER_BYTE: u64 = 0; - - let fee_params = FeeParameters::DEFAULT.with_gas_per_byte(GAS_PER_BYTE); - let v1 = ConsensusParametersV1::standard(); - let mut consensus_params: ConsensusParameters = v1.into(); - consensus_params.set_fee_params(fee_params); - consensus_params.set_block_gas_limit(BLOCK_GAS_LIMIT); - - assert_eq!( - ConsensusParameters::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT, - consensus_params.block_transaction_size_limit() - ) - } -} From d07fbc3dbf1a1965fc35450d6d801874c1ffe532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Mon, 16 Sep 2024 11:32:30 +0200 Subject: [PATCH 11/11] Add test showing that it is not possible to set explicit block size limit for legacy (V1) consensus parameters --- .../src/transaction/consensus_parameters.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 97abce810c..e2e410f81b 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -1028,3 +1028,39 @@ pub mod typescript { } } } + +#[cfg(test)] +mod tests { + use crate::consensus_parameters::{ + ConsensusParametersV2, + SettingBlockTransactionSizeLimitNotSupported, + }; + + use super::{ + ConsensusParameters, + ConsensusParametersV1, + }; + + #[test] + fn error_when_setting_block_size_limit_in_consensus_parameters_v1() { + let mut consensus_params: ConsensusParameters = + ConsensusParametersV1::default().into(); + + let result = consensus_params.set_block_transaction_size_limit(0); + + assert!(matches!( + result, + Err(SettingBlockTransactionSizeLimitNotSupported) + )) + } + + #[test] + fn ok_when_setting_block_size_limit_in_consensus_parameters_v2() { + let mut consensus_params: ConsensusParameters = + ConsensusParametersV2::default().into(); + + let result = consensus_params.set_block_transaction_size_limit(0); + + assert!(matches!(result, Ok(()))) + } +}