Dial queue timeout for multiple multiaddresses incorrectly used for each connection separately

[akim-bow]

Version:
libp2p@1.2.0

Severity:

Medium

Description:

If remote peer proposes multiple multiaddresses, e.g. [/dns/nox-1, /ip4/10.50.10.10, /ip4/127.0.0.1] (only the last one is reachable by default), libp2p will try to connect to them one by one. As you can see in the source code, the shared signal is created and used in every attempt to dial multiaddresses. Hence, the first address could take up all the time until the signal aborts and other addresses would be dropped without an attempt to connect to them.

I'm proposing to use 2 separate signals - first one for batch of multiaddresses, the second one for each multiaddress separately. It will solve the case when a peer provides multiple adresses and only some of them are actually valid. If the description isn't clear enough i can try to build simple reproduction example.

[akim-bow]

@achingbrain can you please look into it?

[zeroxbt]

@achingbrain could you take a look at this ? Most auto dials are failing because of it

[achingbrain]

@akim-bow what kind of addresses are you having problems with?

The example you gave was [/dns/nox-1, /ip4/10.50.10.10, /ip4/127.0.0.1] - DNS addresses take time to resolve, so that can be bad, fair enough, but the other two are private local addresses so assuming TCP, they should be fast to fail?

If you know certain addresses are going to be unreliable or slow, you can also pass an addressSorter as part of the connectionManager config to ensure they are dialled last, or a connectionGater to ensure they are not dialled at all.

[zeroxbt]

@achingbrain I'm having the same issue when running nodes in local environment.

When dialing a peer, these are the addresses found in the peer store:

[/ip4/10.2.0.2/tcp/9106, /ip4/127.0.0.1/tcp/9106, /ip4/172.20.5.94/tcp/9106]

The first address is not reachable, while the other two are. The dialer fails after dialTimeout milliseconds because of the first address, without ever attempting to dial the other two. All auto dials are also failing for the same reason.

[achingbrain]

My concern with the proposed solution is that having separate timeouts for individual addresses could lead to very long dial times.

I wonder if we could be smarter with the dialling, for example if the IP address of the target multiaddr is a class A private network address, deprioritise or entirely skip dialling it unless the current node also has a class A private network address in the same logical network?

[zeroxbt]

Although sorting and filtering addresses is a good option, as a user I would still expect the dial function to attempt dialing all provided addresses.
Why not give users the option to choose a maxParallelDialsPerPeer parameter with a default value of 1 ?

[mkermani144]

@achingbrain
I think the problem with the addressSorter or connectionGater solution is that, you don't always know how to sort the addresses. In my case (which is a special case), the nodes advertise a hopefully public ip multiaddress (the node tries to identify its public ip through some third party, but it may get a wrong public ip from the third party - note that the ip is in public range, not private), and a circuit relay one. If the public ip is valid, I prefer it to be used for dial. If it's not, the circuit relay address should be dialed. In my case, there is no way to check if the address is valid in the addressSorter or connectionGater. Because the public ip comes first in the list of addresses, other nodes dial this address (which may be unreachable), and the same issue happens to me as others.

[achingbrain]

the nodes advertise a hopefully public ip multiaddress (the node tries to identify its public ip through some third party, but it may get a wrong public ip from the third party - note that the ip is in public range, not private)

Are you not using the @libp2p/autonat protocol? This sounds like the exact problem it's designed to solve.

[mkermani144]

Are you not using the @libp2p/autonat protocol? This sounds like the exact problem it's designed to solve.

I know about the autonat protocol. For some reasons that are beyond the context of this issue, I couldn't use it. In short, it uses random walk, which requires the node to include a service implementing it (such as kad-dht), but mine doesn't.

[mkermani144]

By the way, I implemented a workaround for the issue that can be used until a good solution is found here. It's special to my case, but can be generalized based on other use cases, too. It simply first tries to dial a multiaddress of a peer with the higher priority (in my case, the hopefully public multiaddress) but with reduced timeout, and if it fails, it falls back to dialing other addresses (in my case, the relayed ones).

I leave it here just for a quick solution; it doesn't cover many edge cases, for sure.

const dialPeer = async (peerId: PeerId) => {
  const publicMultiaddr = (await node.peerStore.get(peerId)).addresses.find(
    (address) => address.multiaddr.isThinWaistAddress(),
  );
  const otherMultiaddrs = (await node.peerStore.get(peerId)).addresses
    .filter(
      (address) =>
        !address.multiaddr.isThinWaistAddress() && address.multiaddr, // in my case, high priority means not being a relayed multiaddr
    )
    .map((addr) =>
       // advertised relay addresses may omit their peerId, so we need to append them again, otherwise transportManager will throw
      addr.multiaddr.toString().endsWith('p2p-circuit')
        ? addr.multiaddr.encapsulate(`/p2p/${peerId.toString()}`)
        : addr.multiaddr,
    );
  try {
    if (publicMultiaddr) {
      return await node.dial(publicMultiaddr?.multiaddr, {
        signal: AbortSignal.timeout(5000), // Default timeout is 30s, so we make sure this dial "consumes" at most 5s
      });
    } else {
      return await node.dial(otherMultiaddrs!);
    }
  } catch {
    return await node.dial(otherMultiaddrs!);
  }
}