cmd/utils: use eth DNS tree for snap discovery #22808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When multiple iterators use the same enrtree:// URL on a shared client, duplicate DNS requests can be avoided using singleflight.Group.
Measurements have shown that > 75% of eth nodes in the public DNS trees support snap.
fjl
requested review from
holiman,
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
karalabe
reviewed
May 4, 2021
| eth.ethDialCandidates, err = setupDiscovery(eth.config.EthDiscoveryURLs) | ||
| // Setup DNS discovery iterators. | ||
| dnsclient := dnsdisc.NewClient(dnsdisc.Config{}) | ||
| eth.ethDialCandidates, err = dnsclient.NewIterator(eth.config.EthDiscoveryURLs...) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But let me try it anyway.
There was a problem hiding this comment.
I've tested it, and there is no problem with empty list. The iterator is basically closed immediately.
karalabe
reviewed
May 4, 2021
| c.entries.Add(cacheKey, e) | ||
| return e, nil | ||
| }) | ||
| e, _ := ei.(entry) |
There was a problem hiding this comment.
Won't this crash if doResolveEntry errors? We're returning a nil interface in that case (line 184)
There was a problem hiding this comment.
This is why it uses the two-value type assertion. It would crash with e := ei.(entry).
atif-konasl
pushed a commit
to frozeman/pandora-execution-engine
that referenced
this pull request
Oct 15, 2021
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.
9 tasks
9 tasks
This was referenced Sep 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes auto-configuration of the
snap.*.ethdisco.netDNS discovery tree.Since measurements have shown that > 75% of nodes in
all.*.ethdisco.netsupportsnap, 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.