Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fetch blobs from EL prior to block verification #6600

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open

Fetch blobs from EL prior to block verification #6600

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3cfe9df
Fetch blobs from EL prior to block verification
michaelsproul Nov 20, 2024
5f563ef
Run fetch blobs in parallel with block import
michaelsproul Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions beacon_node/network/src/network_beacon_processor/gossip_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,6 +1444,20 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
}
}

// Block is gossip valid. Attempt to fetch blobs from the EL using versioned hashes derived
// from kzg commitments, without having to wait for all blobs to be sent from the peers.
let publish_blobs = true;
let self_clone = self.clone();
let block_clone = block.clone();
self.executor.spawn(
async move {
self_clone
.fetch_engine_blobs_and_publish(block_clone, block_root, publish_blobs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is running as a task, it's no longer bound by the beacon processor queue. Could someone spam gossip blocks and cause a lot of fetch blobs work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They would need to be beacon blocks with valid signatures, and this is a linear factor, so it can't really blow up much beyond the number of threads allocated to the beacon processor. E.g. if we have 16 threads in the BP, we might end up with 32 running tasks max, which are mostly I/O bound and should be handled just fine by Tokio.

We do this in a few other places, like when we check the payload with the EL:

lighthouse/beacon_node/beacon_chain/src/block_verification.rs

Lines 1407 to 1416 in 6329042

// Spawn the payload verification future as a new task, but don't wait for it to complete.
// The `payload_verification_future` will be awaited later to ensure verification completed
// successfully.
let payload_verification_handle = chain
.task_executor
.spawn_handle(
payload_verification_future,
"execution_payload_verification",
)
.ok_or(BeaconChainError::RuntimeShutdown)?;

.await
},
"fetch_blobs_gossip",
);

let result = self
.chain
.process_block_with_early_caching(
Expand Down Expand Up @@ -1494,13 +1508,6 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
"slot" => slot,
"block_root" => %block_root,
);

// Block is valid, we can now attempt fetching blobs from EL using version hashes
// derived from kzg commitments from the block, without having to wait for all blobs
// to be sent from the peers if we already have them.
let publish_blobs = true;
self.fetch_engine_blobs_and_publish(block.clone(), *block_root, publish_blobs)
.await;
}
Err(BlockError::ParentUnknown { .. }) => {
// This should not occur. It should be checked by `should_forward_block`.
Expand Down
Loading