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.

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

[libp2p-dns] Implement /dnsaddr resolution. #1931

Merged
merged 4 commits into from Mar 17, 2021
Merged

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Jan 21, 2021

Closes #1920, as well as, by necessity, #1462.

This PR is based on #1927 - a proper diff until that PR is merged can be seen here.

In order to correctly resolve /dnsaddr addresses, the DNS transport needs to be given "fully qualified" multiaddresses when dialing, i.e. those that end with the /p2p/... protocol, since it chooses from the TXT records such addresses with a matching suffix of the address being dialed. So to pick the addresses for the correct peer, the /p2p protocol needs to be at the end of the address being dialed. This essentially goes back to #1462. So to address #1462 as well as implement /dnsaddr resolution, we now make sure that dialing always uses such fully qualified addresses by appending the /p2p protocol as necessary in the Network of libp2p-core before giving the address for dialing to the transport. This is mostly transparent for the PeerId-based APIs, i.e. it is of course still possible to use a PeerId together with an "unqualified" address for dialing. It is just that at the end of the day, Transport::dial() always gets a fully-qualified address (when called from the Network anyway) and hence transports now permit a trailing /p2p suffix (and in the case of the DNS transport, even make essential use of it).

As a side-effect, Network::dial(addr) and Swarm::dial_addr(addr) now also handle "fully-qualified" /p2p addresses, since the Network will then take the peer ID from the end of the address.

A related change as part of implementing /dnsaddr resolution is that libp2p-dns now actually tries to use all resolved addresses (well, up to a fixed maximum of dialing attempts).

The net result of these changes can be seen in the now cleaned up ipfs-kad example, which can finally make use of /dnsaddr/bootstrap.libp2p.io.

@romanb romanb linked an issue Jan 21, 2021 that may be closed by this pull request
@romanb
Copy link
Contributor Author

romanb commented Jan 21, 2021

It looks like looking up TXT records from _dnsaddr.bootstrap.libp2p.io does not work on the build machine with the system DNS configuration.

@mxinden
Copy link
Member

mxinden commented Jan 25, 2021

It looks like looking up TXT records from _dnsaddr.bootstrap.libp2p.io does not work on the build machine with the system DNS configuration.

This seems odd to me. Any suspicion why that might be the case?

As a data point, though I doubt a very helpful one, the basic_resolve unit test succeeds on my machine.

@romanb
Copy link
Contributor Author

romanb commented Jan 25, 2021

This seems odd to me. Any suspicion why that might be the case?

I don't know yet. I may have to do some noisy debugging on this branch for CI, for which I apologise in advance.

transports/dns/src/lib.rs Outdated Show resolved Hide resolved
@romanb
Copy link
Contributor Author

romanb commented Jan 25, 2021

I changed the tests to be explicit about the name server(s) to use (quad9 in this case). Besides solving the problem with the request timeouts for TXT lookups with the system configuration, I think it is better to avoid the indirection via the opaque system configuration for the tests.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Changes on top of #1927 look sound to me.

In my eyes your changes to Swarm::dial_addr and Network::dial prevent a lot of confusion (thinking back to my early days with rust-libp2p). Very much appreciated.

@romanb
Copy link
Contributor Author

romanb commented Feb 17, 2021

Still waiting on #1927.

To that end, since resolving `/dnsaddr` addresses needs
"fully qualified" multiaddresses when dialing, i.e. those
that end with the `/p2p/...` protocol, we make sure that
dialing always uses such fully qualified addresses by
appending the `/p2p` protocol as necessary. As a side-effect,
this adds support for dialing peers via "fully qualified"
addresses, as an alternative to using a `PeerId` together
with a `Multiaddr` with or without the `/p2p` protocol.
@romanb
Copy link
Contributor Author

romanb commented Mar 16, 2021

@mxinden The changes in this PR seem to have had a small (I think positive) impact on libp2p-relay, mainly the tests, due to the changes for better support of /p2p addresses. Could you take a look at efe0960 and check whether that all makes sense? Thanks!

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

New changes look good to me. Thanks for the additional assert_eqs.

})) => {
return Poll::Ready(Some(Ok(ListenerEvent::Upgrade {
upgrade: RelayedListenerUpgrade::Relayed(Some(stream)),
local_addr: relay_addr
.with(Protocol::P2p(relay_peer_id.into()))
Copy link
Member

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate /dnsaddr support in libp2p-dns Allow dialing multiaddr containing a peer id
2 participants