existing connection assumed to be incoming in outgoing connection preference check

[anacrolix]

libtorrent version (or branch):

RC_2_0

When checking if a new connection has the preferred connection direction to an existing connection, there's no additional check that the existing connection is an incoming connection. If the existing connection is not an incoming connection, then the new connection does not have priority over the existing one (currently in the implementation it does).

Transmission does not appear to do any preference of connections based on their direction, instead just preferring the existing connection in all cases. In libtorrent, in the case that the existing connection and the new connection can not be ordered (possible with a fix for the issue I described earlier), the existing connection might be preferred to match the behaviour in Transmission. If this makes sense, I can provide a PR doing exactly this.

Just to confirm my understanding of the logic, the client with the highest peer ID is preferred to have the outbound connection to the lower peer ID. For example a clients with peer IDs beginning with -BA-, and -AB- will always prefer connections between them that are initiated from -BA- over those that are initiated by -AB-. I call this out because I believe the behaviour at https://github.com/anacrolix/torrent/blob/7e65e55c35010daf3a189e2b5ebfabf1c5bdd6e0/peerconn.go#L80 is the reverse order and I intend to correct it.

[arvidn]

there's no additional check that the existing connection is an incoming connection

I believe the assumption is that libtorrent would never make an outgoing connection to a peer that it's already connected to.

When you prefer the existing connection, both peers may end up closing different connections still, because that timing may be observed in different orders by the different peers.

[anacrolix]

there's no additional check that the existing connection is an incoming connection

I believe the assumption is that libtorrent would never make an outgoing connection to a peer that it's already connected to.

Even if that's the case, I don't think the code handles the case when the new connection is also incoming. I think I can kind of see the logic in that might equally not happen if the remote does not attempt another connection to us, but there's also timing there. I wrote a little logic table to walk through the cases, new conn incoming/outgoing, existing conn incoming/outgoing, and who has the higher peer ID. In the case that the linked code is handling an incoming connection, and the existing connection is also incoming (per your assumption), then if our ID is higher than theirs, we keep the existing connection, and if our ID is lower, we keep the new connection. I can't quite see if it's correct, however I can see value in it if it's deterministic for both peers.

When you prefer the existing connection, both peers may end up closing different connections still, because that timing may be observed in different orders by the different peers.

Thanks, this is a valuable insight.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[anacrolix]

I think this is understandably a complex issue. It's difficult to test torrent clients, and sometimes the only indication that something is wrong is that the performance seems degraded or some other vague metric changes.

I changed anacrolix/torrent to prefer connections from higher peer IDs to lower peer IDs (completely 180 to what it was). I also made it balance incoming and outgoing connections so one kind doesn't start the other.

It seems to be working fine. I don't think I've noticed any different. This recent issue suggests that at least one guy is getting ridiculously good speeds 🤷🏻 .