From 6050cdef7e266bdfd061bc80fbdbadb14d1f0da3 Mon Sep 17 00:00:00 2001
From: diegomrsantos <diego@status.im>
Date: Thu, 25 May 2023 15:47:00 +0200
Subject: [PATCH] Refinement of Hole Punching Service (#892)

---
 .pinned                                       |   2 +-
 libp2p/multiaddress.nim                       |  14 +-
 .../protocols/connectivity/dcutr/client.nim   |   4 +-
 libp2p/protocols/connectivity/dcutr/core.nim  |   2 +-
 .../protocols/connectivity/dcutr/server.nim   |   2 +-
 libp2p/services/hpservice.nim                 |   8 +-
 libp2p/wire.nim                               |   6 +-
 tests/testdcutr.nim                           |  30 ++--
 tests/testhpservice.nim                       |  15 +-
 tests/testwire.nim                            | 151 ++++++++++++++++++
 10 files changed, 197 insertions(+), 37 deletions(-)
 create mode 100644 tests/testwire.nim

diff --git a/.pinned b/.pinned
index 465e3da70..e89bfaea4 100644
--- a/.pinned
+++ b/.pinned
@@ -9,7 +9,7 @@ metrics;https://github.com/status-im/nim-metrics@#abf3acc7f06cee9ee2c287d2f31413
 nimcrypto;https://github.com/cheatfate/nimcrypto@#4014ef939b51e02053c2e16dd3481d47bc9267dd
 secp256k1;https://github.com/status-im/nim-secp256k1@#fd173fdff863ce2e211cf64c9a03bc7539fe40b0
 serialization;https://github.com/status-im/nim-serialization@#5b7cea55efeb074daa8abd8146a03a34adb4521a
-stew;https://github.com/status-im/nim-stew@#8caa9771995b266e10b2e7c0de6cbfa698902e68
+stew;https://github.com/status-im/nim-stew@#003fe9f0c83c2b0b2ccbd37087e6d1ccd30a3234
 testutils;https://github.com/status-im/nim-testutils@#dfc4c1b39f9ded9baf6365014de2b4bfb4dafc34
 unittest2;https://github.com/status-im/nim-unittest2@#883c7a50ad3b82158e64d074c5578fe33ab3c452
 websock;https://github.com/status-im/nim-websock@#fea05cde8b123b38d1a0a8524b77efbc84daa848
diff --git a/libp2p/multiaddress.nim b/libp2p/multiaddress.nim
index 7ed76682d..2c5d09995 100644
--- a/libp2p/multiaddress.nim
+++ b/libp2p/multiaddress.nim
@@ -470,13 +470,23 @@ const
   DNS* = mapOr(DNSANY, DNS4, DNS6, DNSADDR)
   IP* = mapOr(IP4, IP6)
   DNS_OR_IP* = mapOr(DNS, IP)
-  TCP* = mapOr(mapAnd(DNS, mapEq("tcp")), mapAnd(IP, mapEq("tcp")))
-  UDP* = mapOr(mapAnd(DNS, mapEq("udp")), mapAnd(IP, mapEq("udp")))
+  TCP_DNS* = mapAnd(DNS, mapEq("tcp"))
+  TCP_IP* =mapAnd(IP, mapEq("tcp"))
+  TCP* = mapOr(TCP_DNS, TCP_IP)
+  UDP_DNS* = mapAnd(DNS, mapEq("udp"))
+  UDP_IP* = mapAnd(IP, mapEq("udp"))
+  UDP* = mapOr(UDP_DNS, UDP_IP)
   UTP* = mapAnd(UDP, mapEq("utp"))
   QUIC* = mapAnd(UDP, mapEq("quic"))
   UNIX* = mapEq("unix")
+  WS_DNS* = mapAnd(TCP_DNS, mapEq("ws"))
+  WS_IP* = mapAnd(TCP_IP, mapEq("ws"))
   WS* = mapAnd(TCP, mapEq("ws"))
+  WSS_DNS* = mapAnd(TCP_DNS, mapEq("wss"))
+  WSS_IP* = mapAnd(TCP_IP, mapEq("wss"))
   WSS* = mapAnd(TCP, mapEq("wss"))
+  WebSockets_DNS* = mapOr(WS_DNS, WSS_DNS)
+  WebSockets_IP* = mapOr(WS_IP, WSS_IP)
   WebSockets* = mapOr(WS, WSS)
   Onion3* = mapEq("onion3")
   TcpOnion3* = mapAnd(TCP, Onion3)
diff --git a/libp2p/protocols/connectivity/dcutr/client.nim b/libp2p/protocols/connectivity/dcutr/client.nim
index 220b6cdb5..1aafeb160 100644
--- a/libp2p/protocols/connectivity/dcutr/client.nim
+++ b/libp2p/protocols/connectivity/dcutr/client.nim
@@ -63,13 +63,13 @@ proc startSync*(self: DcutrClient, switch: Switch, remotePeerId: PeerId, addrs:
     let rttEnd = Moment.now()
     debug "Dcutr initiator has received a Connect message back.", connectAnswer
     let halfRtt = (rttEnd - rttStart) div 2'i64
-    await stream.send(MsgType.Sync, addrs)
+    await stream.send(MsgType.Sync, @[])
     debug "Dcutr initiator has sent a Sync message."
     await sleepAsync(halfRtt)
 
     if peerDialableAddrs.len > self.maxDialableAddrs:
         peerDialableAddrs = peerDialableAddrs[0..<self.maxDialableAddrs]
-    var futs = peerDialableAddrs.mapIt(switch.connect(stream.peerId, @[it], forceDial = true, reuseConnection = false, upgradeDir = Direction.In))
+    var futs = peerDialableAddrs.mapIt(switch.connect(stream.peerId, @[it], forceDial = true, reuseConnection = false))
     try:
       discard await anyCompleted(futs).wait(self.connectTimeout)
       debug "Dcutr initiator has directly connected to the remote peer."
diff --git a/libp2p/protocols/connectivity/dcutr/core.nim b/libp2p/protocols/connectivity/dcutr/core.nim
index 041829912..07585dc51 100644
--- a/libp2p/protocols/connectivity/dcutr/core.nim
+++ b/libp2p/protocols/connectivity/dcutr/core.nim
@@ -24,7 +24,7 @@ import ../../../multiaddress,
 export multiaddress
 
 const
-  DcutrCodec* = "/libp2p/dcutr/1.0.0"
+  DcutrCodec* = "/libp2p/dcutr"
 
 type
   MsgType* = enum
diff --git a/libp2p/protocols/connectivity/dcutr/server.nim b/libp2p/protocols/connectivity/dcutr/server.nim
index b0f14d3b0..c9f4fcb4b 100644
--- a/libp2p/protocols/connectivity/dcutr/server.nim
+++ b/libp2p/protocols/connectivity/dcutr/server.nim
@@ -60,7 +60,7 @@ proc new*(T: typedesc[Dcutr], switch: Switch, connectTimeout = 15.seconds, maxDi
 
       if peerDialableAddrs.len > maxDialableAddrs:
         peerDialableAddrs = peerDialableAddrs[0..<maxDialableAddrs]
-      var futs = peerDialableAddrs.mapIt(switch.connect(stream.peerId, @[it], forceDial = true, reuseConnection = false))
+      var futs = peerDialableAddrs.mapIt(switch.connect(stream.peerId, @[it], forceDial = true, reuseConnection = false, upgradeDir = Direction.In))
       try:
         discard await anyCompleted(futs).wait(connectTimeout)
         debug "Dcutr receiver has directly connected to the remote peer."
diff --git a/libp2p/services/hpservice.nim b/libp2p/services/hpservice.nim
index 9cf45e673..5c2b6fc81 100644
--- a/libp2p/services/hpservice.nim
+++ b/libp2p/services/hpservice.nim
@@ -12,18 +12,17 @@ when (NimMajor, NimMinor) < (1, 4):
 else:
   {.push raises: [].}
 
-import std/[tables, sequtils]
+import std/sequtils
 
 import chronos, chronicles
 
 import ../switch, ../wire
 import ../protocols/rendezvous
 import ../services/autorelayservice
-import ../discovery/[rendezvousinterface, discoverymngr]
 import ../protocols/connectivity/relay/relay
 import ../protocols/connectivity/autonat/service
 import ../protocols/connectivity/dcutr/[client, server]
-
+import ../multicodec
 
 logScope:
   topics = "libp2p hpservice"
@@ -52,6 +51,9 @@ proc tryStartingDirectConn(self: HPService, switch: Switch, peerId: PeerId): Fut
   await sleepAsync(500.milliseconds) # wait for AddressBook to be populated
   for address in switch.peerStore[AddressBook][peerId]:
     try:
+      let isRelayed = address.contains(multiCodec("p2p-circuit"))
+      if isRelayed.isErr() or isRelayed.get():
+        continue
       if DNS.matchPartial(address):
         return await tryConnect(address)
       else:
diff --git a/libp2p/wire.nim b/libp2p/wire.nim
index f375e7b77..533024755 100644
--- a/libp2p/wire.nim
+++ b/libp2p/wire.nim
@@ -23,14 +23,14 @@ else:
 
 const
   RTRANSPMA* = mapOr(
-    TCP,
-    WebSockets,
+    TCP_IP,
+    WebSockets_IP,
     UNIX
   )
 
   TRANSPMA* = mapOr(
     RTRANSPMA,
-    UDP
+    UDP_IP
   )
 
 
diff --git a/tests/testdcutr.nim b/tests/testdcutr.nim
index 5207a972a..8cb7e946c 100644
--- a/tests/testdcutr.nim
+++ b/tests/testdcutr.nim
@@ -57,15 +57,14 @@ suite "Dcutr":
     for t in behindNATSwitch.transports:
       t.networkReachability = NetworkReachability.NotReachable
 
-    expect CatchableError:
-      # we can't hole punch when both peers are in the same machine. This means that the simultaneous dialings will result
-      # in two connections attemps, instead of one. This dial is going to fail because the dcutr client is acting as the
-      # tcp simultaneous incoming upgrader in the dialer which works only in the simultaneous open case.
-      await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
-      .wait(300.millis)
+    await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
+    .wait(300.millis)
 
     checkExpiring:
-      # we still expect a new connection to be open by the receiver peer acting as the dcutr server
+      # we can't hole punch when both peers are in the same machine. This means that the simultaneous dialings will result
+      # in two connections attemps, instead of one. The server dial is going to fail because it is acting as the
+      # tcp simultaneous incoming upgrader in the dialer which works only in the simultaneous open case, but the client
+      # dial will succeed.
       behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 2
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
@@ -84,8 +83,8 @@ suite "Dcutr":
     body
 
     checkExpiring:
-      # we still expect a new connection to be open by the receiver peer acting as the dcutr server
-      behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 2
+      # no connection will be open by the receiver peer acting as the dcutr server
+      behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 1
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
 
@@ -133,16 +132,13 @@ suite "Dcutr":
     for t in behindNATSwitch.transports:
       t.networkReachability = NetworkReachability.NotReachable
 
-    expect CatchableError:
-      # we can't hole punch when both peers are in the same machine. This means that the simultaneous dialings will result
-      # in two connections attemps, instead of one. This dial is going to fail because the dcutr client is acting as the
-      # tcp simultaneous incoming upgrader in the dialer which works only in the simultaneous open case.
-      await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
-      .wait(300.millis)
+    await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
+    .wait(300.millis)
 
     checkExpiring:
-      # we still expect a new connection to be open by the receiver peer acting as the dcutr server
-      behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 1
+      # we can't hole punch when both peers are in the same machine. This means that the simultaneous dialings will result
+      # in two connections attemps, instead of one. The server dial is going to fail, but the client dial will succeed.
+      behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 2
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
 
diff --git a/tests/testhpservice.nim b/tests/testhpservice.nim
index 9dfce61db..8ff6efcf2 100644
--- a/tests/testhpservice.nim
+++ b/tests/testhpservice.nim
@@ -25,7 +25,6 @@ import ../libp2p/[builders,
                   services/autorelayservice]
 import ../libp2p/protocols/connectivity/relay/[relay, client]
 import ../libp2p/protocols/connectivity/autonat/[service]
-import ../libp2p/wire
 import ../libp2p/nameresolving/nameresolver
 import ../libp2p/nameresolving/mockresolver
 
@@ -81,11 +80,13 @@ suite "Hole Punching":
     let hpservice = HPService.new(autonatService, autoRelayService, isPublicAddrIPAddrMock)
 
     let privatePeerSwitch = createSwitch(relayClient, hpservice)
+    let peerSwitch = createSwitch()
     let switchRelay = createSwitch(Relay.new())
 
-    await allFutures(switchRelay.start(), privatePeerSwitch.start(), publicPeerSwitch.start())
+    await allFutures(switchRelay.start(), privatePeerSwitch.start(), publicPeerSwitch.start(), peerSwitch.start())
 
     await privatePeerSwitch.connect(switchRelay.peerInfo.peerId, switchRelay.peerInfo.addrs)
+    await privatePeerSwitch.connect(peerSwitch.peerInfo.peerId, peerSwitch.peerInfo.addrs) # for autonat
 
     await publicPeerSwitch.connect(privatePeerSwitch.peerInfo.peerId, (await privatePeerRelayAddr))
 
@@ -94,7 +95,7 @@ suite "Hole Punching":
       not isRelayed(privatePeerSwitch.connManager.selectMuxer(publicPeerSwitch.peerInfo.peerId).connection)
 
     await allFuturesThrowing(
-      privatePeerSwitch.stop(), publicPeerSwitch.stop(), switchRelay.stop())
+      privatePeerSwitch.stop(), publicPeerSwitch.stop(), switchRelay.stop(), peerSwitch.stop())
 
   asyncTest "Direct connection must work when peer address is public and dns is used":
 
@@ -105,7 +106,6 @@ suite "Hole Punching":
     let relayClient = RelayClient.new()
     let privatePeerRelayAddr = newFuture[seq[MultiAddress]]()
 
-
     let resolver = MockResolver.new()
     resolver.ipResponses[("localhost", false)] = @["127.0.0.1"]
     resolver.ipResponses[("localhost", true)] = @["::1"]
@@ -126,11 +126,13 @@ suite "Hole Punching":
     let hpservice = HPService.new(autonatService, autoRelayService, isPublicAddrIPAddrMock)
 
     let privatePeerSwitch = createSwitch(relayClient, hpservice, nameResolver = resolver)
+    let peerSwitch = createSwitch()
     let switchRelay = createSwitch(Relay.new())
 
-    await allFutures(switchRelay.start(), privatePeerSwitch.start(), publicPeerSwitch.start())
+    await allFutures(switchRelay.start(), privatePeerSwitch.start(), publicPeerSwitch.start(), peerSwitch.start())
 
     await privatePeerSwitch.connect(switchRelay.peerInfo.peerId, switchRelay.peerInfo.addrs)
+    await privatePeerSwitch.connect(peerSwitch.peerInfo.peerId, peerSwitch.peerInfo.addrs) # for autonat
 
     await publicPeerSwitch.connect(privatePeerSwitch.peerInfo.peerId, (await privatePeerRelayAddr))
 
@@ -139,7 +141,7 @@ suite "Hole Punching":
       not isRelayed(privatePeerSwitch.connManager.selectMuxer(publicPeerSwitch.peerInfo.peerId).connection)
 
     await allFuturesThrowing(
-      privatePeerSwitch.stop(), publicPeerSwitch.stop(), switchRelay.stop())
+      privatePeerSwitch.stop(), publicPeerSwitch.stop(), switchRelay.stop(), peerSwitch.stop())
 
   proc holePunchingTest(connectStub: proc (): Future[void] {.async.},
                         isPublicIPAddrProc: IsPublicIPAddrProc,
@@ -215,4 +217,3 @@ suite "Hole Punching":
       raise newException(CatchableError, "error")
 
     await holePunchingTest(connectStub, isPublicAddrIPAddrMock, Reachable)
-
diff --git a/tests/testwire.nim b/tests/testwire.nim
new file mode 100644
index 000000000..6c1f674b4
--- /dev/null
+++ b/tests/testwire.nim
@@ -0,0 +1,151 @@
+{.used.}
+
+# Nim-Libp2p
+# Copyright (c) 2023 Status Research & Development GmbH
+# Licensed under either of
+#  * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE))
+#  * MIT license ([LICENSE-MIT](LICENSE-MIT))
+# at your option.
+# This file may not be copied, modified, or distributed except according to
+# those terms.
+
+import ./helpers
+import ../libp2p/multiaddress
+import ../libp2p/wire
+
+suite "Wire":
+
+  test "initTAddress returns ok and correct result for a Unix domain address":
+    let ma = MultiAddress.init("/unix/tmp/socket").get()
+    let result = initTAddress(ma)
+    var address_un: array[108, uint8]
+    let unixPath = "/tmp/socket"
+    for i in 0..<len(unixPath):
+      address_un[i] = uint8(unixPath[i])
+    let expected = TransportAddress(
+      family: AddressFamily.Unix,
+      address_un: address_un,
+      port: Port(1)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv4/TCP address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/tcp/1234").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv4,
+      address_v4: [127'u8, 0, 0, 1],  # IPv4 address 127.0.0.1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv6/TCP address":
+    let ma = MultiAddress.init("/ip6/::1/tcp/1234").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv6,
+      address_v6: [0'u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],  # IPv6 address ::1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv4/UDP address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/udp/1234").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv4,
+      address_v4: [127'u8, 0, 0, 1],  # IPv4 address 127.0.0.1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv6/UDP address":
+    let ma = MultiAddress.init("/ip6/::1/udp/1234").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv6,
+      address_v6: [0'u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],  # IPv6 address ::1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv4/TCP/WS address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/tcp/1234/ws").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv4,
+      address_v4: [127'u8, 0, 0, 1],  # IPv4 address 127.0.0.1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv6/TCP/WS address":
+      let ma = MultiAddress.init("/ip6/::1/tcp/1234/ws").get()
+      let result = initTAddress(ma)
+      let expected = TransportAddress(
+        family: AddressFamily.IPv6,
+        address_v6: [0'u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],  # IPv6 address ::1
+        port: Port(1234)
+      )
+      check result.isOk
+      check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv4/TCP/WSS address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/tcp/1234/wss").get()
+    let result = initTAddress(ma)
+    let expected = TransportAddress(
+      family: AddressFamily.IPv4,
+      address_v4: [127'u8, 0, 0, 1],  # IPv4 address 127.0.0.1
+      port: Port(1234)
+    )
+    check result.isOk
+    check result.get() == expected
+
+  test "initTAddress returns ok and correct result for an IPv6/TCP/WSS address":
+      let ma = MultiAddress.init("/ip6/::1/tcp/1234/wss").get()
+      let result = initTAddress(ma)
+      let expected = TransportAddress(
+        family: AddressFamily.IPv6,
+        address_v6: [0'u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],  # IPv6 address ::1
+        port: Port(1234)
+      )
+      check result.isOk
+      check result.get() == expected
+
+  test "initTAddress returns error for a DNS/TCP/ws address":
+    let ma = MultiAddress.init("/dns4/localhost/tcp/1234/ws").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a DNS/TCP/wss address":
+    let ma = MultiAddress.init("/dns4/localhost/tcp/1234/wss").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a DNS/TCP address":
+    let ma = MultiAddress.init("/dns4/localhost/tcp/1234").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a DNS/UDP address":
+    let ma = MultiAddress.init("/dns4/localhost/udp/1234").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for an Onion3/TCP address":
+    let ma = MultiAddress.init("/onion3/vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd:1234").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a HTTP WebRTCDirect address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/http/p2p-webrtc-direct").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a HTTPS WebRTCDirect address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/https/p2p-webrtc-direct").get()
+    check initTAddress(ma).isErr
+
+  test "initTAddress returns error for a p2p-circuit address":
+    let ma = MultiAddress.init("/ip4/127.0.0.1/tcp/1234/p2p-circuit").get()
+    check initTAddress(ma).isErr