Cleanup

build.nims

@@ -78,20 +78,6 @@ task testIntegration, "Run integration tests":
   # test "testIntegration", params = "-d:chronicles_sinks=textlines[notimestamps,stdout],textlines[dynamic] " &
   #   "-d:chronicles_enabled_topics:integration:TRACE"
 
-task testNatPortMapping, "Run UPnP NAT integration test (requires miniupnpd container)":
-  buildBinary "storage",
-    outName = "storage",
-    params = "-d:chronicles_runtime_filtering -d:chronicles_log_level=TRACE"
-  putEnv("STORAGE_INTEGRATION_TEST_INCLUDES", "nat/testnatupnp.nim")
-  test "testIntegration", outName = "testIntegrationNat"
-
-task testNatPcpMapping, "Run PCP NAT integration test (requires miniupnpd container)":
-  buildBinary "storage",
-    outName = "storage",
-    params = "-d:chronicles_runtime_filtering -d:chronicles_log_level=TRACE"
-  putEnv("STORAGE_INTEGRATION_TEST_INCLUDES", "nat/testnatpcp.nim")
-  test "testIntegration", outName = "testIntegrationNatPcp"
-
 task testNatNotReachable,
   "Run NAT not-reachable scenario (needs the image + podman-compose)":
   test "integration/nat/not-reachable/testnotreachable", outName = "testNatNotReachable"

storage/nat.nim

@@ -128,7 +128,9 @@ proc stop*(m: NatPortMapper) =
 proc isPortMapped*(m: NatPortMapper, port: Port): bool =
   m.activeTcpPort.isSome and m.activeTcpPort.get == port
 
-method hasActiveMapping*(m: NatPortMapper): bool {.base, gcsafe.} =
+method hasMappingIds*(m: NatPortMapper): bool {.base, gcsafe.} =
+  # Only checks that mappings were created, not that they are still live
+  # (use hasMapping() for liveness check).
   m.tcpMappingId.isSome and m.udpMappingId.isSome
 
 proc announcePeerInfoAddrs*(discovery: Discovery, peerInfo: PeerInfo, udpPort: Port) =
@@ -195,11 +197,11 @@ method handleNatStatus*(
     if dialBackAddr.isNone:
       warn "Got empty dialback address in AutoNat when node is NotReachable"
 
-      if m.hasActiveMapping():
+      if m.hasMappingIds():
         m.close()
 
       discovery.announceDirectAddrs(@[], udpPort = discoveryPort)
-    elif m.hasActiveMapping():
+    elif m.hasMappingIds():
       warn "Not Reachable with active port mapping. The port mapping will be deleted and relay will start."
 
       # The mapping was created the the node is still not reachable.

storage/storage.nim

@@ -462,6 +462,10 @@ proc new*(
         # response can also carry loopback/private addresses:
         # they are never dialable by a remote peer, so drop them.
         let publicAddrs = addresses.filterIt(it.hasPublicRelayTransport())
+        if publicAddrs.len == 0:
+          warn "Relay reservation has no publicly dialable address, keeping previous announce",
+            addresses
+          return
         info "Relay reservation updated", addresses = publicAddrs
         # relay addresses are for download traffic only, not DHT routing
         discovery.announceRelayAddrs(publicAddrs),

tests/integration/nathelper.nim

@@ -1,65 +0,0 @@
-import std/algorithm
-import std/json
-import std/sequtils
-import pkg/chronos
-import pkg/questionable/results
-
-import ./multinodes
-import ./storageclient
-import ./storageconfig
-
-const
-  RelayTimeout* = 30_000
-  PollInterval* = 1_000
-  NatScheduleInterval* = 5.seconds
-
-proc checkNatStatus*(
-    client: StorageClient, reachability: string, relayRunning: bool, clientMode: bool
-) {.async.} =
-  check eventuallySafe(
-    block:
-      let info = (await client.info()).get
-      let nat = info["nat"]
-      let addrs = info["addrs"].getElems.mapIt(it.getStr)
-      let r = nat["reachability"].getStr()
-      let cm = nat["clientMode"].getBool()
-      let rr = nat["relayRunning"].getBool()
-      let ha = addrs.anyIt("p2p-circuit" in it)
-      let pm = nat["portMapping"].getStr()
-      let aa = info["announceAddresses"].getElems.mapIt(it.getStr)
-
-      # Reachable nodes must announce their dialable (non-circuit) addrs to
-      # the DHT (peerInfo observer); relayed nodes must announce their
-      # circuit addrs (onReservation).
-      let announceOk =
-        if reachability == "Reachable":
-          aa.len > 0 and aa.sorted == addrs.filterIt("p2p-circuit" notin it).sorted
-        elif relayRunning:
-          aa.len > 0 and aa.allIt("p2p-circuit" in it)
-        else:
-          true
-
-      # It is important to check all the conditions together to avoid race
-      # (new autonat iteration)
-      # So we add a checkoint for better debug in case of failures
-      checkpoint(
-        "reachability=" & r & " (want " & reachability & ")" & " clientMode=" & $cm &
-          " (want " & $clientMode & ")" & " relayRunning=" & $rr & " (want " &
-          $relayRunning & ")" & " p2p-circuit=" & $ha & " (want " & $relayRunning & ")" &
-          " portMapping=" & pm & " announceAddresses=" & $aa
-      )
-      r == reachability and cm == clientMode and rr == relayRunning and
-        ha == relayRunning and announceOk,
-    timeout = RelayTimeout,
-    pollInterval = PollInterval,
-  )
-
-proc checkReachable*(client: StorageClient) {.async.} =
-  await client.checkNatStatus("Reachable", relayRunning = false, clientMode = false)
-
-# Relay might be false when the mapping has been created for UPnP / TCP but
-# Autonat didn't detect yet Reachable
-proc checkNotReachable*(client: StorageClient, relayRunning = true) {.async.} =
-  await client.checkNatStatus(
-    "NotReachable", relayRunning = relayRunning, clientMode = true
-  )

tests/storage/testaddrutils.nim

@@ -3,6 +3,13 @@ import pkg/libp2p/multiaddress
 import ../asynctest
 import ../../storage/utils/addrutils
 
+const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD"
+
+proc circuitAddr(relayIp: string): MultiAddress =
+  MultiAddress
+    .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit")
+    .expect("valid")
+
 suite "addrutils - getTcpPort":
   test "extracts port from ipv4 tcp address":
     let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid")
@@ -37,13 +44,6 @@ suite "addrutils - remapAddr":
     check remapped == MultiAddress.init("/ip4/8.8.8.8/tcp/5000").expect("valid")
 
 suite "addrutils - hasPublicRelayTransport":
-  const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD"
-
-  proc circuitAddr(relayIp: string): MultiAddress =
-    MultiAddress
-      .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit")
-      .expect("valid")
-
   test "true when the relay has a public ip":
     check circuitAddr("204.168.234.45").hasPublicRelayTransport()
 
@@ -54,13 +54,6 @@ suite "addrutils - hasPublicRelayTransport":
     check not circuitAddr("172.17.0.1").hasPublicRelayTransport()
 
 suite "addrutils - dialableAddressPolicy":
-  const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD"
-
-  proc circuitAddr(relayIp: string): MultiAddress =
-    MultiAddress
-      .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit")
-      .expect("valid")
-
   test "keeps a public direct address":
     check MultiAddress
       .init("/ip4/204.168.234.45/tcp/8070")

tests/storage/testnatreaction.nim

@@ -26,7 +26,7 @@ method mapNatPorts*(
 .} =
   m.mappedPorts
 
-method hasActiveMapping*(m: MockNatPortMapper): bool =
+method hasMappingIds*(m: MockNatPortMapper): bool =
   m.activeMapping
 
 asyncchecksuite "NAT reaction - port mapping":