Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add block_transaction_size_limit to ConsensusParameters #821

Merged
merged 13 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +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`
rafal-ch marked this conversation as resolved.
Show resolved Hide resolved
- [#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]
Expand Down
1 change: 1 addition & 0 deletions fuel-tx/src/tests/valid_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub fn test_params() -> ConsensusParameters {
Default::default(),
Default::default(),
Default::default(),
Default::default(),
)
}

Expand Down
149 changes: 148 additions & 1 deletion fuel-tx/src/transaction/consensus_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -37,12 +41,20 @@ 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()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just use only standard=)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in a1ddd07


#[cfg(feature = "test-helpers")]
/// Constructor for the `ConsensusParameters` with Standard values around `ChainId`.
pub fn standard_with_id(chain_id: ChainId) -> Self {
Expand All @@ -60,9 +72,10 @@ impl ConsensusParameters {
gas_costs: GasCosts,
base_asset_id: AssetId,
block_gas_limit: u64,
block_transaction_size_limit: u64,
privileged_address: Address,
) -> Self {
Self::V1(ConsensusParametersV1 {
Self::V2(ConsensusParametersV2 {
tx_params,
predicate_params,
script_params,
Expand All @@ -72,6 +85,7 @@ impl ConsensusParameters {
gas_costs,
base_asset_id,
block_gas_limit,
block_transaction_size_limit,
privileged_address,
})
}
Expand All @@ -80,69 +94,90 @@ impl ConsensusParameters {
pub const fn tx_params(&self) -> &TxParameters {
match self {
Self::V1(params) => &params.tx_params,
Self::V2(params) => &params.tx_params,
}
}

/// Get the predicate parameters
pub const fn predicate_params(&self) -> &PredicateParameters {
match self {
Self::V1(params) => &params.predicate_params,
Self::V2(params) => &params.predicate_params,
}
}

/// Get the script parameters
pub const fn script_params(&self) -> &ScriptParameters {
match self {
Self::V1(params) => &params.script_params,
Self::V2(params) => &params.script_params,
}
}

/// Get the contract parameters
pub const fn contract_params(&self) -> &ContractParameters {
match self {
Self::V1(params) => &params.contract_params,
Self::V2(params) => &params.contract_params,
}
}

/// Get the fee parameters
pub const fn fee_params(&self) -> &FeeParameters {
match self {
Self::V1(params) => &params.fee_params,
Self::V2(params) => &params.fee_params,
}
}

/// Get the chain ID
pub const fn chain_id(&self) -> ChainId {
match self {
Self::V1(params) => params.chain_id,
Self::V2(params) => params.chain_id,
}
}

/// Get the gas costs
pub const fn gas_costs(&self) -> &GasCosts {
match self {
Self::V1(params) => &params.gas_costs,
Self::V2(params) => &params.gas_costs,
}
}

/// Get the base asset ID
pub const fn base_asset_id(&self) -> &AssetId {
match self {
Self::V1(params) => &params.base_asset_id,
Self::V2(params) => &params.base_asset_id,
}
}

/// Get the block gas limit
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 fn block_transaction_size_limit(&self) -> u64 {
match self {
Self::V1(params) => params
.block_gas_limit
acerone85 marked this conversation as resolved.
Show resolved Hide resolved
.checked_div(self.fee_params().gas_per_byte())
.unwrap_or(Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking a little bit more about it, I think we need to return u64::MAX for backward compatibility reasons. Because in the past it was not limited

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 7b8c998

Self::V2(params) => params.block_transaction_size_limit,
}
}

/// Get the privileged address
pub const fn privileged_address(&self) -> &Address {
match self {
Self::V1(params) => &params.privileged_address,
Self::V2(params) => &params.privileged_address,
}
}
}
Expand All @@ -152,69 +187,93 @@ 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,
}
}

/// Set the predicate parameters.
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,
}
}

/// Set the script parameters.
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,
}
}

/// Set the contract parameters.
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,
}
}

/// Set the fee parameters.
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,
}
}

/// Set the chain ID.
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,
}
}

/// Set the gas costs.
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,
}
}

/// Set the base asset ID.
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,
}
}

/// Set the block gas limit.
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(())
}
}
}

/// Set the privileged address.
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,
}
}
}
Expand Down Expand Up @@ -273,6 +332,63 @@ impl From<ConsensusParametersV1> for ConsensusParameters {
}
}

/// A collection of parameters for convenience
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to mention what is the difference between V1 and V2 here=) You can check how we do it for GasCosts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update in 65087ba

#[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,
Copy link
Contributor

@acerone85 acerone85 Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not related to this PR only, but in general I think we should strive to have as much documentation as possible, including individual fields of structs.

For example in this case it is not 100% clear if block_transaction_size_limit refers to the number of transactions in a block, or the number of bytes in a transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we could enforce this with missing_docs lint, but it'll cause a swarm of warnings.

Also, I see an indication that it is already on our radar.

/// 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<ConsensusParametersV2> 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,
Expand Down Expand Up @@ -916,3 +1032,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()
)
}
}
1 change: 1 addition & 0 deletions fuel-vm/src/checked_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,7 @@ mod tests {
Default::default(),
Default::default(),
Default::default(),
Default::default(),
)
}

Expand Down
1 change: 1 addition & 0 deletions fuel-vm/src/tests/outputs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions fuel-vm/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,10 @@ pub mod test_helpers {
self.consensus_params.block_gas_limit()
}

pub fn get_block_transaction_size_limit(&self) -> u64 {
self.consensus_params.block_transaction_size_limit()
}

pub fn get_privileged_address(&self) -> &Address {
self.consensus_params.privileged_address()
}
Expand Down
Loading