feat(nat): retry nat detction with UPnP

sn_nat_detection/src/behaviour/autonat.rs

@@ -1,9 +1,9 @@
 use libp2p::autonat;
 use tracing::{debug, info, warn};
 
-use crate::EventLoop;
+use crate::App;
 
-impl EventLoop {
+impl App {
     pub(crate) fn on_event_autonat(&mut self, event: autonat::Event) {
         match event {
             autonat::Event::InboundProbe(event) => match event {

sn_nat_detection/src/behaviour/identify.rs

@@ -2,9 +2,9 @@ use libp2p::{autonat, identify};
 use sn_networking::multiaddr_is_global;
 use tracing::{debug, info, warn};
 
-use crate::{behaviour::PROTOCOL_VERSION, EventLoop};
+use crate::{behaviour::PROTOCOL_VERSION, App};
 
-impl EventLoop {
+impl App {
     pub(crate) fn on_event_identify(&mut self, event: identify::Event) {
         match event {
             identify::Event::Received { peer_id, info } => {

sn_nat_detection/src/behaviour/mod.rs

@@ -19,7 +19,11 @@ pub(crate) struct Behaviour {
 }
 
 impl Behaviour {
-    pub(crate) fn new(local_public_key: identity::PublicKey, client_mode: bool) -> Self {
+    pub(crate) fn new(
+        local_public_key: identity::PublicKey,
+        client_mode: bool,
+        upnp: bool,
+    ) -> Self {
         let far_future = Duration::MAX / 10; // `MAX` on itself causes overflows. This is a workaround.
         Self {
             autonat: libp2p::autonat::Behaviour::new(
@@ -28,8 +32,13 @@ impl Behaviour {
                     libp2p::autonat::Config {
                         // Use dialed peers for probing.
                         use_connected: true,
-                        // Start probing 3 seconds after swarm init. This gives us time to connect to the dialed server.
-                        boot_delay: Duration::from_secs(3),
+                        // Start probing a few seconds after swarm init. This gives us time to connect to the dialed server.
+                        // With UPnP enabled, give it a bit more time to possibly open up the port.
+                        boot_delay: if upnp {
+                            Duration::from_secs(7)
+                        } else {
+                            Duration::from_secs(3)
+                        },
                         // Reuse probe server immediately even if it's the only one.
                         throttle_server_period: Duration::ZERO,
                         retry_interval: Duration::from_secs(10),
@@ -56,7 +65,7 @@ impl Behaviour {
                 // Exchange information every 5 minutes.
                 .with_interval(Duration::from_secs(5 * 60)),
             ),
-            upnp: Toggle::from(Some(libp2p::upnp::tokio::Behaviour::default())),
+            upnp: upnp.then(libp2p::upnp::tokio::Behaviour::default).into(),
         }
     }
 }

sn_nat_detection/src/behaviour/upnp.rs

@@ -1,9 +1,10 @@
 use libp2p::upnp;
-use tracing::{debug, info, warn};
+use tracing::{debug, info};
+use tracing_log::log::error;
 
-use crate::EventLoop;
+use crate::App;
 
-impl EventLoop {
+impl App {
     pub(crate) fn on_event_upnp(&mut self, event: upnp::Event) {
         match event {
             upnp::Event::NewExternalAddr(addr) => {
@@ -13,10 +14,10 @@ impl EventLoop {
                 debug!(%addr, "UPnP: External address expired");
             }
             upnp::Event::GatewayNotFound => {
-                warn!("UPnP: Gateway not found");
+                error!("UPnP: No gateway not found");
             }
             upnp::Event::NonRoutableGateway => {
-                warn!("UPnP: Gateway is not routable");
+                error!("UPnP: Gateway is not routable");
             }
         }
     }

sn_nat_detection/src/main.rs

@@ -49,6 +49,10 @@ struct Opt {
     #[clap(name = "SERVER", value_name = "multiaddr", value_delimiter = ',', value_parser = parse_peer_addr)]
     server_addr: Vec<Multiaddr>,
 
+    /// Disable use of UPnP to open a port on the router, before detecting NAT status.
+    #[clap(long, short, default_value_t = false)]
+    no_upnp: bool,
+
     #[command(flatten)]
     verbose: clap_verbosity_flag::Verbosity,
 }
@@ -70,48 +74,39 @@ async fn main() -> Result<(), Box<dyn Error>> {
         registry.with(filter).try_init()
     };
 
-    // If no servers are provided, we are in server mode. Conversely, with servers
-    // provided, we are in client mode.
-    let client_mode = !opt.server_addr.is_empty();
-
-    let mut swarm = libp2p::SwarmBuilder::with_new_identity()
-        .with_tokio()
-        .with_tcp(
-            tcp::Config::default(),
-            noise::Config::new,
-            yamux::Config::default,
-        )?
-        .with_behaviour(|key| Behaviour::new(key.public(), client_mode))?
-        // Make it so that we retry just before idling out, to prevent quickly disconnecting/connecting
-        // to the same server.
-        .with_swarm_config(|c| {
-            c.with_idle_connection_timeout(RETRY_INTERVAL + Duration::from_secs(2))
-        })
-        .build();
-
-    swarm.listen_on(
-        Multiaddr::empty()
-            .with(Protocol::Ip4(Ipv4Addr::UNSPECIFIED))
-            .with(Protocol::Tcp(opt.port)),
-    )?;
-
-    info!(
-        peer_id=%swarm.local_peer_id(),
-        "starting in {} mode",
-        if client_mode { "client" } else { "server" }
-    );
-
-    let event_loop = EventLoop::new(swarm, opt.server_addr);
-    // The main loop will exit once it has gained enough confidence in the NAT status.
-    let status = event_loop.run().await;
-
-    match status {
-        NatStatus::Public(addr) => {
-            info!(%addr, "NAT is public");
-            Ok(())
+    let mut builder = AppBuilder::new()
+        .servers(opt.server_addr)
+        .upnp(false)
+        .port(opt.port);
+
+    // Run the program twice, to first detect NAT status without UPnP,
+    // and then with UPnP enabled. (Unless `--no-upnp` was given.)
+    let mut running_with_upnp = false;
+    loop {
+        let status = builder
+            .build()?
+            // The main loop will exit once it has gained enough confidence in the NAT status.
+            .run()
+            .await;
+
+        match status {
+            NatStatus::Public(addr) => {
+                info!(%addr, "NAT is public{}", if running_with_upnp { " (with UPnP)" } else { "" });
+                break Ok(());
+            }
+            NatStatus::Private => {
+                // Unless `--no-upnp` is set, rerun the program with UPnP enabled.
+                if !opt.no_upnp && !running_with_upnp {
+                    warn!("NAT is private, rerunning program with UPnP enabled in 2 seconds...");
+                    tokio::time::sleep(Duration::from_secs(2)).await;
+                    builder = builder.upnp(true);
+                    running_with_upnp = true;
+                } else {
+                    break Err("NAT is private".into());
+                }
+            }
+            NatStatus::Unknown => break Err("NAT is unknown".into()),
         }
-        NatStatus::Private => Err("NAT is private".into()),
-        NatStatus::Unknown => Err("NAT is unknown".into()),
     }
 }
 
@@ -127,7 +122,7 @@ enum State {
     Done(NatStatus),
 }
 
-struct EventLoop {
+struct App {
     swarm: libp2p::Swarm<Behaviour>,
     // Interval with which to check the state of the program. (State is also checked on events.)
     interval: tokio::time::Interval,
@@ -137,7 +132,7 @@ struct EventLoop {
     candidate_addrs: HashSet<Multiaddr>,
 }
 
-impl EventLoop {
+impl App {
     fn new(swarm: libp2p::Swarm<Behaviour>, servers: Vec<Multiaddr>) -> Self {
         Self {
             swarm,
@@ -335,3 +330,71 @@ fn parse_peer_addr(addr: &str) -> Result<Multiaddr, &'static str> {
 
     Err("could not parse address")
 }
+
+struct AppBuilder {
+    port: u16,
+    servers: Vec<Multiaddr>,
+    upnp: bool,
+}
+
+impl AppBuilder {
+    fn new() -> Self {
+        Self {
+            port: 0,
+            upnp: false,
+            servers: vec![],
+        }
+    }
+
+    fn port(mut self, port: u16) -> Self {
+        self.port = port;
+        self
+    }
+
+    fn upnp(mut self, upnp: bool) -> Self {
+        self.upnp = upnp;
+        self
+    }
+
+    fn servers(mut self, servers: Vec<Multiaddr>) -> Self {
+        self.servers = servers;
+        self
+    }
+
+    fn build(&self) -> Result<App, Box<dyn Error>> {
+        // If no servers are provided, we are in server mode. Conversely, with servers
+        // provided, we are in client mode.
+        let client_mode = !self.servers.is_empty();
+
+        let mut swarm = libp2p::SwarmBuilder::with_new_identity()
+            .with_tokio()
+            .with_tcp(
+                tcp::Config::default(),
+                noise::Config::new,
+                yamux::Config::default,
+            )?
+            .with_behaviour(|key| Behaviour::new(key.public(), client_mode, self.upnp))?
+            // Make it so that we retry just before idling out, to prevent quickly disconnecting/connecting
+            // to the same server.
+            .with_swarm_config(|c| {
+                c.with_idle_connection_timeout(RETRY_INTERVAL + Duration::from_secs(2))
+            })
+            .build();
+
+        swarm.listen_on(
+            Multiaddr::empty()
+                .with(Protocol::Ip4(Ipv4Addr::UNSPECIFIED))
+                .with(Protocol::Tcp(self.port)),
+        )?;
+
+        info!(
+            peer_id=%swarm.local_peer_id(),
+            "starting in {} mode",
+            if client_mode { "client" } else { "server" }
+        );
+
+        let app = App::new(swarm, self.servers.clone());
+
+        Ok(app)
+    }
+}