Skip to content

eth/downloader: skip nil peer in GetHeader #32369

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

Merged

Conversation

Forostovec
Copy link
Contributor

The GetHeader function was incorrectly returning an error when encountering
nil peers in the peers list, which contradicted the comment "keep retrying
if none are yet available".

Changed the logic to skip nil peers with 'continue' instead of returning
an error, allowing the function to properly iterate through all available
peers and attempt to retrieve the target header from each valid peer.

This ensures the function behaves as intended - trying all available peers
before giving up, rather than failing on the first nil peer encountered.

Copy link

@bhaumikmaan bhaumikmaan left a comment

Choose a reason for hiding this comment

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

Would be better if we log out the case rather than directly continuing for feedback

@fjl fjl self-assigned this Aug 7, 2025
@@ -52,7 +52,7 @@ func (d *Downloader) GetHeader(hash common.Hash) (*types.Header, error) {

for _, peer := range d.peers.peers {
if peer == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a warning level log here, indicating the nil peer is encountered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please add a warning level log here, indicating the nil peer is encountered.

Yes, done

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe bit late for this comment, but I'm not too happy about the log message. Log messages are for users, but this warning is not something the user has any chance of fixing. I honestly don't know when a nil peer will even be encountered here. So it'd be good to research why this condition can happen, and fix it, instead of printing a weird warning.

Copy link

@bhaumikmaan bhaumikmaan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

SGTM

@rjl493456442 rjl493456442 changed the title fix: skip nil peers in GetHeader instead of returning error eth/downloader: skip nil peer in GetHeader Aug 11, 2025
@rjl493456442 rjl493456442 added this to the 1.16.3 milestone Aug 11, 2025
@rjl493456442 rjl493456442 merged commit 55a471e into ethereum:master Aug 11, 2025
1 of 2 checks passed
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.

5 participants