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: dial nodes from discv5 #30302

Merged
merged 6 commits into from
Aug 15, 2024
Merged

eth: dial nodes from discv5 #30302

merged 6 commits into from
Aug 15, 2024

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Aug 14, 2024

Here I am adding a discv5 nodes source into the p2p dial iterator. It's an improved version of #29533.

Unlike discv4, the discv5 random nodes iterator will always provide full ENRs. This means we can apply filtering to the results and will only try dialing nodes which explictly opt into the eth protocol with a matching chain.

I have also removed the dial iterator from snap. We don't have an official DNS list for snap anymore, and I doubt anyone else is running one. While we could potentially filter for snap on discv5, there will be very few nodes announcing it, and the extra iterator would just stall the dialer.

Unlike discv4, the discv5 random nodes iterator will always provide full ENRs. This means
we can apply filtering to the results and will only try dialing nodes which explictly opt
into the eth protocol with a matching chain.

I have also removed the dial iterator from snap. We don't have an official DNS list for
snap anymore, and I doubt anyone else is running one. While we could potentially filter
for snap on discv5, there will be very few nodes announcing it, and the extra iterator
would just stall the dialer.
eth/backend.go Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

Generally looks good to me, but could explain the circular dependency you described in #29533? Were you referring to way the PR used RandomNodes() right after starting the discovering, which would block until the table was finished initializing? Or something else?

@fjl
Copy link
Contributor Author

fjl commented Aug 15, 2024

About the circular dependency: Ultimately, the nodes iterator has to end up in p2p.Protocol, which we then register in the node.Node object. However, we can only register protocols before the node is started, because the final list will be entered into p2p.Config, which cannot be changed after starting the server.

Previously, we only used the DNS list iterator as a per-protocol discovery source. We can create this iterator before starting the node because it is only dependent on the list of URLs which are provided in ethconfig.Config.

However, to call RandomNodes, we need access to the discovery instance, which is a part of p2p.Server. This is where the circular dependency is.

I have resolved the dependency here by putting an empty FairMix into the p2p.Protocol. We then add sources into the mixer after the node is started, when p2p.Server becomes available.

@lightclient
Copy link
Member

Okay thanks I see, now the Node is started first which later triggers the start for all the lifecycles, which eventually calls setupDiscovery(). Before the iterators came in New(..) which would be called before the node is started.

@lightclient
Copy link
Member

lightclient commented Aug 15, 2024

A full integration test of drawing devp2p peers from discv5 would be good, maybe that's a job for hive?

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Ran locally and am at least able to find peers with DNS / discv4. Probably best to start rolling this out and go from there.

@fjl
Copy link
Contributor Author

fjl commented Aug 15, 2024

You can test like this:

modified   eth/protocols/eth/discovery.go
@@ -17,6 +17,8 @@
 package eth
 
 import (
+	"fmt"
+
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/forkid"
 	"github.com/ethereum/go-ethereum/p2p/enode"
@@ -72,9 +74,15 @@ func NewNodeFilter(chain *core.BlockChain) func(*enode.Node) bool {
 	return func(n *enode.Node) bool {
 		var entry enrEntry
 		if err := n.Load(entry); err != nil {
+			fmt.Println("no eth", n.ID())
 			return false
 		}
 		err := filter(entry.ForkID)
+		if err != nil {
+			fmt.Println("wrong chain", n.ID())
+		} else {
+			fmt.Println("match!", n.ID())
+		}
 		return err == nil
 	}
 }
$ ./geth --vmodule=p2p=5 --discovery.v5=true --discovery.v4=false --discovery.dns="" 

If it ever prints match! you found a node. But it will take a long time.

@fjl fjl added this to the 1.14.9 milestone Aug 15, 2024
@fjl fjl merged commit 6eb42a6 into ethereum:master Aug 15, 2024
3 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.

2 participants