-
Notifications
You must be signed in to change notification settings - Fork 88
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
Changes from all commits
ac17f85
6c29921
1fa9ee4
ccd219a
4f8ecbc
1a1f730
f920967
a1ddd07
28b3cbb
65087ba
8bc3bfd
7b8c998
d07fbc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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")] | ||
|
@@ -40,13 +44,13 @@ impl ConsensusParameters { | |
#[cfg(feature = "test-helpers")] | ||
/// Constructor for the `ConsensusParameters` with Standard values. | ||
pub fn standard() -> Self { | ||
ConsensusParametersV1::standard().into() | ||
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` | ||
|
@@ -60,9 +64,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, | ||
|
@@ -72,6 +77,7 @@ impl ConsensusParameters { | |
gas_costs, | ||
base_asset_id, | ||
block_gas_limit, | ||
block_transaction_size_limit, | ||
privileged_address, | ||
}) | ||
} | ||
|
@@ -80,69 +86,91 @@ impl ConsensusParameters { | |
pub const fn tx_params(&self) -> &TxParameters { | ||
match self { | ||
Self::V1(params) => ¶ms.tx_params, | ||
Self::V2(params) => ¶ms.tx_params, | ||
} | ||
} | ||
|
||
/// Get the predicate parameters | ||
pub const fn predicate_params(&self) -> &PredicateParameters { | ||
match self { | ||
Self::V1(params) => ¶ms.predicate_params, | ||
Self::V2(params) => ¶ms.predicate_params, | ||
} | ||
} | ||
|
||
/// Get the script parameters | ||
pub const fn script_params(&self) -> &ScriptParameters { | ||
match self { | ||
Self::V1(params) => ¶ms.script_params, | ||
Self::V2(params) => ¶ms.script_params, | ||
} | ||
} | ||
|
||
/// Get the contract parameters | ||
pub const fn contract_params(&self) -> &ContractParameters { | ||
match self { | ||
Self::V1(params) => ¶ms.contract_params, | ||
Self::V2(params) => ¶ms.contract_params, | ||
} | ||
} | ||
|
||
/// Get the fee parameters | ||
pub const fn fee_params(&self) -> &FeeParameters { | ||
match self { | ||
Self::V1(params) => ¶ms.fee_params, | ||
Self::V2(params) => ¶ms.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) => ¶ms.gas_costs, | ||
Self::V2(params) => ¶ms.gas_costs, | ||
} | ||
} | ||
|
||
/// Get the base asset ID | ||
pub const fn base_asset_id(&self) -> &AssetId { | ||
match self { | ||
Self::V1(params) => ¶ms.base_asset_id, | ||
Self::V2(params) => ¶ms.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(_) => { | ||
// 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, | ||
} | ||
} | ||
|
||
/// Get the privileged address | ||
pub const fn privileged_address(&self) -> &Address { | ||
match self { | ||
Self::V1(params) => ¶ms.privileged_address, | ||
Self::V2(params) => ¶ms.privileged_address, | ||
} | ||
} | ||
} | ||
|
@@ -152,69 +180,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, | ||
} | ||
} | ||
} | ||
|
@@ -273,6 +325,66 @@ impl From<ConsensusParametersV1> 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, | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we could enforce this with 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 { | ||
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()) | ||
} | ||
|
||
/// 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: Self::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, | ||
|
@@ -916,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(()))) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -920,6 +920,7 @@ mod tests { | |
Default::default(), | ||
Default::default(), | ||
Default::default(), | ||
Default::default(), | ||
) | ||
} | ||
|
||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update in 65087ba