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.

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

eth/downloader: move the pivot in beacon sync mode too #26453

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jan 9, 2023

Fixes #26429.

In legacy (pre-merge) sync mode, headers were contiguously downloaded from the network and when no more headers were available, we checked every few seconds whether there are 64 new blocks to move the pivot.

In beacon (post-merge) sync mode, we don't need to check for new skeleton headers non stop, since those re delivered one by one by the engine API. The missing code snippet from the header fetcher was to actually look at the latest head and move the pivot if it was more than 2*64-8 away. This PR adds the missing movement logic.

The convoluted logic of pulling headers from disk if they don't exist in the skeleton chain shouldn't really be needed, but I've added it as a sanity check in case I'm missing some reasoning now. It should result in a warning anyway so we can investigate if that clause is actually triggered.

@karalabe karalabe added this to the 1.11.0 milestone Jan 9, 2023
@karalabe
Copy link
Member Author

karalabe commented Jan 9, 2023

Master branch:

This PR:

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM!

@holiman
Copy link
Contributor

holiman commented Jan 10, 2023

This PR

Some charts
Screenshot 2023-01-10 at 09-06-08 Single Geth - Grafana

This is this PR. IT has a fairly stable ingress rate for the duration of the sync.

Master

Here's master:
Screenshot 2023-01-10 at 09-06-41 Single Geth - Grafana

As you can see, the snap traffic comes if fits and bursts - not quite like the report in #26429 , where it's totally dead for the duration of the block downloads. The reason for this is this change in factor , which made factor check new heads less often. Because of this, there's a higher chance that factor misses a head, and thus that geth notices a gapped beaconchain. When geth detects a beacon gap, the sync is restarted.

And restarting the sync is similar to moving the pivot.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman merged commit 452a12a into ethereum:master Jan 10, 2023
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
In legacy (pre-merge) sync mode, headers were contiguously downloaded from the network and when no more headers were available, we checked every few seconds whether there are 64 new blocks to move the pivot.

In beacon (post-merge) sync mode, we don't need to check for new skeleton headers non stop, since those re delivered one by one by the engine API. The missing code snippet from the header fetcher was to actually look at the latest head and move the pivot if it was more than 2*64-8 away. This PR adds the missing movement logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No pivot movements during block download
3 participants