From 645b0db98e5d264cd0b33b6a2711ca4413ae9172 Mon Sep 17 00:00:00 2001 From: lightclient <14004106+lightclient@users.noreply.github.com> Date: Tue, 11 Jul 2023 13:21:32 -0600 Subject: [PATCH] cmd/utils, p2p: clean up discovery setup (#27518) This simplifies the code that initializes the discovery a bit, and adds new flags for enabling/disabling discv4 and discv5 separately. --------- Co-authored-by: Felix Lange --- cmd/geth/les_test.go | 4 +- cmd/geth/main.go | 2 + cmd/utils/flags.go | 33 +++++++----- cmd/utils/flags_legacy.go | 21 ++++++-- p2p/server.go | 111 ++++++++++++++++++++------------------ 5 files changed, 100 insertions(+), 71 deletions(-) diff --git a/cmd/geth/les_test.go b/cmd/geth/les_test.go index 607b454ea..746f12c76 100644 --- a/cmd/geth/les_test.go +++ b/cmd/geth/les_test.go @@ -146,13 +146,13 @@ func startLightServer(t *testing.T) *gethrpc { t.Logf("Importing keys to geth") runGeth(t, "account", "import", "--datadir", datadir, "--password", "./testdata/password.txt", "--lightkdf", "./testdata/key.prv").WaitExit() account := "0x02f0d131f1f97aef08aec6e3291b957d9efe7105" - server := startGethWithIpc(t, "lightserver", "--allow-insecure-unlock", "--datadir", datadir, "--password", "./testdata/password.txt", "--unlock", account, "--miner.etherbase=0x02f0d131f1f97aef08aec6e3291b957d9efe7105", "--mine", "--light.serve=100", "--light.maxpeers=1", "--nodiscover", "--nat=extip:127.0.0.1", "--verbosity=4") + server := startGethWithIpc(t, "lightserver", "--allow-insecure-unlock", "--datadir", datadir, "--password", "./testdata/password.txt", "--unlock", account, "--miner.etherbase=0x02f0d131f1f97aef08aec6e3291b957d9efe7105", "--mine", "--light.serve=100", "--light.maxpeers=1", "--discv4=false", "--nat=extip:127.0.0.1", "--verbosity=4") return server } func startClient(t *testing.T, name string) *gethrpc { datadir := initGeth(t) - return startGethWithIpc(t, name, "--datadir", datadir, "--nodiscover", "--syncmode=light", "--nat=extip:127.0.0.1", "--verbosity=4") + return startGethWithIpc(t, name, "--datadir", datadir, "--discv4=false", "--syncmode=light", "--nat=extip:127.0.0.1", "--verbosity=4") } func TestPriorityClient(t *testing.T) { diff --git a/cmd/geth/main.go b/cmd/geth/main.go index 42a05f4d1..b9584e70b 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -122,7 +122,9 @@ var ( utils.MinerNewPayloadTimeout, utils.NATFlag, utils.NoDiscoverFlag, + utils.DiscoveryV4Flag, utils.DiscoveryV5Flag, + utils.LegacyDiscoveryV5Flag, utils.NetrestrictFlag, utils.NodeKeyFileFlag, utils.NodeKeyHexFlag, diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 525abf042..feceb99d3 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -253,11 +253,6 @@ var ( Usage: "Comma separated block number-to-hash mappings to require for peering (=)", Category: flags.EthCategory, } - LegacyWhitelistFlag = &cli.StringFlag{ - Name: "whitelist", - Usage: "Comma separated block number-to-hash mappings to enforce (=) (deprecated in favor of --eth.requiredblocks)", - Category: flags.DeprecatedCategory, - } BloomFilterSizeFlag = &cli.Uint64Flag{ Name: "bloomfilter.size", Usage: "Megabytes of memory allocated to bloom-filter for pruning", @@ -770,8 +765,16 @@ var ( Usage: "Disables the peer discovery mechanism (manual peer addition)", Category: flags.NetworkingCategory, } + DiscoveryV4Flag = &cli.BoolFlag{ + Name: "discovery.v4", + Aliases: []string{"discv4"}, + Usage: "Enables the V4 discovery mechanism", + Category: flags.NetworkingCategory, + Value: true, + } DiscoveryV5Flag = &cli.BoolFlag{ - Name: "v5disc", + Name: "discovery.v5", + Aliases: []string{"discv5"}, Usage: "Enables the experimental RLPx V5 (Topic Discovery) mechanism", Category: flags.NetworkingCategory, } @@ -1361,13 +1364,17 @@ func SetP2PConfig(ctx *cli.Context, cfg *p2p.Config) { cfg.NoDiscovery = true } - // if we're running a light client or server, force enable the v5 peer discovery - // unless it is explicitly disabled with --nodiscover note that explicitly specifying - // --v5disc overrides --nodiscover, in which case the later only disables v4 discovery - forceV5Discovery := (lightClient || lightServer) && !ctx.Bool(NoDiscoverFlag.Name) - if ctx.IsSet(DiscoveryV5Flag.Name) { - cfg.DiscoveryV5 = ctx.Bool(DiscoveryV5Flag.Name) - } else if forceV5Discovery { + // Disallow --nodiscover when used in conjunction with light mode. + if (lightClient || lightServer) && ctx.Bool(NoDiscoverFlag.Name) { + Fatalf("Cannot use --" + NoDiscoverFlag.Name + " in light client or light server mode") + } + CheckExclusive(ctx, DiscoveryV4Flag, NoDiscoverFlag) + CheckExclusive(ctx, DiscoveryV5Flag, NoDiscoverFlag) + cfg.DiscoveryV4 = ctx.Bool(DiscoveryV4Flag.Name) + cfg.DiscoveryV5 = ctx.Bool(DiscoveryV5Flag.Name) + + // If we're running a light client or server, force enable the v5 peer discovery. + if lightClient || lightServer { cfg.DiscoveryV5 = true } diff --git a/cmd/utils/flags_legacy.go b/cmd/utils/flags_legacy.go index b554deba8..cf51b5193 100644 --- a/cmd/utils/flags_legacy.go +++ b/cmd/utils/flags_legacy.go @@ -33,27 +33,40 @@ var ShowDeprecated = &cli.Command{ var DeprecatedFlags = []cli.Flag{ NoUSBFlag, + LegacyWhitelistFlag, CacheTrieJournalFlag, CacheTrieRejournalFlag, + LegacyDiscoveryV5Flag, } var ( - // (Deprecated May 2020, shown in aliased flags section) + // Deprecated May 2020, shown in aliased flags section NoUSBFlag = &cli.BoolFlag{ Name: "nousb", Usage: "Disables monitoring for and managing USB hardware wallets (deprecated)", Category: flags.DeprecatedCategory, } - // (Deprecated June 2023, shown in aliased flags section) + // Deprecated March 2022 + LegacyWhitelistFlag = &cli.StringFlag{ + Name: "whitelist", + Usage: "Comma separated block number-to-hash mappings to enforce (=) (deprecated in favor of --eth.requiredblocks)", + Category: flags.DeprecatedCategory, + } + // Deprecated July 2023 CacheTrieJournalFlag = &cli.StringFlag{ Name: "cache.trie.journal", Usage: "Disk journal directory for trie cache to survive node restarts", - Category: flags.PerfCategory, + Category: flags.DeprecatedCategory, } CacheTrieRejournalFlag = &cli.DurationFlag{ Name: "cache.trie.rejournal", Usage: "Time interval to regenerate the trie cache journal", - Category: flags.PerfCategory, + Category: flags.DeprecatedCategory, + } + LegacyDiscoveryV5Flag = &cli.BoolFlag{ + Name: "v5disc", + Usage: "Enables the experimental RLPx V5 (Topic Discovery) mechanism (deprecated, use --discv5 instead)", + Category: flags.DeprecatedCategory, } ) diff --git a/p2p/server.go b/p2p/server.go index 8c417635e..bd0fd7095 100644 --- a/p2p/server.go +++ b/p2p/server.go @@ -93,6 +93,9 @@ type Config struct { // Disabling is useful for protocol debugging (manual topology). NoDiscovery bool + // DiscoveryV4 specifies whether V4 discovery should be started. + DiscoveryV4 bool `toml:",omitempty"` + // DiscoveryV5 specifies whether the new topic-discovery based V5 discovery // protocol should be started or not. DiscoveryV5 bool `toml:",omitempty"` @@ -542,57 +545,28 @@ func (srv *Server) setupLocalNode() error { func (srv *Server) setupDiscovery() error { srv.discmix = enode.NewFairMix(discmixTimeout) - // Add protocol-specific discovery sources. - added := make(map[string]bool) - for _, proto := range srv.Protocols { - if proto.DialCandidates != nil && !added[proto.Name] { - srv.discmix.AddSource(proto.DialCandidates) - added[proto.Name] = true - } - } - // Don't listen on UDP endpoint if DHT is disabled. - if srv.NoDiscovery && !srv.DiscoveryV5 { + if srv.NoDiscovery { return nil } - - listenAddr := srv.ListenAddr - - // Use an alternate listening address for UDP if - // a custom discovery address is configured. - if srv.DiscAddr != "" { - listenAddr = srv.DiscAddr - } - - addr, err := net.ResolveUDPAddr("udp", listenAddr) + conn, err := srv.setupUDPListening() if err != nil { return err } - conn, err := net.ListenUDP("udp", addr) - if err != nil { - return err - } - realaddr := conn.LocalAddr().(*net.UDPAddr) - srv.log.Debug("UDP listener up", "addr", realaddr) - if srv.NAT != nil { - if !realaddr.IP.IsLoopback() { - srv.loopWG.Add(1) - go func() { - nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") - srv.loopWG.Done() - }() - } - } - srv.localnode.SetFallbackUDP(realaddr.Port) - // Discovery V4 - var unhandled chan discover.ReadPacket - var sconn *sharedUDPConn - if !srv.NoDiscovery { - if srv.DiscoveryV5 { - unhandled = make(chan discover.ReadPacket, 100) - sconn = &sharedUDPConn{conn, unhandled} - } + var ( + sconn discover.UDPConn = conn + unhandled chan discover.ReadPacket + ) + // If both versions of discovery are running, setup a shared + // connection, so v5 can read unhandled messages from v4. + if srv.DiscoveryV4 && srv.DiscoveryV5 { + unhandled = make(chan discover.ReadPacket, 100) + sconn = &sharedUDPConn{conn, unhandled} + } + + // Start discovery services. + if srv.DiscoveryV4 { cfg := discover.Config{ PrivateKey: srv.PrivateKey, NetRestrict: srv.NetRestrict, @@ -607,8 +581,6 @@ func (srv *Server) setupDiscovery() error { srv.ntab = ntab srv.discmix.AddSource(ntab.RandomNodes()) } - - // Discovery V5 if srv.DiscoveryV5 { cfg := discover.Config{ PrivateKey: srv.PrivateKey, @@ -616,16 +588,20 @@ func (srv *Server) setupDiscovery() error { Bootnodes: srv.BootstrapNodesV5, Log: srv.log, } - var err error - if sconn != nil { - srv.DiscV5, err = discover.ListenV5(sconn, srv.localnode, cfg) - } else { - srv.DiscV5, err = discover.ListenV5(conn, srv.localnode, cfg) - } + srv.DiscV5, err = discover.ListenV5(sconn, srv.localnode, cfg) if err != nil { return err } } + + // Add protocol-specific discovery sources. + added := make(map[string]bool) + for _, proto := range srv.Protocols { + if proto.DialCandidates != nil && !added[proto.Name] { + srv.discmix.AddSource(proto.DialCandidates) + added[proto.Name] = true + } + } return nil } @@ -696,6 +672,37 @@ func (srv *Server) setupListening() error { return nil } +func (srv *Server) setupUDPListening() (*net.UDPConn, error) { + listenAddr := srv.ListenAddr + + // Use an alternate listening address for UDP if + // a custom discovery address is configured. + if srv.DiscAddr != "" { + listenAddr = srv.DiscAddr + } + addr, err := net.ResolveUDPAddr("udp", listenAddr) + if err != nil { + return nil, err + } + conn, err := net.ListenUDP("udp", addr) + if err != nil { + return nil, err + } + realaddr := conn.LocalAddr().(*net.UDPAddr) + srv.log.Debug("UDP listener up", "addr", realaddr) + if srv.NAT != nil { + if !realaddr.IP.IsLoopback() { + srv.loopWG.Add(1) + go func() { + nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") + srv.loopWG.Done() + }() + } + } + srv.localnode.SetFallbackUDP(realaddr.Port) + return conn, nil +} + // doPeerOp runs fn on the main loop. func (srv *Server) doPeerOp(fn peerOpFunc) { select {