Provide different small fixes

library/storage_thread_requests/requests/node_debug_request.nim

@@ -9,7 +9,6 @@ import std/[options]
 import chronos
 import chronicles
 import codexdht/discv5/spr
-import pkg/libp2p/protocols/connectivity/autonat/service
 import ../../alloc
 import ../../../storage/conf
 import ../../../storage/rest/json
@@ -60,7 +59,13 @@ proc getDebug(
       if node.discovery.dhtRecord.isSome: node.discovery.dhtRecord.get.toURI else: "",
     "announceAddresses": node.discovery.announceAddrs,
     "table": table,
-    "nat": {"reachability": $storage[].autonatService.networkReachability},
+    "nat": {
+      "reachability":
+        if storage[].autonatService.isSome:
+          $storage[].autonatService.get.networkReachability
+        else:
+          "unknown"
+    },
   }
 
   return ok($json)

storage/discovery.nim

@@ -200,6 +200,8 @@ proc updateRecords*(
     d.protocol.updateRecord(d.dhtRecord).expect("Should update SPR")
 
 proc updateAnnounceRecord*(d: Discovery, addrs: openArray[MultiAddress]) =
+  # Updates announce addresses only, not the DHT routing record.
+  # Relay addresses should not pollute DHT routing.
   d.announceAddrs = @addrs
   info "Updating announce record", addrs = d.announceAddrs
   d.providerRecord = SignedPeerRecord
@@ -254,7 +256,8 @@ proc new*(
     key: PrivateKey,
     bindIp = IPv4_any(),
     bindPort = 0.Port,
-    announceAddrs: openArray[MultiAddress],
+    announceAddrs: openArray[MultiAddress] = [],
+    discoveryPort = 0.Port,
     bootstrapNodes: openArray[SignedPeerRecord] = [],
     store: Datastore = SQLiteDatastore.new(Memory).expect("Should not fail!"),
 ): Discovery =
@@ -265,8 +268,7 @@ proc new*(
     key: key, peerId: PeerId.init(key).expect("Should construct PeerId"), store: store
   )
 
-  # Update with empty values to get a valid SPR
-  self.updateRecords(@[], Port(0))
+  self.updateRecords(announceAddrs, discoveryPort)
 
   # --------------------------------------------------------------------------
   # FIXME disable IP limits temporarily so we can run our workshop. Re-enable

storage/storage.nim

@@ -85,6 +85,9 @@ proc start*(s: StorageServer) {.async.} =
     if s.config.nat.hasExtIp:
       # extip means that we assume the IP is reachable
       # So we just take the first peer addr and remap it with extip to keep the port only
+      if s.storageNode.switch.peerInfo.addrs.len == 0:
+        raise
+          newException(StorageError, "extip is set but switch has no listen addresses")
       @[
         s.storageNode.switch.peerInfo.addrs[0].remapAddr(
           ip = some(s.config.nat.extIp), port = none(Port)
@@ -94,7 +97,7 @@ proc start*(s: StorageServer) {.async.} =
       # If extip is not set, we have 2 choices:
       # 1- Announce the peer addrs contains detected addresses on the machine.
       # 2- Wait for AutoNat
-      # The probleme with 1 is that you will certainly announce private addresses
+      # The problem with 1 is that you will certainly announce private addresses
       # and if you advertise a CID, you will advertise these private addresses.
       # TODO: DHT client mode
       #s.storageNode.switch.peerInfo.addrs
@@ -124,7 +127,8 @@ proc stop*(s: StorageServer) {.async.} =
 
   notice "Stopping Storage node"
 
-  s.natMapper.close()
+  if s.natMapper != nil:
+    s.natMapper.close()
 
   var futures = @[
     s.storageNode.switch.stop(),
@@ -273,8 +277,9 @@ proc new*(
 
     discovery = Discovery.new(
       switch.peerInfo.privateKey,
-      announceAddrs = @[listenMultiAddr],
+      announceAddrs = @[],
       bindPort = config.discoveryPort,
+      discoveryPort = config.discoveryPort,
       bootstrapNodes = config.bootstrapNodes,
       store = discoveryStore,
     )

tests/storage/testnat.nim

@@ -1,9 +1,8 @@
-import std/[net, importutils]
+import std/[net, importutils, envvars]
 import pkg/chronos
 import ../../storage/utils/natutils
 import pkg/libp2p/[multiaddress, multihash, multicodec]
-import pkg/libp2p/protocols/connectivity/autonatv2/service except setup
-import pkg/libp2p/protocols/connectivity/autonatv2/types
+import pkg/libp2p/protocols/connectivity/autonat/types
 import pkg/libp2p/protocols/connectivity/relay/client as relayClientModule
 import pkg/libp2p/services/autorelayservice except setup
 
@@ -15,7 +14,6 @@ import ../../storage/nat
 import ../../storage/discovery
 import ../../storage/rng
 import ../../storage/utils
-import ../../storage/utils/addrutils
 
 privateAccess(NatMapper)
 
@@ -42,7 +40,7 @@ method mapNatPorts*(
 ): Future[Option[(Port, Port)]] {.async: (raises: [CancelledError]), gcsafe.} =
   m.mappedPorts
 
-suite "NatMapper.close":
+suite "NAT - NatMapper.close":
   test "does nothing when no upnp mapping":
     let mapper = MockNatMapper(
       natConfig: NatConfig(hasExtIp: false, nat: NatAuto),
@@ -65,30 +63,15 @@ suite "NatMapper.close":
     check device.deletedPorts ==
       @[(Port(8080), NatIpProtocol.Tcp), (Port(8090), NatIpProtocol.Udp)]
 
-suite "remapAddr":
-  test "replaces protocol tcp with udp":
-    let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid")
-    let remapped = ma.remapAddr(protocol = some("udp"), port = some(Port(9000)))
-    check remapped == MultiAddress.init("/ip4/1.2.3.4/udp/9000").expect("valid")
-
-  test "replaces only port, keeping protocol":
-    let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid")
-    let remapped = ma.remapAddr(port = some(Port(9000)))
-    check remapped == MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid")
-
-  test "replaces only ip, keeping protocol and port":
-    let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid")
-    let remapped = ma.remapAddr(ip = some(parseIpAddress("8.8.8.8")))
-    check remapped == MultiAddress.init("/ip4/8.8.8.8/tcp/5000").expect("valid")
-
-asyncchecksuite "handleNatStatus":
+asyncchecksuite "NAT - handleNatStatus":
   var sw: Switch
   var key: PrivateKey
   var disc: Discovery
-  let autoRelay =
-    AutoRelayService.new(1, relayClientModule.RelayClient.new(), nil, Rng.instance())
+  var autoRelay: AutoRelayService
 
   setup:
+    autoRelay =
+      AutoRelayService.new(1, relayClientModule.RelayClient.new(), nil, Rng.instance())
     key = PrivateKey.random(Rng.instance[]).get()
     disc = Discovery.new(key, announceAddrs = @[])
     sw = newStandardSwitch()
@@ -155,3 +138,22 @@ asyncchecksuite "handleNatStatus":
 
     check not autoRelay.isRunning
     check disc.announceAddrs == @[dialBack]
+
+suite "NAT - UPnP port mapping (requires NAT_TEST_UPNP=1)":
+  test "mapPorts and cleanup":
+    if getEnv("NAT_TEST_UPNP") != "1":
+      skip()
+
+    let res = UpnpDevice.init()
+    check res.isOk
+
+    let device = res.value
+    let ports = device.mapPorts(Port(8101), Port(8090))
+    check ports.isSome
+
+    let (tcp, udp) = ports.get()
+    check tcp == Port(8101)
+    check udp == Port(8090)
+
+    check device.deletePortMapping(Port(8101), NatIpProtocol.Tcp).isOk
+    check device.deletePortMapping(Port(8090), NatIpProtocol.Udp).isOk

tests/storage/testnatutils.nim

@@ -46,7 +46,7 @@ method addPortMapping*(
   else:
     err("mapping failed")
 
-suite "UpnpDevice.init":
+suite "NAT - UpnpDevice.init":
   test "returns err when discover fails":
     check MockUpnpDev(discoverOk: false).init().isErr
 
@@ -65,7 +65,7 @@ suite "UpnpDevice.init":
   test "returns ok when IP not routable":
     check MockUpnpDev(discoverOk: true, igdResult: IGDIpNotRoutable).init().isOk
 
-suite "UpnpDevice.mapPorts":
+suite "NAT - UpnpDevice.mapPorts":
   test "returns none when addPortMapping fails":
     check MockUpnpDev(addPortMappingOk: false).mapPorts(Port(8080), Port(8090)).isNone
 
@@ -82,7 +82,7 @@ suite "UpnpDevice.mapPorts":
     let d = MockUpnpDev(addPortMappingOk: true, failOnProto: some(NatIpProtocol.Udp))
     check d.mapPorts(Port(8080), Port(8090)).isNone
 
-suite "PmpDevice.mapPorts":
+suite "NAT - PmpDevice.mapPorts":
   test "returns none when mapping fails":
     check MockPmpDev(addPortMappingOk: false).mapPorts(Port(8080), Port(8090)).isNone