feat: add upnp support #1022

qizhou · 2023-11-01T20:30:30Z

What was wrong?

Currently, Trin cannot support a private node running behind a firewall/gateway. Inspired by libp2p, this PR adds UPnP support, which is the default NAT used in libp2p.

How was it fixed?

UPnP protocol will ask the gateway to map a private IP/port to an external IP/port and return the external address to Trin. By encoding the external address in ENR, Trin will allow any node to access a Trin node running behind the gateway.

Test:

Run a Trin node behind a gateway with an empty bootnode. The node will print the ENR with an external IP/port mapped by UPnP.
Run another Trin node using the bootnode's ENR, which connects the bootnode.
Call portal_historyGossip (using test vector) from one node and check that another node receives the data.

To-Do

Clean up commit history and use conventional commits.

KolbyML · 2023-11-03T20:36:44Z

The guy at nimbus who was pushing for 4444s told me why aren't you guys just using upnp, so I think this is a good first direction.

As Qi said on discord in general channel we will probably end up supporting

UPnP (default)
rendezvous
relay

In that order of priority.

This will also majorly support our effort to having a large network.

Thank you Qi for looking into this I believe this PR solves or at least work towards #22 Carver can correct me if I am wrong on that

I guess I am saying I support this change

portalnet/src/socket.rs

ethportal-api/src/types/cli.rs

njgheorghita · 2023-11-10T10:00:28Z

ethportal-api/src/types/cli.rs

@@ -170,6 +177,7 @@ impl Default for TrinConfig {
            bootnodes: Bootnodes::Default,
            external_addr: None,
            no_stun: false,
+            no_upnp: false,


It seems like this means we're enabling upnp by default? Is that the desired behavior?

The behavior is borrowed from geth's default behavior, where UPnP is used by default (and there is no flag to disable it). Feel free to let me know if you have better thoughts.

I believe enable by default is the desired behavior

portalnet/src/discovery.rs

portalnet/src/socket.rs

Co-authored-by: Nick Gheorghita <[email protected]>

ogenev

Added some questions/comments. I think we should also remove the port mapping on ctr-c or panic. I don't think we have a panic hooks right now for the main Trin process but adding a remove port mapping function and a call in main.rs should be ok for now.

portalnet/src/socket.rs

ogenev · 2023-11-28T10:53:45Z

portalnet/src/socket.rs

+        "new_port",
+    ) {
+        Ok(()) => {
+            thread::spawn(move || loop {


What is the reason for spawning this thread and the loop in the background? If gateway.add_port returns Ok(()), the mapping should be successfull ?

This thread is used to renew the port periodically so that Trin can own the port as long as it is alive, while the port will be released in < 60 mins if Trin quits in any matter (crash or gracefully shutdown).

Ahh, it makes sense. It will be nice to have some comments with a short description of what is happening here.

Let's use tokio to spawn the thread with tokio sleep:

Suggested change

thread::spawn(move || loop {

tokio::spawn(async move { loop {

tokio::time::sleep(time::Duration::from_secs(UPNP_MAPPING_TIMEOUT)).await;

A comment is added.

I did some study on tokio::spawn, and I believe that it may not be a good way because gateway.add_port into the UPnP library is a blocking IO, which may block other async tasks. I tried to use tokio::tasks::spawn_blocking, but I encountered an issue that SIGINT (ctrl + c) cannot exit Trin gracefully. As a result, I have to revert to the dedicated thread method for this purpose.

Please let me know if you have better ideas.

Hmm, I didn't know that gateway.add_port is a blocking IO. In this case we have two options tokio spawn_blocking or a dedicated thread but because we expect this thread to run forever, I think std::thread::spawn is the way to go 👍 .

portalnet/src/socket.rs

ogenev · 2023-11-28T11:09:04Z

portalnet/src/socket.rs

 use tracing::{debug, info, warn};

 // This stun server is part of the testnet infrastructure.
 // If you are unable to connect, please create an issue.
 const STUN_SERVER: &str = "159.223.0.83:3478";

+/// The duration in seconds of an external port mapping by UPnP.
+const UPNP_MAPPING_DURATION: u32 = 3600;


Why are we mapping UPnP for only 60 minutes? I think we want to use an infinite value here, which in this case should be 0.

I am slightly in the camp of having a limit unless we're absolutely sure that these will expire after a period of disuse. We don't want to be opening ports up on people's home networks indefinitely. Seems worth the effort to implement periodic renewal of the mapping to make sure we aren't being bad citizens on user machines.

I am slightly in the camp of having a limit unless we're absolutely sure that these will expire after a period of disuse. We don't want to be opening ports up on people's home networks indefinitely. Seems worth the effort to implement periodic renewal of the mapping to make sure we aren't being bad citizens on user machines.

Agree. The current design will initially map the port for 60 minutes, and then periodically (every 30 minutes) renew the port for another 60 minutes. That said if Trin is down for no matter what reasons (crash or gracefully closed), the port reserved by Trin will be automatically released in < 60 minutes.

ogenev · 2023-11-28T11:38:45Z

portalnet/src/socket.rs

+        }
+    };
+
+    let local_ip = match TcpStream::connect(gateway.addr) {


I'm not sure if I'm in love with the idea of establishing a TCP stream to the gateway and getting the local IP address from there.

I don't know if https://docs.rs/local-ip-address/0.5.6/local_ip_address/fn.local_ip.html guarantees to return the correct IP address if more than one default interface is enabled. (if it is comparing the metric value from the routing table info, should be ok).

Actually, the previous version of the PR uses local_ip and local_ip_v6 to find the local IP address as below:

local_addr = match local_ip_address::local_ip().or(local_ip_address::local_ipv6()) ...

However, as @carver points out that this may be problematic if multiple interfaces exist. That is why I switch to a TCP way to detect the local IP.

Note that the UPnP protocol also uses an HTTP/TCP connection to the gateway. Unfortunately, the UPnP library does not return which local IP address is used to talk to the gateway. So, without modifying UPnP library, making a TCP connection to the gateway is an easy solution to determine the local IP address

ogenev · 2023-11-28T11:41:29Z

ethportal-api/src/types/cli.rs

+        group = "external-ips",
+        help = "Do not use UPnP to determine an external port."
+    )]
+    pub no_upnp: bool,


I would prefer allow_upnp with the default value true, I think it is less mentally overwhelming that way.

I tried to change it to allow_upnp, but looks like clap does not support setting a boolean flag to false. Below is the error message I got

$ trin --allow-upnp=false error: unexpected value 'false' for '--allow-upnp' found; no more were expected Usage: trin --allow-upnp For more information, try '--help'.

Co-authored-by: Ognyan Genev <[email protected]>

portalnet/src/socket.rs

ogenev

LGTM 👍

Co-authored-by: Ognyan Genev <[email protected]>

Qi Zhou and others added 5 commits November 1, 2023 16:07

add upnp support

38e0012

skip upnp if local ip == external ip

81597f6

pass fmt lint check

614fda7

upnp: check external_ip is local with all local ip's

f0a2ff8

upnp: internal port == external port

baa7b5c

KolbyML requested review from carver, njgheorghita, KolbyML and ogenev November 3, 2023 20:31

carver reviewed Nov 4, 2023

View reviewed changes

portalnet/src/socket.rs Outdated Show resolved Hide resolved

find local ip connected to the gateway

8a655e6

qizhou mentioned this pull request Nov 7, 2023

EthStorage Devs Meeting #55 Agenda ethstorage/pm#60

Closed

njgheorghita reviewed Nov 10, 2023

View reviewed changes

qizhou and others added 2 commits November 12, 2023 19:38

Update portalnet/src/socket.rs

a1aceec

Co-authored-by: Nick Gheorghita <[email protected]>

address comments

a8ef990

qizhou changed the title ~~add upnp support~~ feat: add upnp support Nov 13, 2023

ogenev reviewed Nov 28, 2023

View reviewed changes

qizhou and others added 6 commits November 28, 2023 23:38

Update portalnet/src/socket.rs

173a59a

Co-authored-by: Ognyan Genev <[email protected]>

Update portalnet/src/socket.rs

2bba09d

Co-authored-by: Ognyan Genev <[email protected]>

Update portalnet/src/socket.rs

edde84c

Co-authored-by: Ognyan Genev <[email protected]>

Update portalnet/src/socket.rs

a4f5b60

Co-authored-by: Ognyan Genev <[email protected]>

add renew comments

d359f79

revert to thread::spawn to address blocking IO issue

0b002c3

ogenev reviewed Nov 30, 2023

View reviewed changes

portalnet/src/socket.rs Outdated Show resolved Hide resolved

ogenev approved these changes Nov 30, 2023

View reviewed changes

Update portalnet/src/socket.rs

9918459

Co-authored-by: Ognyan Genev <[email protected]>

ogenev merged commit 3f24614 into ethereum:master Dec 1, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add upnp support #1022

feat: add upnp support #1022

qizhou commented Nov 1, 2023

KolbyML commented Nov 3, 2023

njgheorghita Nov 10, 2023

qizhou Nov 13, 2023

pipermerriam Nov 28, 2023

ogenev left a comment

ogenev Nov 28, 2023

qizhou Nov 29, 2023

ogenev Nov 29, 2023 •

edited

Loading

qizhou Nov 29, 2023

ogenev Nov 30, 2023

ogenev Nov 28, 2023

pipermerriam Nov 28, 2023

qizhou Nov 29, 2023

ogenev Nov 28, 2023

qizhou Nov 29, 2023 •

edited

Loading

ogenev Nov 28, 2023 •

edited

Loading

qizhou Nov 29, 2023

ogenev left a comment

	thread::spawn(move \|\| loop {
	tokio::spawn(async move { loop {
	tokio::time::sleep(time::Duration::from_secs(UPNP_MAPPING_TIMEOUT)).await;

feat: add upnp support #1022

feat: add upnp support #1022

Conversation

qizhou commented Nov 1, 2023

What was wrong?

How was it fixed?

To-Do

KolbyML commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qizhou Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

ogenev Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev left a comment

Choose a reason for hiding this comment

ogenev Nov 29, 2023 •

edited

Loading

qizhou Nov 29, 2023 •

edited

Loading

ogenev Nov 28, 2023 •

edited

Loading