Skip to content

Send byrange responses in the correct requested range #7611

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

Merged
merged 1 commit into from
Jun 23, 2025
Merged

Send byrange responses in the correct requested range #7611

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 26 additions & 14 deletions beacon_node/network/src/network_beacon_processor/rpc_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,12 +945,18 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
match self.chain.get_blobs(&root) {
Ok(blob_sidecar_list) => {
for blob_sidecar in blob_sidecar_list.iter() {
blobs_sent += 1;
self.send_network_message(NetworkMessage::SendResponse {
peer_id,
inbound_request_id,
response: Response::BlobsByRange(Some(blob_sidecar.clone())),
});
// Due to skip slots, blobs could be out of the range, we ensure they
// are in the range before sending
if blob_sidecar.slot() >= request_start_slot
&& blob_sidecar.slot() < request_start_slot + req.count
{
blobs_sent += 1;
Copy link
Member

@michaelsproul michaelsproul Jun 23, 2025

Choose a reason for hiding this comment

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

Related, the debug message we log about blobs_sent has always seemed wrong to me. In the case of skipped slots it's impossible to achieve blocks_sent == req_count, but the message we log kind of smells like an error:

lighthouse/beacon_node/network/src/network_beacon_processor/rpc_methods.rs

Lines 631 to 639 in 0d311df

debug!(
%peer_id,
msg = "Failed to return all requested blocks",
start_slot = %req_start_slot,
%current_slot,
requested = req_count,
returned = blocks_sent,
"BlocksByRange outgoing response processed"
);

I wonder if we should get rid of that msg seeing as it's kind of misleading.

self.send_network_message(NetworkMessage::SendResponse {
peer_id,
inbound_request_id,
response: Response::BlobsByRange(Some(blob_sidecar.clone())),
});
}
}
}
Err(e) => {
Expand Down Expand Up @@ -1058,14 +1064,20 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
for index in &req.columns {
match self.chain.get_data_column(&root, index) {
Ok(Some(data_column_sidecar)) => {
data_columns_sent += 1;
self.send_network_message(NetworkMessage::SendResponse {
peer_id,
inbound_request_id,
response: Response::DataColumnsByRange(Some(
data_column_sidecar.clone(),
)),
});
// Due to skip slots, data columns could be out of the range, we ensure they
// are in the range before sending
if data_column_sidecar.slot() >= request_start_slot
&& data_column_sidecar.slot() < request_start_slot + req.count
{
data_columns_sent += 1;
self.send_network_message(NetworkMessage::SendResponse {
peer_id,
inbound_request_id,
response: Response::DataColumnsByRange(Some(
data_column_sidecar.clone(),
)),
});
}
}
Ok(None) => {} // no-op
Err(e) => {
Expand Down