Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/utils: use eth DNS tree for snap discovery #22808

Merged
merged 4 commits into from
May 4, 2021
Merged

cmd/utils: use eth DNS tree for snap discovery #22808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d460c68
p2p/dnsdisc: add singleflight group around requests
fjl May 3, 2021
e2c8716
eth/protocols/snap: filter discovery iterator by snap ENR entry
fjl May 3, 2021
c6ff4cd
eth: use shared client for DNS discovery iterators
fjl May 3, 2021
29fed45
cmd/utils: use eth DNS tree for snap discovery
fjl May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions cmd/utils/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down
10 changes: 8 additions & 2 deletions eth/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -237,14 +238,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...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, is this safe if eth.config.EthDiscoveryURLs == []? Previously we had a special clause that returned nil, nil for that, but here we're instantiating a random iterator even for no-urls. Just want a confirm that we won't spin loop shomewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But let me try it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it, and there is no problem with empty list. The iterator is basically closed immediately.

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)

Expand Down Expand Up @@ -542,6 +546,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.
Expand Down
11 changes: 0 additions & 11 deletions eth/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -62,13 +61,3 @@ func (eth *Ethereum) currentEthEntry() *ethEntry {
return &ethEntry{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...)
}
6 changes: 6 additions & 0 deletions eth/protocols/snap/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
51 changes: 30 additions & 21 deletions p2p/dnsdisc/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this crash if doResolveEntry errors? We're returning a nil interface in that case (line 184)

Copy link
Contributor Author

@fjl fjl May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why it uses the two-value type assertion. It would crash with e := ei.(entry).

return e, err
}

// doResolveEntry fetches an entry via DNS.
Expand Down