Add `block_transaction_size_limit` to `ConsensusParameters` #821

rafal-ch · 2024-09-11T13:41:29Z

Partial implementation of FuelLabs/fuel-core#2133.

This PR adds a new version (V2) of the ConsensusParameters which includes the block_transaction_size_limit parameter.

Linked Issues/PRs

FuelLabs/fuel-core#2188

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification 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
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

…onsensus_parameter

xgreenx · 2024-09-12T08:50:02Z

fuel-tx/src/transaction/consensus_parameters.rs

+    #[cfg(feature = "test-helpers")]
+    /// Constructor for the `ConsensusParameters` with Standard values.
+    pub fn standard_v2() -> Self {
+        ConsensusParametersV2::standard().into()
+    }


We can just use only standard=)

Updated in a1ddd07

CHANGELOG.md

xgreenx · 2024-09-12T08:51:35Z

fuel-tx/src/transaction/consensus_parameters.rs

@@ -273,6 +332,63 @@ impl From<ConsensusParametersV1> for ConsensusParameters {
    }
 }

+/// A collection of parameters for convenience


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

Update in 65087ba

…or and use V2 by default

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

…meter' into 2133_block_size_consensus_parameter

fuel-tx/src/transaction/consensus_parameters.rs

xgreenx · 2024-09-12T09:51:48Z

fuel-tx/src/transaction/consensus_parameters.rs

+            Self::V1(params) => params
+                .block_gas_limit
+                .checked_div(self.fee_params().gas_per_byte())
+                .unwrap_or(Self::DEFAULT_BLOCK_TRANSACTION_SIZE_LIMIT),


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

Updated in 7b8c998

acerone85 · 2024-09-12T10:01:05Z

fuel-tx/src/transaction/consensus_parameters.rs

+    pub gas_costs: GasCosts,
+    pub base_asset_id: AssetId,
+    pub block_gas_limit: u64,
+    pub block_transaction_size_limit: u64,


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.

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.

rymnc

LGTM, a test which asserts SettingBlockTransactionSizeLimitNotSupported is thrown would be nice :)

…imit for legacy (V1) consensus parameters

rafal-ch · 2024-09-16T09:33:39Z

LGTM, a test which asserts SettingBlockTransactionSizeLimitNotSupported is thrown would be nice :)

Test added in d07fbc3

acerone85 · 2024-09-16T11:11:21Z

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 <xgreenx9999@gmail.com> Co-authored-by: Hannes Karppila <2204863+Dentosal@users.noreply.github.com>

rafal-ch added 6 commits September 6, 2024 18:36

Add block_transaction_size_limit to ConsensusParameters

ac17f85

Satisfy clippy

6c29921

Add support for using ConsensusParametersV2

1fa9ee4

Update CHANGELOG.md

ccd219a

Merge remote-tracking branch 'upstream/master' into 2133_block_size_c…

4f8ecbc

…onsensus_parameter

Remove superfluous constructor ConsensusParameters::new_v2()

1a1f730

rafal-ch mentioned this pull request Sep 11, 2024

Add block_transaction_size_limit to ConsensusParameters #819

Closed

9 tasks

Update CHANGELOG.md

f920967

rafal-ch mentioned this pull request Sep 11, 2024

Add support for ConsensusParametersVersion::V2 FuelLabs/fuel-core#2188

Merged

9 tasks

rafal-ch marked this pull request as ready for review September 12, 2024 08:48

rafal-ch requested review from xgreenx, Dentosal and MitchTurner as code owners September 12, 2024 08:48

rafal-ch requested a review from a team September 12, 2024 08:50

xgreenx reviewed Sep 12, 2024

View reviewed changes

rafal-ch and others added 4 commits September 12, 2024 11:21

Remove the superfluous ConsensusParameters::standard_v2() construct…

a1ddd07

…or and use V2 by default

Update CHANGELOG.md

28b3cbb

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

Explain the changes in ConsensusParametersV2 in a doc comment

65087ba

Merge remote-tracking branch 'upstream/2133_block_size_consensus_para…

8bc3bfd

…meter' into 2133_block_size_consensus_parameter

acerone85 reviewed Sep 12, 2024

View reviewed changes

fuel-tx/src/transaction/consensus_parameters.rs Outdated Show resolved Hide resolved

xgreenx reviewed Sep 12, 2024

View reviewed changes

acerone85 reviewed Sep 12, 2024

View reviewed changes

block_transaction_size_limit for parametes is set to u64::MAX

7b8c998

xgreenx previously approved these changes Sep 12, 2024

View reviewed changes

rymnc reviewed Sep 16, 2024

View reviewed changes

Add test showing that it is not possible to set explicit block size l…

d07fbc3

…imit for legacy (V1) consensus parameters

rafal-ch dismissed xgreenx’s stale review via d07fbc3 September 16, 2024 09:32

acerone85 approved these changes Sep 16, 2024

View reviewed changes

xgreenx approved these changes Sep 16, 2024

View reviewed changes

rafal-ch added this pull request to the merge queue Sep 16, 2024

Merged via the queue into master with commit 2999f40 Sep 16, 2024
39 checks passed

rafal-ch deleted the 2133_block_size_consensus_parameter branch September 16, 2024 12:49

xgreenx mentioned this pull request Sep 17, 2024

Release v0.57.0 #828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `block_transaction_size_limit` to `ConsensusParameters` #821

Add `block_transaction_size_limit` to `ConsensusParameters` #821

rafal-ch commented Sep 11, 2024 •

edited

Loading

xgreenx Sep 12, 2024

rafal-ch Sep 12, 2024

xgreenx Sep 12, 2024

rafal-ch Sep 12, 2024

xgreenx Sep 12, 2024

rafal-ch Sep 12, 2024

acerone85 Sep 12, 2024 •

edited

Loading

rafal-ch Sep 12, 2024

rymnc left a comment

rafal-ch commented Sep 16, 2024

acerone85 commented Sep 16, 2024

Add block_transaction_size_limit to ConsensusParameters #821

Add block_transaction_size_limit to ConsensusParameters #821

Conversation

rafal-ch commented Sep 11, 2024 • edited Loading

Linked Issues/PRs

Checklist

Before requesting review

After merging, notify other teams

xgreenx Sep 12, 2024

Choose a reason for hiding this comment

rafal-ch Sep 12, 2024

Choose a reason for hiding this comment

xgreenx Sep 12, 2024

Choose a reason for hiding this comment

rafal-ch Sep 12, 2024

Choose a reason for hiding this comment

xgreenx Sep 12, 2024

Choose a reason for hiding this comment

rafal-ch Sep 12, 2024

Choose a reason for hiding this comment

acerone85 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

rafal-ch Sep 12, 2024

Choose a reason for hiding this comment

rymnc left a comment

Choose a reason for hiding this comment

rafal-ch commented Sep 16, 2024

acerone85 commented Sep 16, 2024

Add `block_transaction_size_limit` to `ConsensusParameters` #821

Add `block_transaction_size_limit` to `ConsensusParameters` #821

rafal-ch commented Sep 11, 2024 •

edited

Loading

acerone85 Sep 12, 2024 •

edited

Loading