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

Dialer not use discv5 #31168

Closed
sonhv0212 opened this issue Feb 13, 2025 · 5 comments · Fixed by #31185
Closed

Dialer not use discv5 #31168

sonhv0212 opened this issue Feb 13, 2025 · 5 comments · Fixed by #31185

Comments

@sonhv0212
Copy link

Seem that discv5 source is not used anywhere, the last PR #30302 removes the DialCandidates of the eth protocol that includes discv5 source but that change is not mentioned in the description

@fjl
Copy link
Contributor

fjl commented Feb 13, 2025

WDYM? the linked PR adds discv5 as a nodes source for the dialer!

@sonhv0212
Copy link
Author

But seems that it is not passed to the dialer via DialCandidates, you can check this line https://github.com/ethereum/go-ethereum/pull/30302/files#diff-5beb4052baf94e3025566d929c63974350e4b1eae18cce8eb21b4579d6788ffeL117

@fjl
Copy link
Contributor

fjl commented Feb 13, 2025

I checked and I agree the naming is confusing there. The dnsdisc there is the name of the function parameter of MakeProtocols, and we pass Ethereum.discmix to it. The discv5 iterator is a part of this discmix.

The parameter name should be updated. I just left it as-is from before, but that's confusing.

@sonhv0212
Copy link
Author

yeah, I see the discv5 iterator is added to Ethereum.discmix, but the dnsdisc is not used in eth.MakeProtocols (the previous version is passed to the dialer via DialCandidates, which will be added to p2p.Server.discmix and then dialer use that discmix to try dialing). So, in this case, the discv5 iterator is useless

@fjl
Copy link
Contributor

fjl commented Feb 14, 2025

Damn, you are right! Thank you!

@fjl fjl closed this as completed in d37a0b8 Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants