refactor: proper use of setupNat (#1740)

Notice that I had to adapt to use 'rlpx_connected_peers' instead
of 'connected_peers' in 'wakunode1.nim' because due to the update
of the 'vendor/nim-eth', which adds the dependency-break with
'confutils' but also includes another changes.

Aside note: we cannot have 'confutils' dependency in 'nim-eth' because
that will prevent the generation of any waku dynamic library.

apps/chat2/chat2.nim

@@ -404,11 +404,15 @@ proc processInput(rfd: AsyncFD, rng: ref HmacDrbgContext) {.async.} =
   if conf.logLevel != LogLevel.NONE:
     setLogLevel(conf.logLevel)
 
-  let
+  let natRes = setupNat(conf.nat, clientId,
-    (extIp, extTcpPort, extUdpPort) = setupNat(conf.nat, clientId,
       Port(uint16(conf.tcpPort) + conf.portsShift),
       Port(uint16(conf.udpPort) + conf.portsShift))
 
+  if natRes.isErr():
+    raise newException(ValueError, "setupNat error " & natRes.error)
+
+  let (extIp, extTcpPort, extUdpPort) = natRes.get()
+
   let node = block:
       var builder = WakuNodeBuilder.init()
       builder.withNodeKey(nodeKey)

apps/chat2bridge/chat2bridge.nim

@@ -248,11 +248,15 @@ when isMainModule:
   if conf.logLevel != LogLevel.NONE:
     setLogLevel(conf.logLevel)
 
-  # Load address configuration
+  let natRes = setupNat(conf.nat, clientId,
-  let
-    (nodev2ExtIp, nodev2ExtPort, _) = setupNat(conf.nat, clientId,
                         Port(uint16(conf.libp2pTcpPort) + conf.portsShift),
                         Port(uint16(conf.udpPort) + conf.portsShift))
+  if natRes.isErr():
+    error "Error in setupNat", error = natRes.error
+
+  # Load address configuration
+  let
+    (nodev2ExtIp, nodev2ExtPort, _) = natRes.get()
     ## The following heuristic assumes that, in absence of manual
     ## config, the external port is the same as the bind port.
     extPort = if nodev2ExtIp.isSome() and nodev2ExtPort.isNone():

apps/wakubridge/wakubridge.nim

@@ -349,11 +349,23 @@ when isMainModule:
   ## actually a supported transport.
   let udpPort = conf.devp2pTcpPort
 
-  # Load address configuration
+  let natRes = setupNat(conf.nat, ClientIdV1,
-  let
-    (nodev1ExtIp, _, _) = setupNat(conf.nat, ClientIdV1,
                         Port(conf.devp2pTcpPort + conf.portsShift),
                         Port(udpPort + conf.portsShift))
+  if natRes.isErr():
+    error "failed setupNat", error = natRes.error
+    quit(QuitFailure)
+
+  let natRes2 = setupNat(conf.nat, clientId,
+                         Port(uint16(conf.libp2pTcpPort) + conf.portsShift),
+                         Port(uint16(udpPort) + conf.portsShift))
+  if natRes2.isErr():
+    error "failed setupNat", error = natRes2.error
+    quit(QuitFailure)
+
+  # Load address configuration
+  let
+    (nodev1ExtIp, _, _) = natRes.get()
     # TODO: EthereumNode should have a better split of binding address and
     # external address. Also, can't have different ports as it stands now.
     nodev1Address = if nodev1ExtIp.isNone():
@@ -364,9 +376,7 @@ when isMainModule:
                       Address(ip: nodev1ExtIp.get(),
                               tcpPort: Port(conf.devp2pTcpPort + conf.portsShift),
                               udpPort: Port(udpPort + conf.portsShift))
-    (nodev2ExtIp, nodev2ExtPort, _) = setupNat(conf.nat, clientId,
+    (nodev2ExtIp, nodev2ExtPort, _) = natRes2.get()
-                                               Port(uint16(conf.libp2pTcpPort) + conf.portsShift),
-                                               Port(uint16(udpPort) + conf.portsShift))
 
   # Topic interest and bloom
   var topicInterest: Option[seq[waku_protocol.Topic]]

apps/wakunode2/app.nim

@@ -13,12 +13,12 @@ import
   libp2p/protocols/pubsub/gossipsub,
   libp2p/peerid,
   eth/keys,
-  eth/net/nat,
   json_rpc/rpcserver,
   presto,
   metrics,
   metrics/chronos_httpserver
 import
+  ../../waku/common/utils/nat,
   ../../waku/common/sqlite,
   ../../waku/v2/waku_core,
   ../../waku/v2/waku_node,
@@ -317,51 +317,6 @@ proc setupDyamicBootstrapNodes*(app: var App): AppResult[void] =
 
 ## Init waku node instance
 
-proc setupNat(natConf, clientId: string, tcpPort, udpPort: Port):
-  AppResult[tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]]] {.gcsafe.} =
-
-  let strategy = case natConf.toLowerAscii():
-      of "any": NatAny
-      of "none": NatNone
-      of "upnp": NatUpnp
-      of "pmp": NatPmp
-      else: NatNone
-
-  var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]]
-
-  if strategy != NatNone:
-    let extIp = getExternalIP(strategy)
-    if extIP.isSome():
-      endpoint.ip = some(ValidIpAddress.init(extIp.get()))
-      # RedirectPorts in considered a gcsafety violation
-      # because it obtains the address of a non-gcsafe proc?
-      var extPorts: Option[(Port, Port)]
-      try:
-        extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort,
-                                              udpPort = udpPort,
-                                              description = clientId))
-      except CatchableError:
-        # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now.
-        error "unable to determine external ports"
-        extPorts = none((Port, Port))
-
-      if extPorts.isSome():
-        let (extTcpPort, extUdpPort) = extPorts.get()
-        endpoint.tcpPort = some(extTcpPort)
-        endpoint.udpPort = some(extUdpPort)
-
-  else: # NatNone
-    if not natConf.startsWith("extip:"):
-      return err("not a valid NAT mechanism: " & $natConf)
-
-    try:
-      # any required port redirection is assumed to be done by hand
-      endpoint.ip = some(ValidIpAddress.init(natConf[6..^1]))
-    except ValueError:
-      return err("not a valid IP address: " & $natConf[6..^1])
-
-  return ok(endpoint)
-
 proc initNode(conf: WakuNodeConf,
               rng: ref HmacDrbgContext,
               peerStore: Option[WakuPeerStorage],
@@ -401,7 +356,6 @@ proc initNode(conf: WakuNodeConf,
 
   let (extIp, extTcpPort, _) = natRes.get()
 
-
   let
     dns4DomainName = if conf.dns4DomainName != "": some(conf.dns4DomainName)
                       else: none(string)

examples/v1/example.nim

@@ -11,10 +11,16 @@ import
 const clientId = "Waku example v1"
 
 proc run(config: WakuNodeConf, rng: ref HmacDrbgContext) =
-  # Set up the address according to NAT information.
+
-  let (ipExt, tcpPortExt, udpPortExt) = setupNat(config.nat, clientId,
+  let natRes = setupNat(config.nat, clientId,
                         Port(config.tcpPort + config.portsShift),
                         Port(config.udpPort + config.portsShift))
+  if natRes.isErr():
+    fatal "setupNat failed", error = natRes.error
+    quit(1)
+
+  # Set up the address according to NAT information.
+  let (ipExt, tcpPortExt, udpPortExt) = natRes.get()
   # TODO: EthereumNode should have a better split of binding address and
   # external address. Also, can't have different ports as it stands now.
   let address = if ipExt.isNone():

library/libwaku.nim

@@ -7,8 +7,7 @@ import
   chronicles,
   chronos,
   libp2p/crypto/secp,
-  stew/shims/net,
+  stew/shims/net
-  eth/net/nat as todo_delete_this_module
 import
   ../vendor/nim-libp2p/libp2p/crypto/crypto,
   ../../waku/common/utils/nat,
@@ -81,52 +80,6 @@ proc errResp(message: string): string =
   # }
   return $(%* { "error": message })
 
-proc setupNat(natConf, clientId: string, tcpPort, udpPort: Port):
-            Result[tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]], string] {.gcsafe.} =
-  # TODO reorganize all setupNat calls and use only one commom proc
-
-  let strategy = case natConf.toLowerAscii():
-      of "any": NatAny
-      of "none": NatNone
-      of "upnp": NatUpnp
-      of "pmp": NatPmp
-      else: NatNone
-
-  var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]]
-
-  if strategy != NatNone:
-    let extIp = getExternalIP(strategy)
-    if extIP.isSome():
-      endpoint.ip = some(ValidIpAddress.init(extIp.get()))
-      # RedirectPorts in considered a gcsafety violation
-      # because it obtains the address of a non-gcsafe proc?
-      var extPorts: Option[(Port, Port)]
-      try:
-        extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort,
-                                              udpPort = udpPort,
-                                              description = clientId))
-      except CatchableError:
-        # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now.
-        error "unable to determine external ports"
-        extPorts = none((Port, Port))
-
-      if extPorts.isSome():
-        let (extTcpPort, extUdpPort) = extPorts.get()
-        endpoint.tcpPort = some(extTcpPort)
-        endpoint.udpPort = some(extUdpPort)
-
-  else: # NatNone
-    if not natConf.startsWith("extip:"):
-      return err("not a valid NAT mechanism: " & $natConf)
-
-    try:
-      # any required port redirection is assumed to be done by hand
-      endpoint.ip = some(ValidIpAddress.init(natConf[6..^1]))
-    except ValueError:
-      return err("not a valid IP address: " & $natConf[6..^1])
-
-  return ok(endpoint)
-
 proc parseConfig(config: ConfigNode,
                  privateKey: var PrivateKey,
                  netConfig: var NetConfig,

tests/v1/test_waku_connect.nim

@@ -58,9 +58,9 @@ procSuite "Waku connections":
       p2 = await n2.rlpxConnect(newNode(n3.toENode()))
       p3 = await n4.rlpxConnect(newNode(n3.toENode()))
     check:
-      p1.isNil
+      p1.isErr() == true
-      p2.isNil == false
+      p2.isErr() == false
-      p3.isNil == false
+      p3.isErr() == false
 
   asyncTest "Filters with encryption and signing":
     var node1 = setupTestNode(rng, Waku)
@@ -386,7 +386,7 @@ procSuite "Waku connections":
 
     ln2.startListening()
     let peer = await ln1.rlpxConnect(newNode(ln2.toENode()))
-    check peer.isNil == true
+    check peer.isErr() == true
 
   asyncTest "Waku set-topic-interest":
     var

tests/whisper/test_shh_connect.nim

@@ -326,4 +326,4 @@ procSuite "Whisper connections":
 
     ln2.startListening()
     let peer = await ln1.rlpxConnect(newNode(ln2.toENode()))
-    check peer.isNil == true
+    check peer.isErr() == true

vendor/nim-eth

@@ -1 +1 @@
-Subproject commit 72c98589278aec949c13435d9bcacdb306faa5a8
+Subproject commit 285da12bf318a2e21182dc2453a4a30c58f73067

vendor/nim-secp256k1

@@ -1 +1 @@
-Subproject commit 4c41c5029ffc73b732233f06018cd52f3ed47dce
+Subproject commit 5fd81357839d57ef38fb17647bd5e31dfa9f55b8

waku/common/utils/nat.nim

@@ -1,70 +1,67 @@
+
 when (NimMajor, NimMinor) < (1, 4):
   {.push raises: [Defect].}
 else:
   {.push raises: [].}
 
 import
-  std/[strutils, options],
+  std/[options, strutils]
-  chronicles, stew/shims/net as stewNet,
+import
-  eth/net/nat
+  chronicles,
+  eth/net/nat,
+  stew/results,
+  stew/shims/net,
+  nativesockets
 
 logScope:
   topics = "nat"
 
-proc setupNat*(natConf, clientId: string, tcpPort, udpPort: Port):
+proc setupNat*(natConf, clientId: string,
-    tuple[ip: Option[ValidIpAddress],
+               tcpPort, udpPort: Port):
+               Result[tuple[ip: Option[ValidIpAddress],
                             tcpPort: Option[Port],
-          udpPort: Option[Port]] {.gcsafe, deprecated:
+                            udpPort: Option[Port]], string]
-  "Unsafe: this proc quits the app if something is not ok".} =
+               {.gcsafe.} =
 
-  var
+  let strategy = case natConf.toLowerAscii():
-    endpoint: tuple[ip: Option[ValidIpAddress],
+      of "any": NatAny
-                    tcpPort: Option[Port],
+      of "none": NatNone
-                    udpPort: Option[Port]]
+      of "upnp": NatUpnp
-    nat: NatStrategy
+      of "pmp": NatPmp
+      else: NatNone
 
-  case natConf.toLowerAscii:
+  var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]]
-    of "any":
-      nat = NatAny
-    of "none":
-      nat = NatNone
-    of "upnp":
-      nat = NatUpnp
-    of "pmp":
-      nat = NatPmp
-    else:
-      if natConf.startsWith("extip:"):
-        try:
-          # any required port redirection is assumed to be done by hand
-          endpoint.ip = some(ValidIpAddress.init(natConf[6..^1]))
-          nat = NatNone
-        except ValueError:
-          error "not a valid IP address", address = natConf[6..^1]
-          quit QuitFailure
-      else:
-        error "not a valid NAT mechanism", value = natConf
-        quit QuitFailure
 
-  if nat != NatNone:
+  if strategy != NatNone:
-    let extIp = getExternalIP(nat)
+    let extIp = getExternalIP(strategy)
-    if extIP.isSome:
+    if extIP.isSome():
-      endpoint.ip = some(ValidIpAddress.init extIp.get)
+      endpoint.ip = some(ValidIpAddress.init(extIp.get()))
-      # TODO redirectPorts in considered a gcsafety violation
+      # RedirectPorts in considered a gcsafety violation
       # because it obtains the address of a non-gcsafe proc?
       var extPorts: Option[(Port, Port)]
       try:
-        extPorts = ({.gcsafe.}:
+        extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort,
-                    redirectPorts(tcpPort = tcpPort,
                                               udpPort = udpPort,
                                               description = clientId))
-      except Exception:
+      except CatchableError:
-        # @TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now.
+        # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now.
         error "unable to determine external ports"
         extPorts = none((Port, Port))
 
-      if extPorts.isSome:
+      if extPorts.isSome():
         let (extTcpPort, extUdpPort) = extPorts.get()
         endpoint.tcpPort = some(extTcpPort)
         endpoint.udpPort = some(extUdpPort)
 
-  return endpoint
+  else: # NatNone
+    if not natConf.startsWith("extip:"):
+      return err("not a valid NAT mechanism: " & $natConf)
+
+    try:
+      # any required port redirection is assumed to be done by hand
+      endpoint.ip = some(ValidIpAddress.init(natConf[6..^1]))
+    except ValueError:
+      return err("not a valid IP address: " & $natConf[6..^1])
+
+  return ok(endpoint)
+

waku/v1/node/wakunode1.nim

@@ -21,10 +21,15 @@ proc run(config: WakuNodeConf, rng: ref HmacDrbgContext)
   ## `udpPort` is only supplied to satisfy underlying APIs but is not
   ## actually a supported transport.
   let udpPort = config.tcpPort
-  let
+  let natRes = setupNat(config.nat, clientId,
-    (ipExt, tcpPortExt, _) = setupNat(config.nat, clientId,
                         Port(config.tcpPort + config.portsShift),
                         Port(udpPort + config.portsShift))
+  if natRes.isErr():
+    fatal "setupNat failed", error = natRes.error
+    quit(1)
+
+  let
+    (ipExt, tcpPortExt, _) = natRes.get()
   # TODO: EthereumNode should have a better split of binding address and
   # external address. Also, can't have different ports as it stands now.
     address = if ipExt.isNone():
@@ -119,7 +124,7 @@ proc run(config: WakuNodeConf, rng: ref HmacDrbgContext)
     logMetrics = proc(udata: pointer) =
       {.gcsafe.}:
         let
-          connectedPeers = connected_peers
+          connectedPeers = rlpx_connected_peers
           validEnvelopes = waku_protocol.envelopes_valid
           droppedEnvelopes = waku_protocol.envelopes_dropped