Skip to content

Commit

Permalink
chore(sealed_blocks): optimize fetching of sealed block header at a g…
Browse files Browse the repository at this point in the history
…iven height (#2112)

## Linked Issues/PRs
<!-- List of related issues/PRs -->
- #2023

## Description
<!-- List of detailed changes -->
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
- [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 <[email protected]>
Co-authored-by: Mårten Blankfors <[email protected]>
  • Loading branch information
3 people authored Aug 21, 2024
1 parent b602a5d commit 5871591
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 27 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Changed
- [2113](https://github.com/FuelLabs/fuel-core/pull/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](https://github.com/FuelLabs/fuel-core/pull/2112): Alter the way the sealed blocks are fetched with a given height.


## [Version 0.34.0]

Expand Down
4 changes: 2 additions & 2 deletions bin/fuel-core/src/cli/run/p2p.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub struct P2PArgs {
#[clap(long = "max-block-size", default_value = MAX_RESPONSE_SIZE_STR, env)]
pub max_block_size: usize,

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

Expand All @@ -76,7 +76,7 @@ pub struct P2PArgs {
#[clap(long = "reserved-nodes", value_delimiter = ',', env)]
pub reserved_nodes: Vec<Multiaddr>,

/// With this set to `true` you create a guarded node that is only ever connected to trusted, reserved nodes.
/// With this set to `true` you create a guarded node that is only ever connected to trusted, reserved nodes.
#[clap(long = "reserved-nodes-only-mode", env)]
pub reserved_nodes_only_mode: bool,

Expand Down
5 changes: 1 addition & 4 deletions crates/fuel-core/src/database/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,7 @@ impl OnChainIterableKeyValueView {
}

/// Retrieve the full block and all associated transactions
pub(crate) fn get_full_block(
&self,
height: &BlockHeight,
) -> StorageResult<Option<Block>> {
pub fn get_full_block(&self, height: &BlockHeight) -> StorageResult<Option<Block>> {
let db_block = self.storage::<FuelBlocks>().get(height)?;
if let Some(block) = db_block {
// fetch all the transactions
Expand Down
18 changes: 11 additions & 7 deletions crates/fuel-core/src/database/sealed_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use fuel_core_types::{
consensus::{
Consensus,
Genesis,
Sealed,
},
SealedBlock,
SealedBlockHeader,
Expand All @@ -36,10 +35,11 @@ impl OnChainIterableKeyValueView {
height: &BlockHeight,
) -> StorageResult<Option<SealedBlock>> {
// combine the block and consensus metadata into a sealed fuel block type
let block = self.get_full_block(height)?;
let consensus = self.storage::<SealedBlockConsensus>().get(height)?;

if let (Some(block), Some(consensus)) = (block, consensus) {
if let Some(consensus) = consensus {
let block = self.get_full_block(height)?.ok_or(not_found!(FuelBlocks))?;

let sealed_block = SealedBlock {
entity: block,
consensus: consensus.into_owned(),
Expand Down Expand Up @@ -96,10 +96,14 @@ impl OnChainIterableKeyValueView {
&self,
height: &BlockHeight,
) -> StorageResult<Option<SealedBlockHeader>> {
let header = self.storage::<FuelBlocks>().get(height)?;
let consensus = self.storage::<SealedBlockConsensus>().get(height)?;

if let (Some(header), Some(consensus)) = (header, consensus) {
if let Some(consensus) = consensus {
let header = self
.storage::<FuelBlocks>()
.get(height)?
.ok_or(not_found!(FuelBlocks))?; // This shouldn't happen if a block has been sealed

let sealed_block = SealedBlockHeader {
entity: header.into_owned().header().clone(),
consensus: consensus.into_owned(),
Expand All @@ -120,8 +124,8 @@ impl OnChainIterableKeyValueView {
.map(BlockHeight::from)
.map(|block_height| {
let transactions = self
.get_sealed_block_by_height(&block_height)?
.map(|Sealed { entity: block, .. }| block.into_inner().1)
.get_full_block(&block_height)?
.map(|block| block.into_inner().1)
.map(Transactions);
Ok(transactions)
})
Expand Down
2 changes: 1 addition & 1 deletion crates/services/p2p/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const REQ_RES_TIMEOUT: Duration = Duration::from_secs(20);
/// - `nginx.ingress.kubernetes.io/proxy-body-size`
pub const MAX_RESPONSE_SIZE: usize = 18 * 1024 * 1024;

/// Maximum number of headers per request.
/// Maximum number of blocks per request.
pub const MAX_HEADERS_PER_REQUEST: u32 = 100;

#[derive(Clone, Debug)]
Expand Down
25 changes: 23 additions & 2 deletions crates/services/p2p/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,24 @@ where
let response_channel = self.request_sender.clone();
match request_message {
RequestMessage::Transactions(range) => {
let max_len = self
.max_headers_per_request
.try_into()
.expect("u32 should always fit into usize");
if range.len() > max_len {
tracing::error!(
requested_length = range.len(),
max_len,
"Requested range of blocks is too big"
);
let response = None;
let _ = self.p2p_service.send_response_msg(
request_id,
ResponseMessage::Transactions(response),
);
return Ok(())
}

let view = self.view_provider.latest_view()?;
let result = self.database_processor.spawn(move || {
if instant.elapsed() > timeout {
Expand Down Expand Up @@ -479,8 +497,11 @@ where
.try_into()
.expect("u32 should always fit into usize");
if range.len() > max_len {
tracing::error!("Requested range of sealed headers is too big. Requested length: {:?}, Max length: {:?}", range.len(), max_len);
// TODO: Return helpful error message to requester. https://github.com/FuelLabs/fuel-core/issues/1311
tracing::error!(
requested_length = range.len(),
max_len,
"Requested range of sealed headers is too big"
); // TODO: Return helpful error message to requester. https://github.com/FuelLabs/fuel-core/issues/1311
let response = None;
let _ = self.p2p_service.send_response_msg(
request_id,
Expand Down
14 changes: 6 additions & 8 deletions tests/tests/poa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,10 @@ async fn starting_node_with_predefined_nodes_produces_these_predefined_blocks(
let predefined_blocks: Vec<_> = (1..=BLOCK_TO_PRODUCE)
.map(|block_height| {
let block_height = block_height as u32;
let sealed_block = on_chain_view
.get_sealed_block_by_height(&block_height.into())
on_chain_view
.get_full_block(&block_height.into())
.unwrap()
.unwrap()
.unwrap();
sealed_block.entity
})
.collect();
assert_eq!(predefined_blocks.len(), BLOCK_TO_PRODUCE);
Expand Down Expand Up @@ -216,11 +215,10 @@ async fn starting_node_with_predefined_nodes_produces_these_predefined_blocks(
let blocks_from_new_node: Vec<_> = (1..=BLOCK_TO_PRODUCE)
.map(|block_height| {
let block_height = block_height as u32;
let sealed_block = on_chain_view
.get_sealed_block_by_height(&block_height.into())
on_chain_view
.get_full_block(&block_height.into())
.unwrap()
.unwrap()
.unwrap();
sealed_block.entity
})
.collect();
assert_eq!(predefined_blocks, blocks_from_new_node);
Expand Down
5 changes: 2 additions & 3 deletions tests/tests/state_rewind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,10 @@ async fn validate_block_at_any_height__only_transfers() -> anyhow::Result<()> {
for i in 0..TOTAL_BLOCKS {
let height_to_execute = rng.gen_range(1..last_block_height);

let sealed_block = view
.get_sealed_block_by_height(&height_to_execute.into())
let block = view
.get_full_block(&height_to_execute.into())
.unwrap()
.unwrap();
let block = sealed_block.entity;

// When
tracing::info!("Validating block {i} at height {}", height_to_execute);
Expand Down

0 comments on commit 5871591

Please sign in to comment.