From b8040a430e34117f121c67e8deee4a5e889e8372 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Tue, 4 May 2021 11:29:32 +0200 Subject: [PATCH] cmd/utils: use eth DNS tree for snap discovery (#22808) This removes auto-configuration of the snap.*.ethdisco.net DNS discovery tree. Since measurements have shown that > 75% of nodes in all.*.ethdisco.net support snap, we have decided to retire the dedicated index for snap and just use the eth tree instead. The dial iterators of eth and snap now use the same DNS tree in the default configuration, so both iterators should use the same DNS discovery client instance. This ensures that the record cache and rate limit are shared. Records will not be requested multiple times. While testing the change, I noticed that duplicate DNS requests do happen even when the client instance is shared. This is because the two iterators request the tree root, link tree root, and first levels of the tree in lockstep. To avoid this problem, the change also adds a singleflight.Group instance in the client. When one iterator attempts to resolve an entry which is already being resolved, the singleflight object waits for the existing resolve call to finish and returns the entry to both places. --- cmd/utils/flags.go | 6 +---- eth/backend.go | 10 +++++-- eth/discovery.go | 11 -------- eth/protocols/snap/handler.go | 6 +++++ go.mod | 1 + go.sum | 2 ++ p2p/dnsdisc/client.go | 51 ++++++++++++++++++++--------------- 7 files changed, 48 insertions(+), 39 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index a50fdd968a..a81188342f 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -1690,11 +1690,7 @@ func SetDNSDiscoveryDefaults(cfg *ethconfig.Config, genesis common.Hash) { } if url := params.KnownDNSNetwork(genesis, protocol); url != "" { cfg.EthDiscoveryURLs = []string{url} - } - if cfg.SyncMode == downloader.SnapSync { - if url := params.KnownDNSNetwork(genesis, "snap"); url != "" { - cfg.SnapDiscoveryURLs = []string{url} - } + cfg.SnapDiscoveryURLs = cfg.EthDiscoveryURLs } } diff --git a/eth/backend.go b/eth/backend.go index 7aac17eeb3..7d8b0c52c6 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -50,6 +50,7 @@ import ( "github.com/ethereum/go-ethereum/miner" "github.com/ethereum/go-ethereum/node" "github.com/ethereum/go-ethereum/p2p" + "github.com/ethereum/go-ethereum/p2p/dnsdisc" "github.com/ethereum/go-ethereum/p2p/enode" "github.com/ethereum/go-ethereum/params" "github.com/ethereum/go-ethereum/rlp" @@ -239,14 +240,17 @@ func New(stack *node.Node, config *ethconfig.Config) (*Ethereum, error) { } eth.APIBackend.gpo = gasprice.NewOracle(eth.APIBackend, gpoParams) - eth.ethDialCandidates, err = setupDiscovery(eth.config.EthDiscoveryURLs) + // Setup DNS discovery iterators. + dnsclient := dnsdisc.NewClient(dnsdisc.Config{}) + eth.ethDialCandidates, err = dnsclient.NewIterator(eth.config.EthDiscoveryURLs...) if err != nil { return nil, err } - eth.snapDialCandidates, err = setupDiscovery(eth.config.SnapDiscoveryURLs) + eth.snapDialCandidates, err = dnsclient.NewIterator(eth.config.SnapDiscoveryURLs...) if err != nil { return nil, err } + // Start the RPC service eth.netRPCService = ethapi.NewPublicNetAPI(eth.p2pServer, config.NetworkId) @@ -544,6 +548,8 @@ func (s *Ethereum) Start() error { // Ethereum protocol. func (s *Ethereum) Stop() error { // Stop all the peer-related stuff first. + s.ethDialCandidates.Close() + s.snapDialCandidates.Close() s.handler.Stop() // Then stop everything else. diff --git a/eth/discovery.go b/eth/discovery.go index 855ce3b0e1..70668b2b70 100644 --- a/eth/discovery.go +++ b/eth/discovery.go @@ -19,7 +19,6 @@ package eth import ( "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/forkid" - "github.com/ethereum/go-ethereum/p2p/dnsdisc" "github.com/ethereum/go-ethereum/p2p/enode" "github.com/ethereum/go-ethereum/rlp" ) @@ -62,13 +61,3 @@ func (eth *Ethereum) currentEthEntry() *ethEntry { return ðEntry{ForkID: forkid.NewID(eth.blockchain.Config(), eth.blockchain.Genesis().Hash(), eth.blockchain.CurrentHeader().Number.Uint64())} } - -// setupDiscovery creates the node discovery source for the `eth` and `snap` -// protocols. -func setupDiscovery(urls []string) (enode.Iterator, error) { - if len(urls) == 0 { - return nil, nil - } - client := dnsdisc.NewClient(dnsdisc.Config{}) - return client.NewIterator(urls...) -} diff --git a/eth/protocols/snap/handler.go b/eth/protocols/snap/handler.go index 9bfac6f03f..3d668a2ebb 100644 --- a/eth/protocols/snap/handler.go +++ b/eth/protocols/snap/handler.go @@ -84,6 +84,12 @@ type Backend interface { // MakeProtocols constructs the P2P protocol definitions for `snap`. func MakeProtocols(backend Backend, dnsdisc enode.Iterator) []p2p.Protocol { + // Filter the discovery iterator for nodes advertising snap support. + dnsdisc = enode.Filter(dnsdisc, func(n *enode.Node) bool { + var snap enrEntry + return n.Load(&snap) == nil + }) + protocols := make([]p2p.Protocol, len(ProtocolVersions)) for i, version := range ProtocolVersions { version := version // Closure diff --git a/go.mod b/go.mod index 63636caae1..079d40ae6f 100644 --- a/go.mod +++ b/go.mod @@ -60,6 +60,7 @@ require ( github.com/tklauser/go-sysconf v0.3.5 // indirect github.com/tyler-smith/go-bip39 v1.0.1-0.20181017060643-dbb3b84ba2ef golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 + golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect golang.org/x/sys v0.0.0-20210420205809-ac73e9fd8988 golang.org/x/text v0.3.4 golang.org/x/time v0.0.0-20201208040808-7e3f01d25324 diff --git a/go.sum b/go.sum index cadcd1de95..3ae80f7b9c 100644 --- a/go.sum +++ b/go.sum @@ -455,6 +455,8 @@ golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9 h1:SQFwaSi55rU7vdNs9Yr0Z324VNlrF+0wMqRXT4St8ck= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= +golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/p2p/dnsdisc/client.go b/p2p/dnsdisc/client.go index 3e4b50aadd..f2a4bed4c6 100644 --- a/p2p/dnsdisc/client.go +++ b/p2p/dnsdisc/client.go @@ -32,15 +32,17 @@ import ( "github.com/ethereum/go-ethereum/p2p/enode" "github.com/ethereum/go-ethereum/p2p/enr" lru "github.com/hashicorp/golang-lru" + "golang.org/x/sync/singleflight" "golang.org/x/time/rate" ) // Client discovers nodes by querying DNS servers. type Client struct { - cfg Config - clock mclock.Clock - entries *lru.Cache - ratelimit *rate.Limiter + cfg Config + clock mclock.Clock + entries *lru.Cache + ratelimit *rate.Limiter + singleflight singleflight.Group } // Config holds configuration options for the client. @@ -135,17 +137,20 @@ func (c *Client) NewIterator(urls ...string) (enode.Iterator, error) { // resolveRoot retrieves a root entry via DNS. func (c *Client) resolveRoot(ctx context.Context, loc *linkEntry) (rootEntry, error) { - txts, err := c.cfg.Resolver.LookupTXT(ctx, loc.domain) - c.cfg.Logger.Trace("Updating DNS discovery root", "tree", loc.domain, "err", err) - if err != nil { - return rootEntry{}, err - } - for _, txt := range txts { - if strings.HasPrefix(txt, rootPrefix) { - return parseAndVerifyRoot(txt, loc) + e, err, _ := c.singleflight.Do(loc.str, func() (interface{}, error) { + txts, err := c.cfg.Resolver.LookupTXT(ctx, loc.domain) + c.cfg.Logger.Trace("Updating DNS discovery root", "tree", loc.domain, "err", err) + if err != nil { + return rootEntry{}, err } - } - return rootEntry{}, nameError{loc.domain, errNoRoot} + for _, txt := range txts { + if strings.HasPrefix(txt, rootPrefix) { + return parseAndVerifyRoot(txt, loc) + } + } + return rootEntry{}, nameError{loc.domain, errNoRoot} + }) + return e.(rootEntry), err } func parseAndVerifyRoot(txt string, loc *linkEntry) (rootEntry, error) { @@ -168,17 +173,21 @@ func (c *Client) resolveEntry(ctx context.Context, domain, hash string) (entry, if err := c.ratelimit.Wait(ctx); err != nil { return nil, err } - cacheKey := truncateHash(hash) if e, ok := c.entries.Get(cacheKey); ok { return e.(entry), nil } - e, err := c.doResolveEntry(ctx, domain, hash) - if err != nil { - return nil, err - } - c.entries.Add(cacheKey, e) - return e, nil + + ei, err, _ := c.singleflight.Do(cacheKey, func() (interface{}, error) { + e, err := c.doResolveEntry(ctx, domain, hash) + if err != nil { + return nil, err + } + c.entries.Add(cacheKey, e) + return e, nil + }) + e, _ := ei.(entry) + return e, err } // doResolveEntry fetches an entry via DNS.