chore(sealed_blocks): optimize fetching of sealed block header at a given height #2112

rymnc · 2024-08-20T09:57:11Z

Linked Issues/PRs

P2P is doing a lot of database lookups #2023

Description

We don't need to fetch the header and the consensus header from the storage if either one of them is None. If consensus evaluates to None, we can early return a None, but if the consensus header exists, we can then fetch the block header.

Performs another optimization in instances where we fetch the SealedBlock and only end up using the entity wrapped within it, and not the consensus headers. Previously, both the consensus headers and the block were fetched from the db and then returned as a SealedBlock, Now, we just check for existence of a given block height in the SealedBlockConsensus table, and avoid the allocation for the consensus headers.

note: also restricts how many blocks worth of transactions can be requested over p2p

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?

rymnc · 2024-08-20T15:17:19Z

added block range restriction in 0a7b23a since the CI runners are struggling with all the open PRs

rymnc · 2024-08-20T15:35:06Z

p2p metrics issue: #808

…iven height

xgreenx · 2024-08-21T08:41:29Z

crates/fuel-core/src/database/sealed_block.rs

+        &self,
+        height: &BlockHeight,
+    ) -> StorageResult<Option<Block>> {
+        if self


Why do we want to do an additional lookup into SealedBlockConsensus table? We can go directly to the self.get_full_block

addressed in 5498c5f and cleaned up in d090523

xgreenx · 2024-08-21T08:43:19Z

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

 ### Changed
 - [2106](https://github.com/FuelLabs/fuel-core/pull/2106): Remove deadline clock in POA and replace with tokio time functions.
+- [2112](https://github.com/FuelLabs/fuel-core/pull/2112): Alter the way the sealed blocks are fetched with a given height.


We need to move it to ## [Unreleased] section

addressed in da650f4

xgreenx · 2024-08-21T08:43:52Z

bin/fuel-core/src/cli/run/p2p.rs

@@ -66,6 +66,10 @@ pub struct P2PArgs {
    #[clap(long = "max-headers-per-request", default_value = "100", env)]
    pub max_headers_per_request: u32,

+    /// Max number of blocks in a transaction request response
+    #[clap(long = "max-blocks-per-tx-request", default_value = "100", env)]
+    pub max_blocks_per_tx_request: u32,


I think we can reuse max_headers_per_request to reduce number of arguments

addressed in 06262b9

crates/fuel-core/src/database/sealed_block.rs

Co-authored-by: Mårten Blankfors <nilssonmartenx@gmail.com>

crates/services/p2p/src/service.rs

netrome · 2024-08-21T09:02:09Z

crates/services/p2p/src/service.rs

+                    .try_into()
+                    .expect("u32 should always fit into usize");
+                if range.len() > max_len {
+                    tracing::error!("Requested range of blocks is too big. Requested length: {:?}, Max length: {:?}", range.len(), max_len);


The logs become more searchable if we add the information as fields instead of formatting them into the message:

Suggested change

tracing::error!("Requested range of blocks is too big. Requested length: {:?}, Max length: {:?}", range.len(), max_len);

tracing::error!(requested_length = range.len(), max_len, "Requested range of blocks is too big");

addressed in ebca20d, with the other usage of this as well

netrome

Just a few nits and enhancement suggestions from me, otherwise the PR looks good!

xgreenx · 2024-08-21T13:36:02Z

crates/services/p2p/src/service.rs

@@ -437,6 +437,24 @@ where
        let response_channel = self.request_sender.clone();
        match request_message {
            RequestMessage::Transactions(range) => {
+                let max_len = self


We can use u32 instead of usize to avoid try_into

## Linked Issues/PRs  - #2112 ## Description  - Addressing #2112 (comment) A follow up PR will be made to address #2121 ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] 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?

## Linked Issues/PRs  - #2112 - #2121 ## Description  Cleans up the impl for `TaskRequest` by breaking the processing into digestable but extendable functions ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] 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 Baneling <XgreenX9999@gmail.com>

@AurelienFT

## Version v0.35.0 ### Added - [2122](#2122): Changed the relayer URI address to be a vector and use a quorum provider. The `relayer` argument now supports multiple URLs to fetch information from different sources. - [2119](#2119): GraphQL query fields for retrieving information about upgrades. ### Changed - [2113](#2113): Modify the way the gas price service and shared algo is initialized to have some default value based on best guess instead of `None`, and initialize service before graphql. - [2112](#2112): Alter the way the sealed blocks are fetched with a given height. - [2120](#2120): Added `submitAndAwaitStatus` subscription endpoint which returns the `SubmittedStatus` after the transaction is submitted as well as the `TransactionStatus` subscription. - [2115](#2115): Add test for `SignMode` `is_available` method. - [2124](#2124): Generalize the way p2p req/res protocol handles requests. #### Breaking - [2040](#2040): Added full `no_std` support state transition related crates. The crates now require the "alloc" feature to be enabled. Following crates are affected: - `fuel-core-types` - `fuel-core-storage` - `fuel-core-executor` - [2116](#2116): Replace `H160` in config and cli options of relayer by `Bytes20` of `fuel-types` ### Fixed - [2134](#2134): Perform RecoveryID normalization for AWS KMS -generated signatures. ## What's Changed * Change TODO clippy to wait for false positive correction by @AurelienFT in #2110 * Remove option on shared algo, give default algo, reorder service startup by @MitchTurner in #2113 * chore(sealed_blocks): optimize fetching of sealed block header at a given height by @rymnc in #2112 * chore(p2p_service): remove unnecessary cast to usize by @rymnc in #2123 * Added a benchmark for the predicate with ed19 verification by @xgreenx in #2127 * Add test SignMode is_available by @AurelienFT in #2115 * feat: Add graphql query fields for retreiving information about upgrades by @netrome in #2119 * Small code optimizations by @MoneyBund in #2035 * Change relayer URI address to be vector and use quorum provider by @AurelienFT in #2122 * chore(p2p_service): clean up processing of p2p req/res protocol by @rymnc in #2124 * feat: add `submitAndAwaitStatus` subscription endpoint by @maschad in #2120 * Replace H160 in config and cli options of relayer by Bytes20 by @AurelienFT in #2116 * Secp256k1 RecoveryID correction for KMS-genrated signatures by @Dentosal in #2134 * Added `no_std` support state transition related crates by @xgreenx in #2040 ## New Contributors * @MoneyBund made their first contribution in #2035 * @maschad made their first contribution in #2120 **Full Changelog**: v0.34.0...v0.35.0

…2p req/res protocol (#2135) ## Linked Issues/PRs  - #2023 - #2112 ## Description  We bubble up the usage of a new function `log_metrics` to clean up how metrics are being logged in the p2p module. additionally, we also specify a `Gauge` for how many blocks worth of data has been requested, so we can re-use those numbers in our simulations to find the most optimal lookup method. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] 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?

rymnc self-assigned this Aug 20, 2024

rymnc force-pushed the chore/optimize-get-sealed-block-header branch from 05cf17d to 93fefa5 Compare August 20, 2024 10:03

rymnc requested a review from a team August 20, 2024 14:18

rymnc marked this pull request as ready for review August 20, 2024 14:18

rymnc force-pushed the chore/optimize-get-sealed-block-header branch from 8db2bd7 to 9f3948d Compare August 20, 2024 14:49

rymnc added 7 commits August 20, 2024 21:42

chore(sealed_blocks): optimize fetching of sealed block header at a g…

b4a63cd

…iven height

fix: changelog

0555645

chore: another usage of unnecessary double get

460265d

fix: optimize block fetching if we only care if it has been sealed

94cee38

fix: clippy

2680878

fix: rename finalized block

ee5820e

chore: also restrict how many blocks can be requested over p2p

3749d6c

rymnc force-pushed the chore/optimize-get-sealed-block-header branch from 0a7b23a to 3749d6c Compare August 20, 2024 16:12

Merge branch 'master' into chore/optimize-get-sealed-block-header

f36b280

xgreenx reviewed Aug 21, 2024

View reviewed changes

xgreenx and others added 2 commits August 21, 2024 10:44

Merge branch 'master' into chore/optimize-get-sealed-block-header

6880cc0

fix: remove additional cli arg

06262b9

netrome reviewed Aug 21, 2024

View reviewed changes

crates/fuel-core/src/database/sealed_block.rs Outdated Show resolved Hide resolved

rymnc and others added 3 commits August 21, 2024 10:52

fix: dont lookup SealedBlockConsensus when getting full block

5498c5f

fix: changelog

da650f4

chore: docstring update

81efb04

Co-authored-by: Mårten Blankfors <nilssonmartenx@gmail.com>

rymnc requested review from xgreenx and netrome August 21, 2024 08:55

fix: remove duped fn

d090523

netrome reviewed Aug 21, 2024

View reviewed changes

crates/services/p2p/src/service.rs Outdated Show resolved Hide resolved

MitchTurner previously approved these changes Aug 21, 2024

View reviewed changes

netrome reviewed Aug 21, 2024

View reviewed changes

netrome previously approved these changes Aug 21, 2024

View reviewed changes

fix: clean up tracing error

ebca20d

rymnc dismissed stale reviews from netrome and MitchTurner via ebca20d August 21, 2024 09:05

rymnc requested review from netrome and MitchTurner August 21, 2024 09:06

Merge branch 'master' into chore/optimize-get-sealed-block-header

0beb28a

MitchTurner previously approved these changes Aug 21, 2024

View reviewed changes

fix: make pub and not pub(crate)

0ad1866

rymnc dismissed MitchTurner’s stale review via 0ad1866 August 21, 2024 10:18

Merge branch 'master' into chore/optimize-get-sealed-block-header

d3d6c49

rymnc requested a review from MitchTurner August 21, 2024 13:28

MitchTurner previously approved these changes Aug 21, 2024

View reviewed changes

fix: dont change cli arg

61525ef

rymnc dismissed MitchTurner’s stale review via 61525ef August 21, 2024 13:33

rymnc requested a review from MitchTurner August 21, 2024 13:33

xgreenx approved these changes Aug 21, 2024

View reviewed changes

netrome approved these changes Aug 21, 2024

View reviewed changes

rymnc enabled auto-merge (squash) August 21, 2024 13:40

rymnc merged commit 5871591 into master Aug 21, 2024
31 checks passed

rymnc deleted the chore/optimize-get-sealed-block-header branch August 21, 2024 14:00

rymnc mentioned this pull request Aug 21, 2024

chore(p2p_service): remove unnecessary cast to usize #2123

Merged

9 tasks

rymnc mentioned this pull request Aug 23, 2024

chore(p2p_service): clean up processing of p2p req/res protocol #2124

Merged

9 tasks

rymnc mentioned this pull request Aug 28, 2024

chore(p2p_service): add metrics for number of blocks requested over p2p req/res protocol #2135

Merged

9 tasks

xgreenx mentioned this pull request Aug 28, 2024

Release v0.35.0 #2136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(sealed_blocks): optimize fetching of sealed block header at a given height #2112

chore(sealed_blocks): optimize fetching of sealed block header at a given height #2112

rymnc commented Aug 20, 2024 •

edited

Loading

rymnc commented Aug 20, 2024

rymnc commented Aug 20, 2024

xgreenx Aug 21, 2024

rymnc Aug 21, 2024 •

edited

Loading

xgreenx Aug 21, 2024

rymnc Aug 21, 2024

xgreenx Aug 21, 2024

rymnc Aug 21, 2024

netrome Aug 21, 2024

rymnc Aug 21, 2024

netrome left a comment

xgreenx Aug 21, 2024

	tracing::error!("Requested range of blocks is too big. Requested length: {:?}, Max length: {:?}", range.len(), max_len);
	tracing::error!(requested_length = range.len(), max_len, "Requested range of blocks is too big");

chore(sealed_blocks): optimize fetching of sealed block header at a given height #2112

chore(sealed_blocks): optimize fetching of sealed block header at a given height #2112

Conversation

rymnc commented Aug 20, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

rymnc commented Aug 20, 2024

rymnc commented Aug 20, 2024

Choose a reason for hiding this comment

rymnc Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Aug 20, 2024 •

edited

Loading

rymnc Aug 21, 2024 •

edited

Loading