Skip to content

Fix leak in range sync components_by_range_requests #7767

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

Closed
wants to merge 4 commits into from
Closed

Fix leak in range sync components_by_range_requests #7767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9181ff4
Fix `components_by_range_requests` leak.
jimmygchen Jul 21, 2025
bf976d3
Merge branch 'unstable' into fix-sync-leak
jimmygchen Jul 21, 2025
790c94c
Try a different hack
jimmygchen Jul 21, 2025
74f8faf
Merge branch 'fix-sync-leak' of github.com:jimmygchen/lighthouse into…
jimmygchen Jul 21, 2025
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
6 changes: 5 additions & 1 deletion beacon_node/network/src/sync/block_sidecar_coupling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<E: EthSpec> RangeBlockComponentsRequest<E> {
/// request for some columns.
pub fn reinsert_failed_column_requests(
&mut self,
failed_column_requests: Vec<(DataColumnsByRangeRequestId, Vec<u64>)>,
failed_column_requests: Vec<(DataColumnsByRangeRequestId, Vec<ColumnIndex>)>,
) -> Result<(), String> {
match &mut self.block_data_request {
RangeBlockDataRequest::DataColumns {
Expand Down Expand Up @@ -195,6 +195,9 @@ impl<E: EthSpec> RangeBlockComponentsRequest<E> {
spec,
);

// FIXME: If a peer provided _some_ useful columns but not all, do we end up
// removing the entire requests, including the useful columns? These columns won't
// be in the error and may not be retried?
if let Err(err) = &resp {
if let Some((peers, _)) = &err.column_and_peer {
for (_, peer) in peers.iter() {
Expand Down Expand Up @@ -300,6 +303,7 @@ impl<E: EthSpec> RangeBlockComponentsRequest<E> {
.insert(index, column)
.is_some()
{
// FIXME: we could probably ignore and continue? instead of failing the whole batch?
return Err(CouplingError {
msg: format!("Repeated column block_root {block_root:?} index {index}"),
column_and_peer: None,
Expand Down
15 changes: 15 additions & 0 deletions beacon_node/network/src/sync/network_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,21 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
Ok(columns_to_request_by_peer)
}

/// Remove the range request before attempting a retry.
pub(crate) fn remove_range_request_by_id(&mut self, id: Id) {
if let Some(request_id) = self
.components_by_range_requests
.keys()
.find(|r| r.id == id)
{
let request_id = ComponentsByRangeRequestId {
id,
requester: request_id.requester,
};
self.components_by_range_requests.remove(&request_id);
};
}

/// Received a blocks by range or blobs by range response for a request that couples blocks '
/// and blobs.
pub fn range_block_component_response(
Expand Down
2 changes: 2 additions & 0 deletions beacon_node/network/src/sync/range_sync/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,8 @@ impl<T: BeaconChainTypes> SyncingChain<T> {
failing_batch: batch_id,
});
}
// FIXME: hack to remove range request before a retry
network.remove_range_request_by_id(request_id);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this hack didn't fix it

image

The metric i was looking at is sync_active_network_requests, type == components_by_range, recorded here:

lighthouse/beacon_node/network/src/sync/network_context.rs

Lines 1705 to 1708 in 790c94c

(
"components_by_range",
self.components_by_range_requests.len(),
),

This change what caused entries not to be removed i believe, its used by both range and backfill:

lighthouse/beacon_node/network/src/sync/network_context.rs

Lines 745 to 751 in 790c94c

if blocks_result.is_ok() {
// remove the entry only if it coupled successfully with
// no errors
entry.remove();
}
// If the request is finished, dequeue everything
Some(blocks_result.map_err(RpcResponseError::BlockComponentCouplingError))

self.send_batch(network, batch_id)
} else {
debug!(
Expand Down