mirror p2p-discover specs work (#2570)

* opnode/p2p,specs/p2p: discover+connect background process

* opnode/flags: simplify p2p useragent flag

* opnode/p2p: peer discovery background process

* p2p: review cleanup - define consts for numbers, fix imports
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

op-node/flags/p2p_flags.go

@@ -144,7 +144,7 @@ var (
 	}
 	UserAgent = cli.StringFlag{
 		Name:     "p2p.useragent",
-		Usage:    "User-agent string to share via LibP2P identify. If empty it defaults to 'optimism-VERSIONHERE'.",
+		Usage:    "User-agent string to share via LibP2P identify. If empty it defaults to 'optimism'.",
 		Hidden:   true,
 		Required: false,
 		Value:    "optimism",

op-node/node/node.go

@@ -34,7 +34,7 @@ type OpNode struct {
 	l2Engines  []*driver.Driver      // engines to keep synced
 	l2Nodes    []*rpc.Client         // L2 Execution Engines to close at shutdown
 	server     *rpcServer            // RPC server hosting the rollup-node API
-	p2pNode    p2p.Node              // P2P node functionality
+	p2pNode    *p2p.NodeP2P          // P2P node functionality
 	p2pSigner  p2p.Signer            // p2p gogssip application messages will be signed with this signer
 	tracer     Tracer                // tracer to get events for testing/debugging
 
@@ -221,6 +221,9 @@ func (n *OpNode) initP2P(ctx context.Context, cfg *Config) error {
 			return err
 		}
 		n.p2pNode = p2pNode
+		if n.p2pNode.Dv5Udp() != nil {
+			go n.p2pNode.DiscoveryProcess(n.resourcesCtx, n.log, &cfg.Rollup, cfg.P2P.TargetPeers())
+		}
 	}
 	return nil
 }

op-node/p2p/config.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+
 	"github.com/libp2p/go-libp2p-core/peer"
 
 	"github.com/ethereum/go-ethereum/log"
@@ -44,7 +46,8 @@ type SetupP2P interface {
 	// Host creates a libp2p host service. Returns nil, nil if p2p is disabled.
 	Host(log log.Logger) (host.Host, error)
 	// Discovery creates a disc-v5 service. Returns nil, nil, nil if discovery is disabled.
-	Discovery(log log.Logger) (*enode.LocalNode, *discover.UDPv5, error)
+	Discovery(log log.Logger, rollupCfg *rollup.Config, tcpPort uint16) (*enode.LocalNode, *discover.UDPv5, error)
+	TargetPeers() uint
 }
 
 // Config sets up a p2p host and discv5 service from configuration.
@@ -175,6 +178,10 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
 	return conf, nil
 }
 
+func (conf *Config) TargetPeers() uint {
+	return conf.PeersLo
+}
+
 func (conf *Config) loadListenOpts(ctx *cli.Context) error {
 	listenIP := ctx.GlobalString(flags.ListenIP.Name)
 	if listenIP != "" { // optional

op-node/p2p/host_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/ecdsa"
 	"crypto/rand"
+	"math/big"
 	"net"
 	"testing"
 	"time"
@@ -12,6 +13,7 @@ import (
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/testlog"
 	"github.com/ethereum/go-ethereum/log"
+	"github.com/ethereum/go-ethereum/p2p/enode"
 	"github.com/ethereum/go-ethereum/rpc"
 	ds "github.com/ipfs/go-datastore"
 	"github.com/ipfs/go-datastore/sync"
@@ -222,6 +224,123 @@ func TestP2PFull(t *testing.T) {
 	require.NoError(t, p2pClientA.UnprotectPeer(ctx, hostB.ID()))
 }
 
+func TestDiscovery(t *testing.T) {
+	pA, _, err := crypto.GenerateSecp256k1Key(rand.Reader)
+	require.NoError(t, err, "failed to generate new p2p priv key")
+	pB, _, err := crypto.GenerateSecp256k1Key(rand.Reader)
+	require.NoError(t, err, "failed to generate new p2p priv key")
+	pC, _, err := crypto.GenerateSecp256k1Key(rand.Reader)
+	require.NoError(t, err, "failed to generate new p2p priv key")
+
+	logA := testlog.Logger(t, log.LvlError).New("host", "A")
+	logB := testlog.Logger(t, log.LvlError).New("host", "B")
+	logC := testlog.Logger(t, log.LvlError).New("host", "C")
+
+	mplexC, err := mplexC()
+	require.NoError(t, err)
+	yamuxC, err := yamuxC()
+	require.NoError(t, err)
+	noiseC, err := noiseC()
+	require.NoError(t, err)
+	tlsC, err := tlsC()
+	require.NoError(t, err)
+
+	discDBA, err := enode.OpenDB("") // "" = memory db
+	require.NoError(t, err)
+	discDBB, err := enode.OpenDB("")
+	require.NoError(t, err)
+	discDBC, err := enode.OpenDB("")
+	require.NoError(t, err)
+
+	rollupCfg := &rollup.Config{L2ChainID: big.NewInt(901)}
+
+	confA := Config{
+		Priv:                (*ecdsa.PrivateKey)((pA).(*crypto.Secp256k1PrivateKey)),
+		DisableP2P:          false,
+		NoDiscovery:         false,
+		AdvertiseIP:         net.IP{127, 0, 0, 1},
+		ListenUDPPort:       0, // bind to any available port
+		ListenIP:            net.IP{127, 0, 0, 1},
+		ListenTCPPort:       0, // bind to any available port
+		StaticPeers:         nil,
+		HostMux:             []lconf.MsMuxC{yamuxC, mplexC},
+		HostSecurity:        []lconf.MsSecC{noiseC, tlsC},
+		NoTransportSecurity: false,
+		PeersLo:             1,
+		PeersHi:             10,
+		PeersGrace:          time.Second * 10,
+		NAT:                 false,
+		UserAgent:           "optimism-testing",
+		TimeoutNegotiation:  time.Second * 2,
+		TimeoutAccept:       time.Second * 2,
+		TimeoutDial:         time.Second * 2,
+		Store:               sync.MutexWrap(ds.NewMapDatastore()),
+		DiscoveryDB:         discDBA,
+		ConnGater:           DefaultConnGater,
+		ConnMngr:            DefaultConnManager,
+	}
+	// copy config A, and change the settings for B
+	confB := confA
+	confB.Priv = (*ecdsa.PrivateKey)((pB).(*crypto.Secp256k1PrivateKey))
+	confB.Store = sync.MutexWrap(ds.NewMapDatastore())
+	confB.DiscoveryDB = discDBB
+
+	resourcesCtx, resourcesCancel := context.WithCancel(context.Background())
+	defer resourcesCancel()
+
+	nodeA, err := NewNodeP2P(context.Background(), rollupCfg, logA, &confA, &mockGossipIn{})
+	require.NoError(t, err)
+	defer nodeA.Close()
+	hostA := nodeA.Host()
+	go nodeA.DiscoveryProcess(resourcesCtx, logA, rollupCfg, 10)
+
+	// Add A as bootnode to B
+	confB.Bootnodes = []*enode.Node{nodeA.Dv5Udp().Self()}
+	// Copy B config to C, and ensure they have a different priv / peerstore
+	confC := confB
+	confC.Priv = (*ecdsa.PrivateKey)((pC).(*crypto.Secp256k1PrivateKey))
+	confC.Store = sync.MutexWrap(ds.NewMapDatastore())
+	confB.DiscoveryDB = discDBC
+
+	// Start B
+	nodeB, err := NewNodeP2P(context.Background(), rollupCfg, logB, &confB, &mockGossipIn{})
+	require.NoError(t, err)
+	defer nodeB.Close()
+	hostB := nodeB.Host()
+	go nodeB.DiscoveryProcess(resourcesCtx, logB, rollupCfg, 10)
+
+	// Track connections to B
+	connsB := make(chan network.Conn, 2)
+	hostB.Network().Notify(&network.NotifyBundle{
+		ConnectedF: func(n network.Network, conn network.Conn) {
+			log.Info("connection to B", "peer", conn.RemotePeer())
+			connsB <- conn
+		}})
+
+	// Start C
+	nodeC, err := NewNodeP2P(context.Background(), rollupCfg, logC, &confC, &mockGossipIn{})
+	require.NoError(t, err)
+	defer nodeC.Close()
+	hostC := nodeC.Host()
+	go nodeC.DiscoveryProcess(resourcesCtx, logC, rollupCfg, 10)
+
+	// B and C don't know each other yet, but both have A as a bootnode.
+	// It should only be a matter of time for them to connect, if they discover each other via A.
+	var firstPeersOfB []peer.ID
+	for i := 0; i < 2; i++ {
+		select {
+		case <-time.After(time.Second * 30):
+			t.Fatal("failed to get connection to B in time")
+		case c := <-connsB:
+			firstPeersOfB = append(firstPeersOfB, c.RemotePeer())
+		}
+	}
+	// B should be connected to the bootnode it used (it's a valid optimism node to connect to here)
+	require.Contains(t, firstPeersOfB, hostA.ID())
+	// C should be connected, although this one might take more time to discover
+	require.Contains(t, firstPeersOfB, hostC.ID())
+}
+
 // Most tests should use mocknets instead of using the actual local host network
 func TestP2PMocknet(t *testing.T) {
 	mnet, err := mocknet.FullMeshConnected(3)

op-node/p2p/node.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
+
+	ma "github.com/multiformats/go-multiaddr"
 
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum/go-ethereum/log"
@@ -20,10 +23,11 @@ type NodeP2P struct {
 	host    host.Host           // p2p host (optional, may be nil)
 	gater   ConnectionGater     // p2p gater, to ban/unban peers with, may be nil even with p2p enabled
 	connMgr connmgr.ConnManager // p2p conn manager, to keep a reliable number of peers, may be nil even with p2p enabled
-	dv5Local *enode.LocalNode    // p2p discovery identity (optional, may be nil)
-	dv5Udp   *discover.UDPv5     // p2p discovery service (optional, may be nil)
-	gs       *pubsub.PubSub      // p2p gossip router (optional, may be nil)
-	gsOut    GossipOut           // p2p gossip application interface for publishing (optional, may be nil)
+	// the below components are all optional, and may be nil. They require the host to not be nil.
+	dv5Local *enode.LocalNode // p2p discovery identity
+	dv5Udp   *discover.UDPv5  // p2p discovery service
+	gs       *pubsub.PubSub   // p2p gossip router
+	gsOut    GossipOut        // p2p gossip application interface for publishing
 }
 
 func NewNodeP2P(resourcesCtx context.Context, rollupCfg *rollup.Config, log log.Logger, setup SetupP2P, gossipIn GossipIn) (*NodeP2P, error) {
@@ -46,12 +50,6 @@ func NewNodeP2P(resourcesCtx context.Context, rollupCfg *rollup.Config, log log.
 
 func (n *NodeP2P) init(resourcesCtx context.Context, rollupCfg *rollup.Config, log log.Logger, setup SetupP2P, gossipIn GossipIn) error {
 	var err error
-	// All nil if disabled.
-	n.dv5Local, n.dv5Udp, err = setup.Discovery(log.New("p2p", "discv5"))
-	if err != nil {
-		return fmt.Errorf("failed to start discv5: %v", err)
-	}
-
 	// nil if disabled.
 	n.host, err = setup.Host(log)
 	if err != nil {
@@ -81,6 +79,17 @@ func (n *NodeP2P) init(resourcesCtx context.Context, rollupCfg *rollup.Config, l
 			return fmt.Errorf("failed to join blocks gossip topic: %v", err)
 		}
 		log.Info("started p2p host", "addrs", n.host.Addrs(), "peerID", n.host.ID().Pretty())
+
+		tcpPort, err := FindActiveTCPPort(n.host)
+		if err != nil {
+			log.Warn("failed to find what TCP port p2p is binded to", "err", err)
+		}
+
+		// All nil if disabled.
+		n.dv5Local, n.dv5Udp, err = setup.Discovery(log.New("p2p", "discv5"), rollupCfg, tcpPort)
+		if err != nil {
+			return fmt.Errorf("failed to start discv5: %v", err)
+		}
 	}
 	return nil
 }
@@ -130,3 +139,20 @@ func (n *NodeP2P) Close() error {
 	}
 	return result.ErrorOrNil()
 }
+
+func FindActiveTCPPort(h host.Host) (uint16, error) {
+	var tcpPort uint16
+	for _, addr := range h.Addrs() {
+		tcpPortStr, err := addr.ValueForProtocol(ma.P_TCP)
+		if err != nil {
+			continue
+		}
+		v, err := strconv.ParseUint(tcpPortStr, 10, 16)
+		if err != nil {
+			continue
+		}
+		tcpPort = uint16(v)
+		break
+	}
+	return tcpPort, nil
+}

op-node/p2p/prepared.go

@@ -4,6 +4,10 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/ethereum/go-ethereum/p2p/enr"
+
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/p2p/discover"
 	"github.com/ethereum/go-ethereum/p2p/enode"
@@ -20,6 +24,10 @@ type Prepared struct {
 
 var _ SetupP2P = (*Prepared)(nil)
 
+func (p *Prepared) TargetPeers() uint {
+	return 20
+}
+
 func (p *Prepared) Check() error {
 	if (p.LocalNode == nil) != (p.UDPv5 == nil) {
 		return fmt.Errorf("inconsistent discv5 setup: %v <> %v", p.LocalNode, p.UDPv5)
@@ -36,6 +44,16 @@ func (p *Prepared) Host(log log.Logger) (host.Host, error) {
 }
 
 // Discovery creates a disc-v5 service. Returns nil, nil, nil if discovery is disabled.
-func (p *Prepared) Discovery(log log.Logger) (*enode.LocalNode, *discover.UDPv5, error) {
+func (p *Prepared) Discovery(log log.Logger, rollupCfg *rollup.Config, tcpPort uint16) (*enode.LocalNode, *discover.UDPv5, error) {
+	if p.LocalNode != nil {
+		dat := OptimismENRData{
+			chainID: rollupCfg.L2ChainID.Uint64(),
+			version: 0,
+		}
+		p.LocalNode.Set(&dat)
+		if tcpPort != 0 {
+			p.LocalNode.Set(enr.TCP(tcpPort))
+		}
+	}
 	return p.LocalNode, p.UDPv5, nil
 }

op-node/p2p/rpc_server.go

@@ -5,7 +5,6 @@ import (
 	"crypto/ecdsa"
 	"errors"
 	"fmt"
-	"io"
 	"net"
 	"time"
 
@@ -26,8 +25,6 @@ import (
 // TODO: dynamic peering
 // - req-resp protocol to ensure peers from a different chain learn they shouldn't be connected
 // - banning peers based on score
-// - store enode in peerstore in dynamic-peering background process
-// - peers must be tagged with the "optimism" tag and marked with high value if the chain ID matches
 
 var (
 	DisabledDiscovery   = errors.New("discovery disabled")
@@ -50,7 +47,6 @@ type Node interface {
 	ConnectionGater() ConnectionGater
 	// ConnectionManager returns the connection manager, to protect peers with, may be nil
 	ConnectionManager() connmgr.ConnManager
-	io.Closer
 }
 
 type APIBackend struct {

specs/rollup-node-p2p.md

@@ -86,10 +86,10 @@ The Ethereum Node Record (ENR) for an Optimism rollup node must contain the foll
 - A UDP port (`udp` field) representing the local discv5 listening port.
 - An Optimism (`optimism` field) L2 network identifier
 
-The `optimism` value is encoded as the concatenation of:
+The `optimism` value is encoded as a single RLP `bytes` value, the concatenation of:
 
-- chain ID (`varint`)
-- fork ID (`varint`)
+- chain ID (`unsigned varint`)
+- fork ID (`unsigned varint`)
 
 Note that DiscV5 is a shared DHT (Distributed Hash Table): the L1 consensus and execution nodes,
 as well as testnet nodes, and even external IOT nodes, all communicate records in this large common DHT.