Allow for tcp/udp ports to always be configured (#402)

* Allow for tcp/udp ports to always be configured

- Allow for an ENR to be build with tcp and udp ports also when
no IP address is provided
- In the address set-up always provide best efforttcp and udp ports
also when configuration of external ip (and/or ports) fails.

eth/net/nat.nim

@@ -240,7 +240,10 @@ proc redirectPorts*(tcpPort, udpPort: Port, description: string): Option[(Port,
 proc setupNat*(natStrategy: NatStrategy, tcpPort, udpPort: Port,
     clientId: string):
     tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]] =
-
+  ## Setup NAT port mapping and get external IP address.
+  ## If any of this fails, we don't return any IP address but do return the
+  ## original ports as best effort.
+  ## TODO: Allow for tcp or udp port mapping to be optional.
   let extIp = getExternalIP(natStrategy)
   if extIP.isSome:
     let ip = ValidIpAddress.init(extIp.get)
@@ -250,13 +253,13 @@ proc setupNat*(natStrategy: NatStrategy, tcpPort, udpPort: Port,
                     description = clientId))
     if extPorts.isSome:
       let (extTcpPort, extUdpPort) = extPorts.get()
-      (some(ip), some(extTcpPort), some(extUdpPort))
+      (ip: some(ip), tcpPort: some(extTcpPort), udpPort: some(extUdpPort))
     else:
-      error "UPnP/NAT-PMP available but port forwarding failed"
+      warn "UPnP/NAT-PMP available but port forwarding failed"
-      (none(ValidIpAddress), none(Port), none(Port))
+      (ip: none(ValidIpAddress), tcpPort: some(tcpPort), udpPort: some(udpPort))
   else:
     warn "UPnP/NAT-PMP not available"
-    (none(ValidIpAddress), none(Port), none(Port))
+    (ip: none(ValidIpAddress), tcpPort: some(tcpPort), udpPort: some(udpPort))
 
 type
   NatConfig* = object
@@ -293,6 +296,11 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress,
     tcpPort, udpPort: Port, clientId: string):
     tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]]
     {.gcsafe.} =
+  ## Set-up of the external address via any of the ways as configured in
+  ## `NatConfig`. In case all fails an error is logged and the bind ports are
+  ## selected also as external ports, as best effort and in hope that the
+  ## external IP can be figured out by other means at a later stage.
+  ## TODO: Allow for tcp or udp bind ports to be optional.
 
   if natConfig.hasExtIp:
     # any required port redirection must be done by hand
@@ -307,7 +315,7 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress,
           # No route was found, log error and continue without IP.
           error "No routable IP address found, check your network connection",
             error = ip.error
-          return (none(ValidIpAddress), none(Port), none(Port))
+          return (none(ValidIpAddress), some(tcpPort), some(udpPort))
         elif ip.get().isPublic():
           return (some(ip.get()), some(tcpPort), some(udpPort))
         else:
@@ -329,17 +337,17 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress,
           # No route was found, log error and continue without IP.
           error "No routable IP address found, check your network connection",
             error = ip.error
-          return (none(ValidIpAddress), none(Port), none(Port))
+          return (none(ValidIpAddress), some(tcpPort), some(udpPort))
         elif ip.get().isPublic():
           return (some(ip.get()), some(tcpPort), some(udpPort))
         else:
           error "No public IP address found. Should not use --nat:none option"
-          return (none(ValidIpAddress), none(Port), none(Port))
+          return (none(ValidIpAddress), some(tcpPort), some(udpPort))
       elif bindAddress.isPublic():
         # When a specific public interface is provided, use that one.
         return (some(ValidIpAddress.init(bindIP)), some(tcpPort), some(udpPort))
       else:
         error "Bind IP is not a public IP address. Should not use --nat:none option"
-        return (none(ValidIpAddress), none(Port), none(Port))
+        return (none(ValidIpAddress), some(tcpPort), some(udpPort))
     of NatUpnp, NatPmp:
       return setupNat(natConfig.nat, tcpPort, udpPort, clientId)

eth/p2p/discoveryv5/enr.nim

@@ -152,7 +152,9 @@ template toFieldPair*(key: string, value: auto): FieldPair =
 
 proc addAddress(fields: var seq[FieldPair], ip: Option[ValidIpAddress],
     tcpPort, udpPort: Option[Port]) =
-  # It makes sense to add ports only when there is an IP provided
+  ## Add address information in new fields. Incomplete address
+  ## information is allowed (example: Port but not IP) as that information
+  ## might be already in the ENR or added later.
   if ip.isSome():
     let
       ipExt = ip.get()
@@ -164,6 +166,11 @@ proc addAddress(fields: var seq[FieldPair], ip: Option[ValidIpAddress],
       fields.add(((if isV6: "tcp6" else: "tcp"), tcpPort.get().uint16.toField))
     if udpPort.isSome():
       fields.add(((if isV6: "udp6" else: "udp"), udpPort.get().uint16.toField))
+  else:
+    if tcpPort.isSome():
+      fields.add(("tcp", tcpPort.get().uint16.toField))
+    if udpPort.isSome():
+      fields.add(("udp", udpPort.get().uint16.toField))
 
 proc init*(T: type Record, seqNum: uint64,
                            pk: PrivateKey,
@@ -177,6 +184,7 @@ proc init*(T: type Record, seqNum: uint64,
   ## Can fail in case the record exceeds the `maxEnrSize`.
   var fields = newSeq[FieldPair]()
 
+  # TODO: Allow for initializing ENR with both ip4 and ipv6 address.
   fields.addAddress(ip, tcpPort, udpPort)
   fields.add extraFields
   makeEnrAux(seqNum, pk, fields)
@@ -302,6 +310,7 @@ proc update*(r: var Record, pk: PrivateKey,
   ## will not be altered in these cases.
   var fields = newSeq[FieldPair]()
 
+  # TODO: Make updating of both ipv4 and ipv6 address in ENR more convenient.
   fields.addAddress(ip, tcpPort, udpPort)
   fields.add extraFields
   r.update(pk, fields)

tests/p2p/test_enr.nim

@@ -83,7 +83,7 @@ suite "ENR":
   test "ENR without address":
     let
       keypair = KeyPair.random(rng[])
-      port = some(Port(9000))
+      port = none(Port)
       enr = Record.init(
         100, keypair.seckey, none(ValidIpAddress), port, port)[]
       typedEnr = get enr.toTypedRecord()
@@ -192,8 +192,8 @@ suite "ENR":
       check updated.isOk()
       check:
         r.tryGet("ip", uint).isNone()
-        r.tryGet("tcp", uint).isNone()
+        r.tryGet("tcp", uint).isSome()
-        r.tryGet("udp", uint).isNone()
+        r.tryGet("udp", uint).isSome()
         r.seqNum == 1
 
     block:
@@ -202,9 +202,9 @@ suite "ENR":
       check updated.isOk()
       check:
         r.tryGet("ip", uint).isNone()
-        r.tryGet("tcp", uint).isNone()
+        r.tryGet("tcp", uint).isSome()
-        r.tryGet("udp", uint).isNone()
+        r.tryGet("udp", uint).isSome()
-        r.seqNum == 1
+        r.seqNum == 2
 
     block:
       let updated = r.update(pk, some(ValidIpAddress.init("10.20.30.40")),
@@ -223,7 +223,7 @@ suite "ENR":
         typedEnr.udp.isSome()
         typedEnr.udp.get() == 9000
 
-        r.seqNum == 2
+        r.seqNum == 3
 
     block:
       let updated = r.update(pk, some(ValidIpAddress.init("10.20.30.40")),
@@ -242,4 +242,4 @@ suite "ENR":
         typedEnr.udp.isSome()
         typedEnr.udp.get() == 9001
 
-        r.seqNum == 3
+        r.seqNum == 4