p2p: add random nodes as discovery source in setupDiscovery

[islishude]

I'm not sure if it's correct, but if there is no this line for v5 only node, the node won't have any peers.

[fjl]

We do add discv5 into the dialer as a source, but not in the same way as v4. Since discv5 has full ENR support (i.e. all nodes are exchanged as ENRs), we can actually pre-filter the results and only try connecting to nodes which are known to support the eth protocol on the correct chain. We add the discv5 iterator as a protocol-specific peer source here:

go-ethereum/eth/backend.go

Line 410 in d103f17

s.discmix.AddSource(iter)

The dialer should be pulling nodes from that, which should trigger a random walk. If that's not happening, we need to investigate why.

[islishude]

pr #30302 added NewNodeFilter, and it checks if the enode has eth ENRKey

// ENRKey implements enr.Entry.
func (e enrEntry) ENRKey() string {
	return "eth"
}

But how can I add the ENRKey to the bootstrap nodes?

devp2p doesn't have the implementation either

go-ethereum/cmd/devp2p/keycmd.go

Lines 128 to 139 in d103f17

	func makeRecord(ctx *cli.Context) (*enode.Node, error) {
	if ctx.NArg() != 1 {
	return nil, errors.New("need key file as argument")
	}
	var (
	file = ctx.Args().Get(0)
	host = ctx.String(hostFlag.Name)
	tcp = ctx.Int(tcpPortFlag.Name)
	udp = ctx.Int(udpPortFlag.Name)
	)
	key, err := crypto.LoadECDSA(file)

[islishude]

Because of NewNodeFilter, v5 only node can't add any bootstrap nodes to peer list.

I think geth should remove it.

[fjl]

The "eth" ENR key is added when the node is actually running the "eth" p2p protocol. You can see how it's added in setupDiscovery:

go-ethereum/eth/backend.go

Line 385 in d103f17

eth.StartENRUpdater(s.blockchain, s.p2pServer.LocalNode())

If a bootstrap node is run with cmd/devp2p discv5 listen, it can't be added as a peer because it doesn't run the TCP protocol, just discovery. And it will be used by discovery to find other nodes.

[fjl]

If you want to add a peer to Geth permanently, it is best to use the admin_addPeer RPC method at runtime or the P2P.StaticPeers option in the TOML config file.

[fjl]

I guess what you are talking about with the devp2p key command is pre-creating the bootstrap node ENR. When you do that, Geth will connect to the bootstrap node on discovery and fetch its latest ENR. That record should also include the "eth" key, and when it does, the node will be used as a dial candidate.

[islishude]

I have a simple example to replicate the bug.

Add logs

diff --git a/eth/protocols/eth/discovery.go b/eth/protocols/eth/discovery.go
index 075a85b69..580c3b7ba 100644
--- a/eth/protocols/eth/discovery.go
+++ b/eth/protocols/eth/discovery.go
@@ -19,6 +19,7 @@ package eth
 import (
        "github.com/ethereum/go-ethereum/core"
        "github.com/ethereum/go-ethereum/core/forkid"
+       "github.com/ethereum/go-ethereum/log"
        "github.com/ethereum/go-ethereum/p2p/enode"
        "github.com/ethereum/go-ethereum/rlp"
 )
@@ -72,6 +73,9 @@ 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 {
+                       if n.IP().IsLoopback() {
+                               log.Error("Failed to load ENR entry", "node", n.ID(), "ip", n.IP().String(), "err", err)
+                       }
                        return false
                }
                err := filter(entry.ForkID)

Start the first node

$ ./build/bin/geth init --datadir ./tmp/node1 tmp/genesis.json
$ ./build/bin/geth --datadir ./tmp/node1 --discovery.v4=false --port 30303
INFO [02-25|13:56:14.814] New local node record                    seq=1,740,461,693,194 id=d6f17090a0b9467b ip=127.0.0.1 udp=30303 tcp=30303
INFO [02-25|13:56:14.814] Started P2P networking                   self=enode://d5b44c982f699e26c9ef399c016671e7a77d8bde2a811f9cf21de1e686a9b092d514e5b2283020af31efac2c878b81ddc89e7ae1a718bfc4efc003bad1624d93@127.0.0.1:30303

Start the second node

$ ./build/bin/geth init --datadir ./tmp/node2 tmp/genesis.json
$ ./build/bin/geth --datadir ./tmp/node2 --discovery.v4=false --authrpc.port=8552 --port 30304 --bootnodes `./build/bin/devp2p key to-enr -ip 127.0.0.1 -udp 30303 -tcp 30303 ./tmp/node1/geth/nodekey`
ERROR[02-25|13:56:23.756] Failed to load ENR entry                 node=d6f17090a0b9467b ip=127.0.0.1 err="missing ENR key \"eth\""

You can see that the node2 can't connect to bootstrap node1.

[fjl]

Yes, like I said, it doesn't get the correct node right away. However, the node should be tried eventually.
On the bootnode, a log line with text "New local node record" should be printed shortly after startup. This indicates the local record was changed. If you connect to the node console, you can check it with admin.nodeInfo. The enr contained there should have an "eth" key. After a while, the dialing node should pull this ENR from the bootstrap node. This can be observed at loglevel >= 4 (--vmodule=p2p/discover=4), it should print a log like "Node revalidated" and the node ID of the bootstrap node.

[islishude]

It doesn't work

node1

DEBUG[02-25|17:12:43.639] Node revalidated                         b=15 id=31d9163dc5e1aa2c checks=100 q=slow

> admin.nodeInfo.enr
"enr:-KO4QFaoojfMSw5Rcqa3ttKHi7Ei5YImGGBeiKFEO-AyRse-DfMZG70129bqt5vXsAkY0EmP9tfQyc4h1zEnhGhoGeiGAZU8TzTsg2V0aMfGhAZiba2AgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQL13dBBwRltcMjXCL0sur0-yFHDORx4rcE112v-DZOe-oRzbmFwwIN0Y3CCdl-DdWRwgnZf"
> admin.peers
[]
> 

node2

DEBUG[02-25|17:12:49.383] Node revalidated                         b=15 id=539f0709d22ca1c1 checks=96 q=slow

> admin.nodeInfo.enr
"enr:-KO4QFOMpG3EjCdcu5iFIg1TNshh7qf77HHi0T75vZWzFzUgSUqBfc-g-I5Wmq4v1Eph_ohZw4UDQupDFYrYacCmiSKGAZU8T0X5g2V0aMfGhAZiba2AgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQK5IQfyxytqp1Jp2t8AUjpu0zc1Kl4YuCN8ex5IV7TNNYRzbmFwwIN0Y3CCdmCDdWRwgnZg"
> 
> 
> admin.peers
[]

they both have the valid eth enrKey, but node2 can't connect to node1

[islishude]

Well. I found the bug.

ENR key "eth": rlp: interface given to Decode must be a pointer

it didn't use pointer.

var entry enrEntry
		if err := n.Load(&entry); err != nil {
			return false
		}