Add block_transaction_size_limit to ConsensusParameters
#821
Conversation
…onsensus_parameter
rafal-ch
requested review from
xgreenx,
Dentosal and
MitchTurner
as code owners
| #[cfg(feature = "test-helpers")] | ||
| /// Constructor for the `ConsensusParameters` with Standard values. | ||
| pub fn standard_v2() -> Self { | ||
| ConsensusParametersV2::standard().into() | ||
| } |
There was a problem hiding this comment.
We can just use only standard=)
There was a problem hiding this comment.
Updated in a1ddd07
| @@ -273,6 +332,63 @@ impl From<ConsensusParametersV1> for ConsensusParameters { | |||
| } | |||
| } | |||
|
|
|||
| /// A collection of parameters for convenience | |||
There was a problem hiding this comment.
It would be nice to mention what is the difference between V1 and V2 here=) You can check how we do it for GasCosts
…or and use V2 by default
Co-authored-by: Green Baneling <[email protected]>
…meter' into 2133_block_size_consensus_parameter
| Self::V1(params) => params | ||
| .block_gas_limit | ||
| .checked_div(self.fee_params().gas_per_byte()) | ||
| .unwrap_or(Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Updated in 7b8c998
| 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.
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.
There was a problem hiding this comment.
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.
…imit for legacy (V1) consensus parameters
Test added in d07fbc3 |
|
Checked the code again, LGTM. |
Partial implementation of #2133. ## Linked Issues/PRs (FuelLabs/fuel-vm#821) ## Description It adds handling for the updated data struct `ConsensusParameters` (new field: `block_transaction_size_limit`). No additional logic around this new parameter is implemented in this PR, it's just a stub for further work. The new parameter is exposed via GraphQL, example: Request snippet: ```graphql consensusParameters(version: 0) { blockGasLimit blockTransactionSizeLimit } ``` Response snippet: ```json "consensusParameters": { "blockGasLimit": "30000000", "blockTransactionSizeLimit": "129024" } ``` ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] 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 - [ ] 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 <[email protected]> Co-authored-by: Hannes Karppila <[email protected]>
Partial implementation of FuelLabs/fuel-core#2133.
This PR adds a new version (
V2) of theConsensusParameterswhich includes theblock_transaction_size_limitparameter.Linked Issues/PRs
FuelLabs/fuel-core#2188
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]