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(autonatv2): Implement autonat v2 #1

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

Conversation

umgefahren
Copy link
Owner

@umgefahren umgefahren commented Nov 7, 2023

Description

This is a WIP implementation of AutoNATv2.

I would appreciate feedback by @mxinden and @thomaseizinger. You don't have to go into any specifics, I just wanted to check up if the direction is correct. A complete and formal PR will follow once everything is done.

  • 🚧 Client - ConnectionHandler (~80% done)
  • 🚧 Client - Behavior (~10% done)
  • 🌰 Server - ConnectionHandler
  • 🌰 Server - Behavior

I'm sorry it took me so long to give a sign of life. I sunk a lot of hours into doing it with request-response just to discover I need to do it without it. Additionally I couldn't work on it for the last week since I had to study for a midterm.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

…n if we already have a listener for outgoing connections.
@sukunrt
Copy link

sukunrt commented Jun 14, 2024

IPv6 should be handled specially because the majority of nodes do not support IPv6.

Event with 50% adoption and querying 3 nodes will have a 12.5% chance of an incorrect inference. As it stands adoption on the servers is closer to 10%.

This is what I observe on my kubo node, which has happy eyeballs to prefer ipv6 dialing.
ip4-ip6

To make matters worse, providing a service to the network which doesn't work for IPv6 will incentivize servers to not run IPv6.

A similar problem happens with UDP in some enterprise networks, but udp blocking is far less prevalent than not having IPv6 connectivity.

@thomaseizinger
Copy link

As I understand, this is an improvement we can ship for the server later right? At the moment, the algorithm for picking addresses and choosing servers is pretty simple, at least here in rust-libp2p.

What is important in my eyes is that the protocol allows us to change this later. For example, an AutoNAT client could choose to only send probes for IPv6 addresses to AutoNAT servers that it is already connected to via IPv6. Or a server could only pick an IPv6 address from a list if it has at least one IPv6 connection.

I'd advocate for shipping this as an improvement later. There are probably other optimisations that people want as they start using it but you have to ship a first version at some point.

@umgefahren
Copy link
Owner Author

I understand both arguments here. Although the guiding principal was to ship a simple, but correct version. If I could get the error matching right, I wouldn't have a problem with that. It should catch the can't dial IPv4 because I'm only IPv6 case too.

@thomaseizinger
Copy link

I still don't understand why this is such a problem. In my eyes, it is important that AutoNAT doesn't output false-positives, i.e. reports externally reachable when we are not. That is the problem with v1.

False-negatives, i.e. not detecting external reachability when we in fact are is not that big of a deal. It doesn't make it incorrect in my eyes, just not (yet) as useful as it could be.

Another thing is that IPv6 doesn't require NATing necessarily. You can configure your router to just directly assign an address from the block assigned by your ISP. That means the IPv6 address you read of the local interface is publicly routable and you don't need to discover it.

Whilst there is still use for an AutoNAT protocol for IPv6, I think it primarily solves a problem with IPv4 and thus I wouldn't spend much time on it now to optimise for running into less false-negatives.

@sukunrt
Copy link

sukunrt commented Jun 16, 2024

I'd advocate for shipping this as an improvement later.

Sure! I don't want to block this. Feel free to merge. We can add this improvement later.

For example, an AutoNAT client could choose to only send probes for IPv6 addresses to AutoNAT servers that it is already connected to via IPv6

This makes sense. We should implement this on the client too.

Whilst there is still use for an AutoNAT protocol for IPv6, I think it primarily solves a problem with IPv4 and thus I wouldn't spend much time on it now to optimise for running into less false-negatives.

AutoNAT only provides us with firewall status(We can change the name from autoNAT). Wouldn't most firewalls block both IPv6 and IPv4 incoming connections?

@thomaseizinger
Copy link

Whilst there is still use for an AutoNAT protocol for IPv6, I think it primarily solves a problem with IPv4 and thus I wouldn't spend much time on it now to optimise for running into less false-negatives.

AutoNAT only provides us with firewall status(We can change the name from autoNAT). Wouldn't most firewalls block both IPv6 and IPv4 incoming connections?

Hmm, I never saw it that way. To me, it was always about testing address "candidates" like observed external addresses where we weren't sure whether or not they are reachable.

We always send all listen addresses in identify so if there is no NAT with IPv6, there aren't any additional addresses that I need to test.

@umgefahren umgefahren changed the base branch from transport-redesign to master August 3, 2024 14:29
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Aug 8, 2024
Closes: #4524

This is the implementation of the evolved AutoNAT protocol, named AutonatV2 as defined in the [spec](https://github.com/libp2p/specs/blob/03718ef0f2dea4a756a85ba716ee33f97e4a6d6c/autonat/autonat-v2.md).
The stabilization PR for the spec can be found under libp2p/specs#538.

The work on the Rust implementation can be found in the PR to my fork: umgefahren#1.

The implementation has been smoke-tested with the Go implementation (PR: libp2p/go-libp2p#2469).

The new protocol addresses shortcomings of the original AutoNAT protocol:

- Since the server now always dials back over a newly allocated port, this made #4568 necessary; the client can be sure of the reachability state for other peers, even if the connection to the server was made through a hole punch.
- The server can now test addresses different from the observed address (i.e., the connection to the server was made through a `p2p-circuit`). To mitigate against DDoS attacks, the client has to send more data to the server than the dial-back costs.

Pull-Request: #5526.
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
Closes: libp2p#4524

This is the implementation of the evolved AutoNAT protocol, named AutonatV2 as defined in the [spec](https://github.com/libp2p/specs/blob/03718ef0f2dea4a756a85ba716ee33f97e4a6d6c/autonat/autonat-v2.md).
The stabilization PR for the spec can be found under libp2p/specs#538.

The work on the Rust implementation can be found in the PR to my fork: umgefahren#1.

The implementation has been smoke-tested with the Go implementation (PR: libp2p/go-libp2p#2469).

The new protocol addresses shortcomings of the original AutoNAT protocol:

- Since the server now always dials back over a newly allocated port, this made libp2p#4568 necessary; the client can be sure of the reachability state for other peers, even if the connection to the server was made through a hole punch.
- The server can now test addresses different from the observed address (i.e., the connection to the server was made through a `p2p-circuit`). To mitigate against DDoS attacks, the client has to send more data to the server than the dial-back costs.

Pull-Request: libp2p#5526.
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.

6 participants