From 6189c2aaf5af11f8afd7d0cdac85de04c35dfec7 Mon Sep 17 00:00:00 2001
From: diegomrsantos <diego@status.im>
Date: Fri, 17 Nov 2023 10:46:35 +0100
Subject: [PATCH] fix(dcutr): make the dcutr client inbound and the server
 outbound (#983)

---
 .../protocols/connectivity/dcutr/client.nim   |  2 +-
 .../protocols/connectivity/dcutr/server.nim   |  2 +-
 tests/testdcutr.nim                           | 30 +++++++++++--------
 tests/testhpservice.nim                       | 22 +++-----------
 4 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/libp2p/protocols/connectivity/dcutr/client.nim b/libp2p/protocols/connectivity/dcutr/client.nim
index cda04c72a..19dda0cef 100644
--- a/libp2p/protocols/connectivity/dcutr/client.nim
+++ b/libp2p/protocols/connectivity/dcutr/client.nim
@@ -66,7 +66,7 @@ proc startSync*(self: DcutrClient, switch: Switch, remotePeerId: PeerId, addrs:
 
     if peerDialableAddrs.len > self.maxDialableAddrs:
         peerDialableAddrs = peerDialableAddrs[0..<self.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(self.connectTimeout)
       debug "Dcutr initiator has directly connected to the remote peer."
diff --git a/libp2p/protocols/connectivity/dcutr/server.nim b/libp2p/protocols/connectivity/dcutr/server.nim
index ecd4d15f0..179537af9 100644
--- a/libp2p/protocols/connectivity/dcutr/server.nim
+++ b/libp2p/protocols/connectivity/dcutr/server.nim
@@ -56,7 +56,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, upgradeDir = Direction.In))
+      var futs = peerDialableAddrs.mapIt(switch.connect(stream.peerId, @[it], forceDial = true, reuseConnection = false, upgradeDir = Direction.Out))
       try:
         discard await anyCompleted(futs).wait(connectTimeout)
         debug "Dcutr receiver has directly connected to the remote peer."
diff --git a/tests/testdcutr.nim b/tests/testdcutr.nim
index 83822e872..a61237af1 100644
--- a/tests/testdcutr.nim
+++ b/tests/testdcutr.nim
@@ -57,14 +57,15 @@ suite "Dcutr":
     for t in behindNATSwitch.transports:
       t.networkReachability = NetworkReachability.NotReachable
 
-    await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
-    .wait(300.millis)
+    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)
 
     checkExpiring:
-      # 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.
+      # 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
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
@@ -83,8 +84,8 @@ suite "Dcutr":
     body
 
     checkExpiring:
-      # no connection will be open by the receiver peer acting as the dcutr server
-      behindNATSwitch.connManager.connCount(publicSwitch.peerInfo.peerId) == 1
+      # 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
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
 
@@ -142,13 +143,16 @@ suite "Dcutr":
     for t in behindNATSwitch.transports:
       t.networkReachability = NetworkReachability.NotReachable
 
-    await DcutrClient.new().startSync(behindNATSwitch, publicSwitch.peerInfo.peerId, behindNATSwitch.peerInfo.addrs)
-    .wait(300.millis)
+    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)
 
     checkExpiring:
-      # 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
+      # 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
 
     await allFutures(behindNATSwitch.stop(), publicSwitch.stop())
 
diff --git a/tests/testhpservice.nim b/tests/testhpservice.nim
index 4f6c7fb5c..48f2bbb83 100644
--- a/tests/testhpservice.nim
+++ b/tests/testhpservice.nim
@@ -193,31 +193,17 @@ suite "Hole Punching":
     await privatePeerSwitch2.connect(privatePeerSwitch1.peerInfo.peerId, (await privatePeerRelayAddr1))
     privatePeerSwitch2.connectStub = rcvConnectStub
 
-    checkExpiring:
-      # 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.
-      privatePeerSwitch1.connManager.connCount(privatePeerSwitch2.peerInfo.peerId) == 1 and
-        not isRelayed(privatePeerSwitch1.connManager.selectMuxer(privatePeerSwitch2.peerInfo.peerId).connection)
+    # wait for hole punching to finish in the background
+    await sleepAsync(600.millis)
 
     await allFuturesThrowing(
       privatePeerSwitch1.stop(), privatePeerSwitch2.stop(), switchRelay.stop(),
       switchAux.stop(), switchAux2.stop(), switchAux3.stop(), switchAux4.stop())
 
   asyncTest "Hole punching when peers addresses are private":
-    proc connectStub(self: SwitchStub,
-                    peerId: PeerId,
-                    addrs: seq[MultiAddress],
-                    forceDial = false,
-                    reuseConnection = true,
-                    upgradeDir = Direction.Out): Future[void] {.async.} =
-      self.connectStub = nil # this stub should be called only once
-      await sleepAsync(100.millis) # avoid simultaneous dialing that causes address in use error
-      await self.switch.connect(peerId, addrs, forceDial, reuseConnection, upgradeDir)
-    await holePunchingTest(nil, connectStub, NotReachable)
+    await holePunchingTest(nil, nil, NotReachable)
 
-  asyncTest "Hole punching when there is an error during unilateral direct connection":
+  asyncTest "Hole punching when peers addresses are private and there is an error in the initiator side":
 
     proc connectStub(self: SwitchStub,
                     peerId: PeerId,