Fixes from Ivan's second pass review

tests/factory/test_node_factory.nim

@@ -13,7 +13,9 @@ import
   tests/testlib/[wakunode, wakucore],
   waku/[waku_node, waku_enr, net/auto_port, discovery/waku_discv5, node/waku_metrics],
   waku/factory/[
-    node_factory, internal_config, conf_builder/conf_builder,
+    node_factory,
+    internal_config,
+    conf_builder/conf_builder,
     conf_builder/web_socket_conf_builder,
   ]
 

waku/discovery/waku_discv5.nim

@@ -478,7 +478,7 @@ proc setupAndStartDiscv5*(
       return err(error)
     let startRes = await wd.start()
     if startRes.isErr():
-      return err(startRes.error)
+      return err("failed to start discovery, attempt: " & startRes.error)
     return ok(wd)
 
   let wd = (await tryWithAutoPort[WakuDiscoveryV5](conf.udpPort, attempt)).valueOr:

waku/factory/waku.nim

@@ -204,7 +204,7 @@ proc new*(
 
   if not restServer.isNil():
     let boundRestPort = restServer.httpServer.address.port
-    node.ports.rest = some(boundRestPort.uint16)
+    node.ports.rest = boundRestPort.uint16
     wakuConf.restServerConf.get().port = boundRestPort
 
   # Set the extMultiAddrsOnly flag so the node knows not to replace explicit addresses
@@ -398,10 +398,8 @@ proc startWaku*(waku: ptr Waku): Future[Result[void, string]] {.async: (raises:
 
   let bound = getPorts(waku.node.switch.peerInfo.listenAddrs).valueOr:
     return err("failed to read bound ports from switch: " & $error)
-  if bound.tcpPort.isSome():
-    waku[].node.ports.tcp = some(bound.tcpPort.get().uint16)
-  if bound.websocketPort.isSome():
-    waku[].node.ports.webSocket = some(bound.websocketPort.get().uint16)
+  waku[].node.ports.tcp = bound.tcpPort.get(Port(0)).uint16
+  waku[].node.ports.webSocket = bound.websocketPort.get(Port(0)).uint16
 
   ## Discv5
   if conf.discv5Conf.isSome():
@@ -420,7 +418,7 @@ proc startWaku*(waku: ptr Waku): Future[Result[void, string]] {.async: (raises:
     ).valueOr:
       return err("failed to start waku discovery v5: " & error)
 
-    waku[].node.ports.discv5Udp = some(waku[].wakuDiscV5.udpPort.uint16)
+    waku[].node.ports.discv5Udp = waku[].wakuDiscV5.udpPort.uint16
     waku[].conf.discv5Conf.get().udpPort = waku[].wakuDiscV5.udpPort
 
   ## Update waku data that is set dynamically on node start
@@ -505,7 +503,7 @@ proc startWaku*(waku: ptr Waku): Future[Result[void, string]] {.async: (raises:
       ).valueOr:
         return err("Starting monitoring and external interfaces failed: " & error)
       waku[].metricsServer = server
-      waku[].node.ports.metrics = some(port.uint16)
+      waku[].node.ports.metrics = port.uint16
       waku[].conf.metricsServerConf.get().httpPort = port
     except CatchableError:
       return err(

waku/net/auto_port.nim

@@ -7,10 +7,10 @@ const
   AutoPortRetryCount* = 20
   AutoPortMin = 50000'u16
   AutoPortMax = 59000'u16
-
-var rng = initRand()
+  AutoPortAttemptTimeout = chronos.seconds(30)
 
 proc getAutoPort*(): uint16 =
+  var rng = initRand()
   uint16(rng.rand(AutoPortMin.int .. AutoPortMax.int))
 
 proc tryWithAutoPort*[T](
@@ -29,10 +29,20 @@ proc tryWithAutoPort*[T](
         Port(getAutoPort())
       else:
         startingPort
-    let res = await attempt(port)
+    let fut = attempt(port)
+    let res =
+      try:
+        if await fut.withTimeout(AutoPortAttemptTimeout):
+          await fut
+        else:
+          fut.cancelSoon()
+          Result[T, string].err("bind attempt timed out")
+      except CancelledError:
+        fut.cancelSoon()
+        Result[T, string].err("bind attempt cancelled")
     if res.isOk():
       return ok(res.get())
     lastErr = res.error
   if autoMode:
     return err("auto-port exhausted; last error: " & lastErr)
-  return err(lastErr)
+  return err("port bind failed: " & lastErr)

waku/net/bound_ports.nim

@@ -1,27 +1,20 @@
 {.push raises: [].}
 
-import std/[json, options]
+import std/json
 
 type BoundPorts* {.requiresInit.} = object
   ## Set by the factory once each service has bound to a port.
-  tcp*: Option[uint16]
-  webSocket*: Option[uint16]
-  rest*: Option[uint16]
-  discv5Udp*: Option[uint16]
-  metrics*: Option[uint16]
+  ## A value of 0 means the service was not enabled or did not bind.
+  tcp*: uint16
+  webSocket*: uint16
+  rest*: uint16
+  discv5Udp*: uint16
+  metrics*: uint16
 
 proc init*(T: type BoundPorts): BoundPorts =
-  BoundPorts(
-    tcp: none(uint16),
-    webSocket: none(uint16),
-    rest: none(uint16),
-    discv5Udp: none(uint16),
-    metrics: none(uint16),
+  return BoundPorts(
+    tcp: 0'u16, webSocket: 0'u16, rest: 0'u16, discv5Udp: 0'u16, metrics: 0'u16
   )
 
 proc toJsonString*(p: BoundPorts): string =
-  var obj = newJObject()
-  for name, value in fieldPairs(p):
-    if value.isSome():
-      obj[name] = %value.get()
-  return $obj
+  return $(%*p)

waku/node/waku_metrics.nim

@@ -67,12 +67,13 @@ proc startMetricsServer(
     info "Starting metrics HTTP server", serverIp = $serverIp, serverPort = $port
 
     let server = MetricsHttpServerRef.new($serverIp, port).valueOr:
-      return err($error)
+      return err("fail to start service metrics server, attempt:" & $error)
 
     try:
       await server.start()
     except CatchableError:
-      return err(getCurrentExceptionMsg())
+      return
+        err("exception while startMetricsServer, attempt: " & getCurrentExceptionMsg())
 
     info "Metrics HTTP server started", serverIp = $serverIp, serverPort = $port
     return ok((server: server, port: port))