feat(swarm): use NotDialing condition by default

[b-zee]

It appears to be the case that kad behavior is dialing multiple times to the same peer (not sure of the reason). By changing the default dial condition, this behavior can be inhibited at least in that case.

[b-zee]

A comment I made earlier (source):

a thought I had before is that I can't specify two conditions simultaneously. We'd both not want to dial when we are already dialing, but also if we are already connected. However, because of NAT, we still always want to make sure we can dial outwards.

Perhaps there is some more thought that should go into this, like being able to specify both conditions. Why wouldn't we want to default to both conditions? Current, before dialing we can only manually call is_connected to simulate this.

[dariusc93]

I have always wondered why Disconnected was default, but I also wonder that with NotDialing if it would create a bit of a problem since to my knowledge is that it will dial a peer regardless of the connection status as long as there is no dial attempt present in the pool, unless we want to expand PeerCondition to have different conditions when dialing? I havent actually took the time to actually test that condition enough to determine benefits though so it might be worth exploring.

As for kad, i recall it dialing a peer if its disconnected in which case if it fails to dial it would remove the addresses from the routing table (would have to confirm that though).

[b-zee]

One of my questions is: when are multiple connections desired? I kind of thought that libp2p is supposed to efficiently use connections by multiplexing. This means we only need one connection (or two, for both directions).

it will dial a peer regardless of the connection status as long as there is no dial attempt present

As far as I know, that is the case, yes. I wonder what kind of scenarios there are when dialing. What's the intent of a dial (from a behavior)?

[b-zee]

I made another PR that supersedes this: #4189

[mxinden]

when are multiple connections desired? I kind of thought that libp2p is supposed to efficiently use connections by multiplexing.

In general there is no need for more than two connections between two peers. Though there are some special cases:

• One connection might be a relayed connection (libp2p-relay) used by libp2p-dcutr to upgrade (hole punch) to a direct connection. In the case of a successful hole punch, you will have two connections (relayed and direct) for a brief period of time.

• For libp2p-autonat one wants to test connectivity by establish a new connection, even though one might already be connected.

• When both peers dial one another around the same time one might end up with two connections. Deciding on which of the two to close is non-trivial. One possible way is to close the one where the dialer has the lower peer ID. As of today, we just maintain two, treating them as not particularly expensive (especially in the case of QUIC).

[mxinden]

As discussed in today's open maintainers call (#4209) I suggest we make two changes:

The first change would be non-breaking, i.e. can be released as a patch release. Here we would change libp2p-kad to set PeerCondition::NotDialing when emitting a ToSwarm::Dial. Note that in some cases we might have to check whether we are already connected via Kademlia::connected_peers.

The second change would be a breaking change, i.e. would need to wait for https://github.com/libp2p/rust-libp2p/milestone/7. Similar to #4189 we would add a 4th variant to the PeerCondition enum. That 4th state would be DisconnectedAndNotDialing. Note that I am fine with a different name or using two bools. The current approach using bit_flags seems unnecessarily complicated, but I still have to follow the full conversation on #4189, thus I might be missing something.

[b-zee]

The first change would be non-breaking, i.e. can be released as a patch release. Here we would change libp2p-kad to set PeerCondition::NotDialing when emitting a ToSwarm::Dial. Note that in some cases we might have to check whether we are already connected via Kademlia::connected_peers.

See #4224

The second change would be a breaking change, i.e. would need to wait for https://github.com/libp2p/rust-libp2p/milestone/7. Similar to #4189 we would add a 4th variant to the PeerCondition enum. That 4th state would be DisconnectedAndNotDialing. Note that I am fine with a different name or using two bools. The current approach using bit_flags seems unnecessarily complicated, but I still have to follow the full conversation on #4189, thus I might be missing something.

See #4225

[b-zee]

Closing in favour of #4224 and #4225