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

les: switch to new discv5 #21940

Merged
merged 10 commits into from
Jan 26, 2021
Merged

les: switch to new discv5 #21940

merged 10 commits into from
Jan 26, 2021

Conversation

zsfelfoldi
Copy link
Contributor

@zsfelfoldi zsfelfoldi commented Dec 1, 2020

This PR enables running the new discv5 protocol in both LES client and server mode. In client mode it mixes discv5 and dnsdisc iterators (if both are enabled) and filters incoming ENRs for "les" tag and fork ID.
The old discv5 package and all references to it are removed.

les/enr_entry.go Outdated
client := dnsdisc.NewClient(dnsdisc.Config{})
return client.NewIterator(eth.config.DiscoveryURLs...)
forkFilter := forkid.NewFilter(eth.blockchain)
return enode.Filter(it, func(n *enode.Node) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's nice to filter the "non-les-server" here before establishing the TCP connection.
However, I don't think it's the proper place to check the forkID. It's better to check in the
handshake because we can check the nodes found by discovery and the nodes connect
us by itself(although I'm not sure it's realistic).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a filter for discovery nodes. Checking forkID at handshake is a different story, you have an open PR to do that (and it was probably my mistake that it was delayed for so long). We can add that to the new (minimal) les4 upgrade.
Discv5 nodes are totally random and need to be filtered here because we don't want to TCP dial everyone. Nodes coming from DNS are pre-filtered by the crawler but it does not hurt to check them again here.

les/client.go Outdated Show resolved Hide resolved
les/client.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member

I can't review the p2p part meaningfully, but the les part looks good to me in general. Please fix a few nitpicks

@rjl493456442
Copy link
Member

# github.com/ethereum/go-ethereum/cmd/faucet
cmd/faucet/faucet.go:233:4: cannot use enodes (type []*discv5.Node) as type []*enode.Node in field value

Please fix it

@rjl493456442
Copy link
Member

Would be nice to enable it for the les server and have a try. Run this PR in client mode, nothing found :P Because no v5 server.

@zsfelfoldi
Copy link
Contributor Author

Now the discovery seems to work properly, my client did find my server (takes a long time though to find a single specific node because there are several thousand non-LES nodes on discv5). Hopefully the ratio will get better eventually.

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.

4 participants