Oneeman/cherry pick discovery bootnode fix (ethereum#1194)

* p2p/discover: add initial discovery v5 implementation (ethereum#20750)

celo-blockchain cherry-pick conflicts:
  cmd/devp2p/discv4cmd.go
  p2p/discover/common.go
  p2p/discover/v4_udp.go

This adds an implementation of the current discovery v5 spec.

There is full integration with cmd/devp2p and enode.Iterator in this
version. In theory we could enable the new protocol as a replacement of
discovery v4 at any time. In practice, there will likely be a few more
changes to the spec and implementation before this can happen.

* p2p/discover: move discv4 encoding to new 'v4wire' package (ethereum#21147)

celo-blockchain cherry-pick conflicts:
  p2p/discover/v4_udp.go
  p2p/discover/v4_udp_test.go

This moves all v4 protocol definitions to a new package, p2p/discover/v4wire.
The new package will be used for low-level protocol tests.

* p2p/discover: avoid dropping unverified nodes when table is almost empty (ethereum#21396)

This change improves discovery behavior in small networks. Very small
networks would often fail to bootstrap because all member nodes were
dropping table content due to findnode failure. The check is now changed
to avoid dropping nodes on findnode failure when their bucket is almost
empty. It also relaxes the liveness check requirement for FINDNODE/v4
response nodes, returning unverified nodes as results when there aren't
any verified nodes yet.

The "findnode failed" log now reports whether the node was dropped
instead of the number of results. The value of the "results" was
always zero by definition.

Co-authored-by: Felix Lange <[email protected]>

* Don't remove the last node from a discovery table bucket

If a node initially has trouble reaching the bootnode, the bootnode was being removed in doRevalidate(), leaving the table empty.

* Take care of lint errors

Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: timcooijmans <[email protected]>

Author: 3 people

cmd/devp2p/crawl.go

@@ -20,14 +20,13 @@ import (
 	"time"
 
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/p2p/discover"
 	"github.com/ethereum/go-ethereum/p2p/enode"
 )
 
 type crawler struct {
 	input     nodeSet
 	output    nodeSet
-	disc      *discover.UDPv4
+	disc      resolver
 	iters     []enode.Iterator
 	inputIter enode.Iterator
 	ch        chan *enode.Node
@@ -37,7 +36,11 @@ type crawler struct {
 	revalidateInterval time.Duration
 }
 
-func newCrawler(input nodeSet, disc *discover.UDPv4, iters ...enode.Iterator) *crawler {
+type resolver interface {
+	RequestENR(*enode.Node) (*enode.Node, error)
+}
+
+func newCrawler(input nodeSet, disc resolver, iters ...enode.Iterator) *crawler {
 	c := &crawler{
 		input:     input,
 		output:    make(nodeSet, len(input)),

cmd/devp2p/discv4cmd.go

@@ -84,6 +84,18 @@ var (
 		Name:  "bootnodes",
 		Usage: "Comma separated nodes used for bootstrapping",
 	}
+	nodekeyFlag = cli.StringFlag{
+		Name:  "nodekey",
+		Usage: "Hex-encoded node key",
+	}
+	nodedbFlag = cli.StringFlag{
+		Name:  "nodedb",
+		Usage: "Nodes database location",
+	}
+	listenAddrFlag = cli.StringFlag{
+		Name:  "addr",
+		Usage: "Listening address",
+	}
 	crawlTimeoutFlag = cli.DurationFlag{
 		Name:  "timeout",
 		Usage: "Time limit for the crawl.",
@@ -181,56 +193,75 @@ func discv4Crawl(ctx *cli.Context) error {
 	return nil
 }
 
-func parseBootnodes(ctx *cli.Context) ([]*enode.Node, error) {
-	s := utils.GetBootstrapNodes(ctx)
-	if ctx.IsSet(bootnodesFlag.Name) {
-		s = strings.Split(ctx.String(bootnodesFlag.Name), ",")
-	}
-	nodes := make([]*enode.Node, len(s))
-	var err error
-	for i, record := range s {
-		nodes[i], err = parseNode(record)
-		if err != nil {
-			return nil, fmt.Errorf("invalid bootstrap node: %v", err)
-		}
-	}
-	return nodes, nil
-}
-
 // startV4 starts an ephemeral discovery V4 node.
 func startV4(ctx *cli.Context) *discover.UDPv4 {
 	networkId := ctx.GlobalUint64(networkIdFlag.Name)
-	socket, ln, cfg, err := listen(networkId)
+	ln, config := makeDiscoveryConfig(ctx, networkId)
+	socket := listen(ln, ctx.String(listenAddrFlag.Name))
+	disc, err := discover.ListenV4(socket, ln, config)
 	if err != nil {
 		exit(err)
 	}
+	return disc
+}
+
+func makeDiscoveryConfig(ctx *cli.Context, networkId uint64) (*enode.LocalNode, discover.Config) {
+	var cfg discover.Config
+
+	if ctx.IsSet(nodekeyFlag.Name) {
+		key, err := crypto.HexToECDSA(ctx.String(nodekeyFlag.Name))
+		if err != nil {
+			exit(fmt.Errorf("-%s: %v", nodekeyFlag.Name, err))
+		}
+		cfg.PrivateKey = key
+	} else {
+		cfg.PrivateKey, _ = crypto.GenerateKey()
+	}
+
 	if commandHasFlag(ctx, bootnodesFlag) {
 		bn, err := parseBootnodes(ctx)
 		if err != nil {
 			exit(err)
 		}
 		cfg.Bootnodes = bn
 	}
-	disc, err := discover.ListenV4(socket, ln, cfg)
+
+	dbpath := ctx.String(nodedbFlag.Name)
+	db, err := enode.OpenDB(dbpath)
 	if err != nil {
 		exit(err)
 	}
-	return disc
-}
-
-func listen(networkId uint64) (*net.UDPConn, *enode.LocalNode, discover.Config, error) {
-	var cfg discover.Config
-	cfg.PrivateKey, _ = crypto.GenerateKey()
-	db, _ := enode.OpenDB("")
 	ln := enode.NewLocalNode(db, cfg.PrivateKey, networkId)
+	return ln, cfg
+}
 
-	socket, err := net.ListenUDP("udp4", &net.UDPAddr{IP: net.IP{0, 0, 0, 0}})
+func listen(ln *enode.LocalNode, addr string) *net.UDPConn {
+	if addr == "" {
+		addr = "0.0.0.0:0"
+	}
+	socket, err := net.ListenPacket("udp4", addr)
 	if err != nil {
-		db.Close()
-		return nil, nil, cfg, err
+		exit(err)
 	}
-	addr := socket.LocalAddr().(*net.UDPAddr)
+	usocket := socket.(*net.UDPConn)
+	uaddr := socket.LocalAddr().(*net.UDPAddr)
 	ln.SetFallbackIP(net.IP{127, 0, 0, 1})
-	ln.SetFallbackUDP(addr.Port)
-	return socket, ln, cfg, nil
+	ln.SetFallbackUDP(uaddr.Port)
+	return usocket
+}
+
+func parseBootnodes(ctx *cli.Context) ([]*enode.Node, error) {
+	s := utils.GetBootstrapNodes(ctx)
+	if ctx.IsSet(bootnodesFlag.Name) {
+		s = strings.Split(ctx.String(bootnodesFlag.Name), ",")
+	}
+	nodes := make([]*enode.Node, len(s))
+	var err error
+	for i, record := range s {
+		nodes[i], err = parseNode(record)
+		if err != nil {
+			return nil, fmt.Errorf("invalid bootstrap node: %v", err)
+		}
+	}
+	return nodes, nil
 }

cmd/devp2p/discv5cmd.go

@@ -0,0 +1,124 @@
+// Copyright 2019 The go-ethereum Authors
+// This file is part of go-ethereum.
+//
+// go-ethereum is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// go-ethereum is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with go-ethereum. If not, see <http://www.gnu.org/licenses/>.
+
+package main
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/p2p/discover"
+	"gopkg.in/urfave/cli.v1"
+)
+
+var (
+	discv5Command = cli.Command{
+		Name:  "discv5",
+		Usage: "Node Discovery v5 tools",
+		Subcommands: []cli.Command{
+			discv5PingCommand,
+			discv5ResolveCommand,
+			discv5CrawlCommand,
+			discv5ListenCommand,
+		},
+	}
+	discv5PingCommand = cli.Command{
+		Name:   "ping",
+		Usage:  "Sends ping to a node",
+		Action: discv5Ping,
+	}
+	discv5ResolveCommand = cli.Command{
+		Name:   "resolve",
+		Usage:  "Finds a node in the DHT",
+		Action: discv5Resolve,
+		Flags:  []cli.Flag{bootnodesFlag},
+	}
+	discv5CrawlCommand = cli.Command{
+		Name:   "crawl",
+		Usage:  "Updates a nodes.json file with random nodes found in the DHT",
+		Action: discv5Crawl,
+		Flags:  []cli.Flag{bootnodesFlag, crawlTimeoutFlag},
+	}
+	discv5ListenCommand = cli.Command{
+		Name:   "listen",
+		Usage:  "Runs a node",
+		Action: discv5Listen,
+		Flags: []cli.Flag{
+			bootnodesFlag,
+			nodekeyFlag,
+			nodedbFlag,
+			listenAddrFlag,
+		},
+	}
+)
+
+func discv5Ping(ctx *cli.Context) error {
+	n := getNodeArg(ctx)
+	disc := startV5(ctx)
+	defer disc.Close()
+
+	fmt.Println(disc.Ping(n))
+	return nil
+}
+
+func discv5Resolve(ctx *cli.Context) error {
+	n := getNodeArg(ctx)
+	disc := startV5(ctx)
+	defer disc.Close()
+
+	fmt.Println(disc.Resolve(n))
+	return nil
+}
+
+func discv5Crawl(ctx *cli.Context) error {
+	if ctx.NArg() < 1 {
+		return fmt.Errorf("need nodes file as argument")
+	}
+	nodesFile := ctx.Args().First()
+	var inputSet nodeSet
+	if common.FileExist(nodesFile) {
+		inputSet = loadNodesJSON(nodesFile)
+	}
+
+	disc := startV5(ctx)
+	defer disc.Close()
+	c := newCrawler(inputSet, disc, disc.RandomNodes())
+	c.revalidateInterval = 10 * time.Minute
+	output := c.run(ctx.Duration(crawlTimeoutFlag.Name))
+	writeNodesJSON(nodesFile, output)
+	return nil
+}
+
+func discv5Listen(ctx *cli.Context) error {
+	disc := startV5(ctx)
+	defer disc.Close()
+
+	fmt.Println(disc.Self())
+	select {}
+}
+
+// startV5 starts an ephemeral discovery v5 node.
+func startV5(ctx *cli.Context) *discover.UDPv5 {
+	networkId := ctx.GlobalUint64(networkIdFlag.Name)
+	ln, config := makeDiscoveryConfig(ctx, networkId)
+	socket := listen(ln, ctx.String(listenAddrFlag.Name))
+	disc, err := discover.ListenV5(socket, ln, config)
+	if err != nil {
+		exit(err)
+	}
+	return disc
+}

cmd/devp2p/main.go

@@ -59,6 +59,7 @@ func init() {
 	app.Commands = []cli.Command{
 		enrdumpCommand,
 		discv4Command,
+		discv5Command,
 		dnsCommand,
 		nodesetCommand,
 	}

p2p/discover/common.go

@@ -20,8 +20,10 @@ import (
 	"crypto/ecdsa"
 	"net"
 
+	"github.com/ethereum/go-ethereum/common/mclock"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/p2p/enode"
+	"github.com/ethereum/go-ethereum/p2p/enr"
 	"github.com/ethereum/go-ethereum/p2p/netutil"
 )
 
@@ -44,6 +46,21 @@ type Config struct {
 	Unhandled        chan<- ReadPacket // unhandled packets are sent on this channel
 	Log              log.Logger        // if set, log messages go here
 	PingIPFromPacket bool
+	ValidSchemes     enr.IdentityScheme // allowed identity schemes
+	Clock            mclock.Clock
+}
+
+func (cfg Config) withDefaults() Config {
+	if cfg.Log == nil {
+		cfg.Log = log.Root()
+	}
+	if cfg.ValidSchemes == nil {
+		cfg.ValidSchemes = enode.ValidSchemes
+	}
+	if cfg.Clock == nil {
+		cfg.Clock = mclock.System{}
+	}
+	return cfg
 }
 
 // ListenUDP starts listening for discovery packets on the given UDP socket.
@@ -52,8 +69,15 @@ func ListenUDP(c UDPConn, ln *enode.LocalNode, cfg Config) (*UDPv4, error) {
 }
 
 // ReadPacket is a packet that couldn't be handled. Those packets are sent to the unhandled
-// channel if configured. This is exported for internal use, do not use this type.
+// channel if configured.
 type ReadPacket struct {
 	Data []byte
 	Addr *net.UDPAddr
 }
+
+func min(x, y int) int {
+	if x > y {
+		return y
+	}
+	return x
+}

p2p/discover/lookup.go

@@ -104,9 +104,7 @@ func (it *lookup) startQueries() bool {
 
 	// The first query returns nodes from the local table.
 	if it.queries == -1 {
-		it.tab.mutex.Lock()
-		closest := it.tab.closest(it.result.target, bucketSize, false)
-		it.tab.mutex.Unlock()
+		closest := it.tab.findnodeByID(it.result.target, bucketSize, false)
 		// Avoid finishing the lookup too quickly if table is empty. It'd be better to wait
 		// for the table to fill in this case, but there is no good mechanism for that
 		// yet.
@@ -150,11 +148,14 @@ func (it *lookup) query(n *node, reply chan<- []*node) {
 	} else if len(r) == 0 {
 		fails++
 		it.tab.db.UpdateFindFails(n.ID(), n.IP(), fails)
-		it.tab.log.Trace("Findnode failed", "id", n.ID(), "failcount", fails, "err", err)
-		if fails >= maxFindnodeFailures {
-			it.tab.log.Trace("Too many findnode failures, dropping", "id", n.ID(), "failcount", fails)
+		// Remove the node from the local table if it fails to return anything useful too
+		// many times, but only if there are enough other nodes in the bucket.
+		dropped := false
+		if fails >= maxFindnodeFailures && it.tab.bucketLen(n.ID()) >= bucketSize/2 {
+			dropped = true
 			it.tab.delete(n)
 		}
+		it.tab.log.Trace("FINDNODE failed", "id", n.ID(), "failcount", fails, "dropped", dropped, "err", err)
 	} else if fails > 0 {
 		// Reset failure counter because it counts _consecutive_ failures.
 		it.tab.db.UpdateFindFails(n.ID(), n.IP(), 0)