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

feat: add connection selection logic #2726

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

wlynxg
Copy link
Contributor

@wlynxg wlynxg commented Mar 8, 2024

Add connection comparison logic and set more preferences to choose better connections:

  1. LAN > WAN > PROXY;
  2. Prefer udp based connection over tcp based;

@sukunrt
Copy link
Member

sukunrt commented Mar 8, 2024

Won't both LAN and WAN have the same performance?
I am not sure if the WAN connection between two nodes that have a LAN connection would be routed over the public internet except in some edge cases.

@wlynxg
Copy link
Contributor Author

wlynxg commented Mar 8, 2024

Won't both LAN and WAN have the same performance? I am not sure if the WAN connection between two nodes that have a LAN connection would be routed over the public internet except in some edge cases.

If the connection between two nodes with a LAN connection is via a WAN address, two situations can occur:

  1. The router does not have hairpin capability and the connection cannot be successful;
  2. The router has hairpin capability. The router will modify the dst ip and then send the data packet out.
    All WAN IP packets need to be modified through the hairpin mode of the router, which will put a certain amount of pressure on the router.

About hairpin mode: https://datatracker.ietf.org/doc/html/rfc5128#autoid-12

p2p/net/swarm/swarm.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

I don't really understand the point of this, when is this code called ?

As far I can tell this code is only used when switching from a transient / relayed connection to a direct one after doing hole punching. Which was broken in this PR.
As the dialer already implements similar logic but with inclusion of RTT in the mix.

p2p/net/swarm/swarm.go Outdated Show resolved Hide resolved
@wlynxg
Copy link
Contributor Author

wlynxg commented Mar 8, 2024

I don't really understand the point of this, when is this code called ?

As far I can tell this code is only used when switching from a transient / relayed connection to a direct one after doing hole punching. Which was broken in this PR. As the dialer already implements similar logic but with inclusion of RTT in the mix.

Each time NewStream is executed, swarm will find the best connection and then create the stream:

c := s.bestConnToPeer(p)

@Jorropo
Copy link
Contributor

Jorropo commented Mar 8, 2024

@wlynxg we are trying to not get into a situation where we have more than one connection in the first place.

For example the dialler cancel dialling once we got a success. I've meant to link the code that does this but that harder than it needs to be because the dialler use the worker goroutine pattern.

@wlynxg
Copy link
Contributor Author

wlynxg commented Mar 8, 2024

we are trying to not get into a situation where we have more than one connection in the first place.

For example the dialler cancel dialling once we got a success. I've meant to link the code that does this but that harder than it needs to be because the dialler use the worker goroutine pattern.

The strategy I adopted when implementing p2p myself was to first sort the target addresses according to priority, and then perform traversal connections until one succeeds.

To some extent, maintaining multiple connections is not a bad thing. It can quickly switch to a connection with another address when one connection is unavailable. In the implementation of tailscale, it also adopts a multi-link strategy, but it also sets up a dedicated goroutine to regularly perform RTT speed testing on each connection, and then selects the best connection.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 8, 2024

@wlynxg I assumed too much mb.
Your idea is good, multiple connections have cases where they are great indeed as you explained.

The problem I have with this code is that it's not how go-libp2p operates, your current code looks correct now (as non Transient connections have priority).
But it also doesn't do anything in 99% of the cases, you are fixing a bug that does not yet exists because we don't even try to have multiple connections right now. Do you have more plans to change other things in future pull requests ?


On a personal note, I do want multiple connections but not at the libp2p layer because they leak to applications, generate stream resets and require applications.
Instead I think this makes more sense to use QUIC Connection Migrations and MPQUIC to do that on a per packet level in the QUIC transport.


There is libp2p/specs#328 on the subject but I believe work there is halted.

p2p/net/swarm/swarm.go Outdated Show resolved Hide resolved
@wlynxg
Copy link
Contributor Author

wlynxg commented Mar 8, 2024

@wlynxg I assumed too much mb. Your idea is good, multiple connections have cases where they are great indeed as you explained.

The problem I have with this code is that it's not how go-libp2p operates, your current code looks correct now (as non Transient connections have priority). But it also doesn't do anything in 99% of the cases, you are fixing a bug that does not yet exists because we don't even try to have multiple connections right now. Do you have more plans to change other things in future pull requests ?

On a personal note, I do want multiple connections but not at the libp2p layer because they leak to applications, generate stream resets and require applications. Instead I think this makes more sense to use QUIC Connection Migrations and MPQUIC to do that on a per packet level in the QUIC transport.

There is libp2p/specs#328 on the subject but I believe work there is halted.

I think the best performance of multi connections is like this:
Let's perform another upper-level abstraction on the stream. The stream is not bound to a fixed connection. It can freely choose what conn is used at the bottom for transmission.
However, this requires fragmentation of data packets for reliable transmission, which will increase a lot of workload😥...

@Jorropo
Copy link
Contributor

Jorropo commented Mar 8, 2024

However, this requires fragmentation of data packets for reliable transmission, which will increase a lot of workload

libp2p/specs#328 libp2p/specs#406 exists for theses purposes

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.

None yet

3 participants