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

Dialing a multiaddr should dial it first #451

Closed
jacobheun opened this issue Sep 3, 2019 · 1 comment
Closed

Dialing a multiaddr should dial it first #451

jacobheun opened this issue Sep 3, 2019 · 1 comment
Assignees
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@jacobheun
Copy link
Contributor

jacobheun commented Sep 3, 2019

The Problem

Currently in libp2p if you dial a Multiaddr, or its string version, the PeerInfo record for that peer will be retrieved and then the Multiaddr will be added to its multiaddr list before dialing. If the peer already has known addresses, the Multiaddr originally provided might not be dialed first.

For example, let's say I am trying to manually connect to an IPFS preload node, /dns4/node0.preload.ipfs.io/tcp/443/wss/ipfs/QmZMxNdpMkewiVZLMRxaNxUeZpDUb34pWjZ1kZvsd16Zic. If I have no information about this node, this address will be dialed, as its the only address.
Now, if I have been on the network for a while, or have previously connected to this node, I will likely have some additional addresses, such as:

/dns4/node0.preload.ipfs.io/tcp/443/wss
/ip4/127.0.0.1/tcp/4001
/ip4/108.61.156.24/tcp/4001
/ip4/45.77.150.82/tcp/4001
/ip6/::1/tcp/4001
/ip6/2001:19f0:5:66d1::/tcp/4001
/ip6/2001:19f0:5:fc8:5400:1ff:feeb:77b3/tcp/4001
/ip4/127.0.0.1/tcp/8081/ws

The problem here, is that the dialing logic causes us to iterate over our available transports and the target peers Multiaddrs. The order of this dial is currently determined by the order our transports were added, and the order of their Multiaddrs. Most likely, this will be the first TCP address in the list, assuming we support TCP as it is often added to configuration first. Instead of dialing /dns4/node0.preload.ipfs.io/tcp/443/wss we would first dial /ip4/127.0.0.1/tcp/4001. This may fail and we will continuing trying other addresses, however, if we happen to have a node running locally on this address, such as go-ipfs, we would connect and then secio would terminate the connection with the error dialed to the wrong peer, Ids do not match as the Peer dialed is not the target Peer. Since the connection has already been "successfully" dialed, no other addresses are currently being tried. Ideally, we should try all addresses until we have a full, encrypted connection.

Solutions

Here are some ways to improve dialing to avoid this:

  1. Specifying a Multiaddr in the dial should cause that Multiaddr to be dialed first. This should mitigate the majority of cases here, unless the target Multiaddr fails, at which point we'd continue down the list.
  2. If encryption fails, try another Multiaddr. Currently we use the first Multiaddr that successfully connects. If encryption fails, we won't try any other Multiaddr, which could actually give us a successful connection.
  3. Dial local Multiaddrs last. While this could potentially slow down connections to local nodes, we will often be dialing external nodes so the benefit here would likely out weight the loss.

Async/Await migration
With the async/await migration we will be getting proper support for aborting dials. This will allow us to dial multiple addresses in parallel, use the first/fastest connection, and then abort the rest. Running all the addresses in parallel (to a reasonable limit), we could ignore the failures we might get by accidentally dialing the wrong, local peer.

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) exp/expert Having worked on the specific codebase is important status/ready Ready to be worked P1 High: Likely tackled by core team if no one steps up labels Sep 3, 2019
@jacobheun jacobheun self-assigned this Sep 3, 2019
@jacobheun
Copy link
Contributor Author

This was fixed as of v0.27.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

1 participant