Sync cleanups #8230

pawanjay176 · 2025-10-17T01:27:00Z

Issue Addressed

N/A

Proposed Changes

In the batch retry logic, we were failing to set the batch state to AwaitingDownload before attempting a retry. This PR sets it to AwaitingDownload before the retry and sets it back to Downloading if the retry suceeded in sending out a request
Remove all peer scoring logic from retrying and rely on just de priorotizing the failed peer. I finally concede the point to @dapplion 😄
Changes block_components_by_range_request to accept block_peers and column_peers. This is to ensure that we use the full synced peerset for requesting columns in order to avoid splitting the column peers among multiple head chains. During forward sync, we want the block peers to be the peers from the syncing chain and column peers to be all synced peers from the peerdb.
Also, fixes a typo and calls attempt_send_awaiting_download_batches from more places

beacon_node/network/src/sync/backfill_sync/mod.rs

beacon_node/network/src/sync/range_sync/chain.rs

dapplion

Looks good! Just two suggestions to document behaviour

jimmygchen · 2025-10-20T04:04:05Z

beacon_node/network/src/sync/range_sync/chain.rs

                        if !*exceeded_retries {
+                            // Set the batch back to `AwaitingDownload` before retrying.
+                            // This is to ensure that the batch doesn't get stuck in `Downloading` state.
+                            batch.download_failed(None)?;


Should we check the returned outcome here?
What happens if it returns Ok(BatchState::Failed)?

Looking at the code it looks like we may hit this?

lighthouse/beacon_node/network/src/sync/range_sync/batch.rs

Lines 344 to 349 in fb77ce9

other => {

self.state = other;

Err(WrongState(format!(

"Starting download for batch in wrong state {:?}",

self.state

)))

Great catch. I realised I was also incrementing the batch failed count when we shouldn't be as we want to keep the batch retries separate from the column retries in the original retry PR.
Fixed in ac8adb4 . Please take a look

jimmygchen · 2025-10-20T06:48:21Z

beacon_node/network/src/sync/range_sync/chain.rs

+                            //
+                            // DataColumn retries has a retry limit so calling `downloading_to_awaiting_download`
+                            // is safe.
+                            if let BatchOperationOutcome::Failed { blacklist } =


BatchOperationOutcome::Failed is no longer reachable after changing the call to downloading_to_awaiting_download.

Also i think the function name conveys clearly what it does, but not its intended usage, but I also struggle to come up with something better.

batch.download_failed_skip_attempt_count seems a bit too wordy

I think handling BatchOperationOutcome::Failed is a good idea though.

jimmygchen

Looks good to me too! Thanks for the cleanup

mergify · 2025-10-20T07:11:06Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

☑️ local-testnet-success

You can check the last failing draft PR here: #8242.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

jimmygchen · 2025-10-20T07:14:30Z

@mergify requeue

mergify · 2025-10-20T07:14:36Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify · 2025-10-20T07:29:29Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

☑️ local-testnet-success

You can check the last failing draft PR here: #8243.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

jimmygchen · 2025-10-20T08:41:22Z

@mergify requeue

mergify · 2025-10-20T08:41:36Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify · 2025-10-20T08:45:22Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

☑️ local-testnet-success

You can check the last failing draft PR here: #8244.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

jimmygchen · 2025-10-20T10:31:27Z

@mergify requeue

mergify · 2025-10-20T10:31:31Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@dapplion

N/A 1. In the batch retry logic, we were failing to set the batch state to `AwaitingDownload` before attempting a retry. This PR sets it to `AwaitingDownload` before the retry and sets it back to `Downloading` if the retry suceeded in sending out a request 2. Remove all peer scoring logic from retrying and rely on just de priorotizing the failed peer. I finally concede the point to @dapplion 😄 3. Changes `block_components_by_range_request` to accept `block_peers` and `column_peers`. This is to ensure that we use the full synced peerset for requesting columns in order to avoid splitting the column peers among multiple head chains. During forward sync, we want the block peers to be the peers from the syncing chain and column peers to be all synced peers from the peerdb. Also, fixes a typo and calls `attempt_send_awaiting_download_batches` from more places Co-Authored-By: Pawan Dhananjay <[email protected]>

pawanjay176 added 2 commits October 16, 2025 17:55

Fix small sync issues

7c07a81

Remove peer action for ambiguous errors

025c7c3

pawanjay176 requested a review from jxs as a code owner October 17, 2025 01:27

jimmygchen added the syncing label Oct 17, 2025

Request columns from entire peer set

c869717

pawanjay176 added the ready-for-review The code is ready for review label Oct 17, 2025

pawanjay176 requested review from dapplion and jimmygchen October 17, 2025 20:03

dapplion reviewed Oct 19, 2025

View reviewed changes

beacon_node/network/src/sync/backfill_sync/mod.rs Outdated Show resolved Hide resolved

beacon_node/network/src/sync/range_sync/chain.rs Show resolved Hide resolved

dapplion approved these changes Oct 19, 2025

View reviewed changes

Add docs

1c59fb8

jimmygchen reviewed Oct 20, 2025

View reviewed changes

pawanjay176 added 2 commits October 19, 2025 21:38

Do not increment batch failed count on column retry

ac8adb4

Merge branch 'unstable' into sync-cleanups

3c5a13a

jimmygchen added the v8.0.0 Q4 2025 Fusaka Mainnet Release label Oct 20, 2025

jimmygchen reviewed Oct 20, 2025

View reviewed changes

jimmygchen approved these changes Oct 20, 2025

View reviewed changes

jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 20, 2025

mergify bot added the queued label Oct 20, 2025

mergify bot added a commit that referenced this pull request Oct 20, 2025

Merge of #8230

52e5a3c

mergify bot mentioned this pull request Oct 20, 2025

merge queue: embarking unstable (2b30c96), #8228 and #8230 together #8242

Closed

6 tasks

mergify bot removed the queued label Oct 20, 2025

mergify bot added the queued label Oct 20, 2025

mergify bot added a commit that referenced this pull request Oct 20, 2025

Merge of #8230

adc2379

mergify bot mentioned this pull request Oct 20, 2025

merge queue: embarking unstable (c012f46) and #8230 together #8243

Closed

6 tasks

mergify bot removed the queued label Oct 20, 2025

mergify bot added the queued label Oct 20, 2025

mergify bot added a commit that referenced this pull request Oct 20, 2025

Merge of #8230

bee7dba

mergify bot mentioned this pull request Oct 20, 2025

merge queue: embarking unstable (c012f46) and #8230 together #8244

Closed

6 tasks

mergify bot removed the queued label Oct 20, 2025

mergify bot added the queued label Oct 20, 2025

mergify bot added a commit that referenced this pull request Oct 20, 2025

Merge of #8230

66ec5f9

mergify bot mentioned this pull request Oct 20, 2025

merge queue: embarking unstable (c012f46) and #8230 together #8245

Closed

6 tasks

mergify bot merged commit 092aaae into sigp:unstable Oct 20, 2025
38 checks passed

mergify bot removed the queued label Oct 20, 2025

jimmygchen mentioned this pull request Oct 20, 2025

Add failing test for a sync race condition scenario #8039

Open

	other => {
	self.state = other;
	Err(WrongState(format!(
	"Starting download for batch in wrong state {:?}",
	self.state
	)))

Sync cleanups #8230

Sync cleanups #8230

Uh oh!

Conversation

pawanjay176 commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

jimmygchen Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawanjay176 Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 20, 2025

Uh oh!

jimmygchen commented Oct 20, 2025

Uh oh!

mergify bot commented Oct 20, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

mergify bot commented Oct 20, 2025

Uh oh!

jimmygchen commented Oct 20, 2025

Uh oh!

mergify bot commented Oct 20, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

mergify bot commented Oct 20, 2025

Uh oh!

jimmygchen commented Oct 20, 2025

Uh oh!

mergify bot commented Oct 20, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pawanjay176 commented Oct 17, 2025 •

edited

Loading

jimmygchen Oct 20, 2025 •

edited

Loading

pawanjay176 Oct 20, 2025 •

edited

Loading