fix: swarm: refactor address resolution #2990
Conversation
MarcoPolo
force-pushed
the
marco/filter-dial-addrs
branch
3 times, most recently
from
16c1688 to
99d2253
Compare
MarcoPolo
changed the title
|
I'm going to refactor this function and clean up things around here. As a side effect, I think we won't need #2989 anymore. |
Refactors how DNS Address resolution works.
MarcoPolo
force-pushed
the
marco/filter-dial-addrs
branch
from
70cb2b5 to
efd1e0e
Compare
|
@sukunrt this is ready now. Hopefully it's a lot clearer |
MarcoPolo
changed the title
MarcoPolo
changed the title
| if recursionLimit <= 0 { | ||
| return []ma.Multiaddr{maddr}, nil | ||
| } | ||
| var resolved, toResolve []ma.Multiaddr |
There was a problem hiding this comment.
You can create these just before using them a couple of lines below and avoid allocations by providing capacity = len(addrs)
options.go
Outdated
| // MultiaddrResolver sets the libp2p dns resolver | ||
| func MultiaddrResolver(rslv *madns.Resolver) Option { | ||
| func MultiaddrResolver(rslv swarm.MultiaddrDNSResolver) Option { |
There was a problem hiding this comment.
Why change this? We can wrap the provided DNS to this swarm interface when constructing the swarm.
options.go
Outdated
| // MultiaddrResolver sets the libp2p dns resolver | ||
| func MultiaddrResolver(rslv *madns.Resolver) Option { | ||
| func MultiaddrResolver(rslv swarm.MultiaddrDNSResolver) Option { |
There was a problem hiding this comment.
If you do want to export the new interface, I prefer just exporting the Swarm Option directly. Swarm options can be used directly with the top level libp2p option SwarmOpts.
Otherwise I'd prefer the name be changed to MultiaddrDNSResolver or just DNSResolver
There was a problem hiding this comment.
Right now only the swarm uses this. No strong opinion on whether it should be a swarm opt or libp2p opt. It feels general enough to be a libp2p opt.
p2p/net/swarm/swarm.go
Outdated
| for _, addr := range toResolve { | ||
| resolvedAddrs, err := r.ResolveDNSAddr(ctx, expectedPeerID, addr, recursionLimit-1, outputLimit-len(resolved)) | ||
| if err != nil { | ||
| log.Warnf("failed to resolve dnsaddr %v %s: ", addr, err) |
There was a problem hiding this comment.
Is there a potential exponential case here?
Consider a dnsaddr that resolves to 10 dnsaddrs. Each of these 10 dnsaddrs resolve to 10 dnsaddrs and so on.
outputLimit-len(resolved) will be 10 because all of these are dnsaddrs and len(resolved) will be 0. So we call ResolveDNSAddrs on these 10 addrs and then call it again with outputLimit 10 for each of these and so on.
* Rebase on top of resolveAddrs refactor * Add comments * Sanitize address inputs when returning a reservation message (#3006)
Fixes an issue around p2p-circuit multiaddrs and DNS resolution.