Skip to content

feat: add debug.setSyncTarget console command (#31375) #31665

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

demoncoder-crypto
Copy link

No description provided.

@MariusVanDerWijden
Copy link
Member

Have you even tried this once?
It's not working like this...
I will push an update to this which makes it work

log.Error("Could not get downloader")
}
// retry 20 times to retrieve the header from random peers
for range 20 {
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this tbh, open for suggestions here

Copy link
Author

Choose a reason for hiding this comment

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

Do you need the complete change here, please do let me know I am more than happy to work on it, Thanks for the reply .

@demoncoder-crypto
Copy link
Author

Have you even tried this once?
It's not working like this...
I will push an update to this which makes it work, @MariusVanDerWijden I have not tested it at all I just wanted to see if my given direction is correct, so I can iteratively fix and work upon the solution. I am sorry for the mess but thought rather than making a faulty implementation right I should ask for general guidance first and then proceed to make it work as this is my first time working with Ethereum.

@demoncoder-crypto
Copy link
Author

So any suggestions on how to proceed with this, if my track is right should I proceed to improve this implementation?

@MariusVanDerWijden
Copy link
Member

I'm currently assigned to this, but I rewrote it, so someone else should also take a look, maybe @jwasinger
It does change something fundamental: now we try to request a head from the network during the forkchoice. If we get that, we will try to sync to it. This makes it possible for us to start a sync even if we have never seen a newPayload. Its something that CLs wanted for a long time

@jwasinger
Copy link
Contributor

Sure, I'll take a look.

@jwasinger jwasinger self-assigned this May 29, 2025
@jwasinger
Copy link
Contributor

jwasinger commented May 29, 2025

This makes it possible for us to start a sync even if we have never seen a newPayload. Its something that CLs wanted for a long time.

What's the rationale behind this? Presumably this is so that the CL can initiate the EL sync while they are syncing. But if they already know the sync target, what's stopping them from send us a payload (before they are synced, to kick-off the sync on our end).

we can better react to scenarios of long non-finality where we have to pick a chain to sync to

Same question. This ought to be a feature on the CL given the leader-follower relationship between the CL and EL.

I'm guessing the answer is that we don't control the development on various CLs, and so it's convenient to have this feature on Geth to save some sync time.

@MariusVanDerWijden
Copy link
Member

Split into two, try all peers in downloader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants