More flexible mixing of clearnet addresses and tor proxy (#3054)

We improve the connection logic when a proxy is configured, but Tor shouldn't
be used for IPv4 or IPv6 remote addresses.

Author: rorp

eclair-core/src/main/scala/fr/acinq/eclair/io/ReconnectionTask.scala

@@ -187,27 +187,32 @@ object ReconnectionTask {
   // @formatter:on
 
   def selectNodeAddress(nodeParams: NodeParams, nodeAddresses: Seq[NodeAddress]): Option[NodeAddress] = {
-    // it doesn't make sense to mix tor and clearnet addresses, so we separate them and decide whether we use one or the other
+    val selectedAddresses = selectNodeAddresses(nodeParams, nodeAddresses)
+    // we pick an address at random
+    if (selectedAddresses.nonEmpty) {
+      Some(selectedAddresses(Random.nextInt(selectedAddresses.size)))
+    } else {
+      None
+    }
+  }
+
+  private[io] def selectNodeAddresses(nodeParams: NodeParams, nodeAddresses: Seq[NodeAddress]): Seq[NodeAddress] = {
     val torAddresses = nodeAddresses.collect { case o: OnionAddress => o }
     val clearnetAddresses = nodeAddresses diff torAddresses
     val selectedAddresses = nodeParams.socksProxy_opt match {
-      case Some(params) if clearnetAddresses.nonEmpty && params.useForTor && (!params.useForIPv4 || !params.useForIPv6) =>
-        // Remote has clearnet (and possibly tor addresses), and we support tor, but we have configured it to only use
-        // tor when strictly necessary. In this case we will only connect over clearnet.
+      case Some(params) if clearnetAddresses.nonEmpty && (!params.useForIPv4 || !params.useForIPv6) =>
+        // Remote has clearnet and possibly tor addresses, but we have configured our proxy to only be used
+        // for tor. In this case we will only connect over clearnet.
         clearnetAddresses
-      case Some(params) if torAddresses.nonEmpty && params.useForTor =>
-        // In all other cases, if they have a tor address and we support tor, we use tor.
-        torAddresses
+      case Some(params) if params.useForTor =>
+        // The SOCKS5 proxy is enabled, and specifically configured to handle tor addresses.
+        // This is the only case when we can connect to both tor and clearnet.
+        nodeAddresses
       case _ =>
-        // Otherwise, if we don't support tor or they don't have a tor address, we use clearnet.
+        // Otherwise, if we don't support tor or remote doesn't have a tor address, we use clearnet.
         clearnetAddresses
     }
-    // finally, we pick an address at random
-    if (selectedAddresses.nonEmpty) {
-      Some(selectedAddresses(Random.nextInt(selectedAddresses.size)))
-    } else {
-      None
-    }
+    selectedAddresses
   }
 
   def getPeerAddressFromDb(nodeParams: NodeParams, remoteNodeId: PublicKey): Option[NodeAddress] = {

eclair-core/src/test/scala/fr/acinq/eclair/io/ReconnectionTaskSpec.scala

@@ -250,15 +250,117 @@ class ReconnectionTaskSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
       assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet, tor)).contains(clearnet))
     }
     {
-      // tor supported and enabled for clearnet addresses: return tor addresses when available
+      // tor supported and enabled for clearnet addresses: return both tor and clearnet addresses when available
       val socksParams = mock[Socks5ProxyParams]
       socksParams.useForTor returns true
       socksParams.useForIPv4 returns true
       socksParams.useForIPv6 returns true
       nodeParams.socksProxy_opt returns Some(socksParams)
       assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet)).contains(clearnet))
       assert(ReconnectionTask.selectNodeAddress(nodeParams, List(tor)).contains(tor))
-      assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet, tor)).contains(tor))
+      assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet, tor)).exists(Set(clearnet, tor)(_)))
+    }
+    {
+      // tor supported and enabled for clearnet addresses, but disabled for tor: return clearnet addresses when available
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns false
+      socksParams.useForIPv4 returns true
+      socksParams.useForIPv6 returns true
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet)).contains(clearnet))
+      assert(ReconnectionTask.selectNodeAddress(nodeParams, List(tor)).isEmpty)
+      assert(ReconnectionTask.selectNodeAddress(nodeParams, List(clearnet, tor)).contains(clearnet))
+    }
+  }
+
+  test("select node addresses for reconnection") { () =>
+    val nodeParams = mock[NodeParams]
+    val clearnetIPv4 = NodeAddress.fromParts("1.2.3.4", 9735).get
+    val clearnetIPv6 = NodeAddress.fromParts("2001:db8::1", 9735).get
+    val tor = NodeAddress.fromParts("iq7zhmhck54vcax2vlrdcavq2m32wao7ekh6jyeglmnuuvv3js57r4id.onion", 9735).get
+    val dnsHostname = NodeAddress.fromParts("example.com", 9735).get
+
+    {
+      // no proxy configured: only return clearnet addresses
+      nodeParams.socksProxy_opt returns None
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List.empty)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname))
+    }
+    {
+      // proxy configured but not for tor: only return clearnet addresses
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns false
+      socksParams.useForIPv4 returns true
+      socksParams.useForIPv6 returns true
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List.empty)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname))
+    }
+    {
+      // proxy configured for tor but not for IPv4: return tor addresses only if there ar no clearnet addresses, otherwise return clearnet addresses
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns true
+      socksParams.useForIPv4 returns false
+      socksParams.useForIPv6 returns true
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List(tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6, tor)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname))
+    }
+    {
+      // proxy configured for tor but not for IPv6: return tor addresses only if there ar no clearnet addresses, otherwise return clearnet addresses
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns true
+      socksParams.useForIPv4 returns true
+      socksParams.useForIPv6 returns false
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List(tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6, tor)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname))
+    }
+    {
+      // proxy configured for tor but not for IPv4 and IPv6: return tor addresses only if there ar no clearnet addresses, otherwise return clearnet addresses
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns true
+      socksParams.useForIPv4 returns false
+      socksParams.useForIPv6 returns false
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List(tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6, tor)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname))
+    }
+    {
+      // proxy configured for tor and both IPv4/IPv6: return all addresses
+      val socksParams = mock[Socks5ProxyParams]
+      socksParams.useForTor returns true
+      socksParams.useForIPv4 returns true
+      socksParams.useForIPv6 returns true
+      nodeParams.socksProxy_opt returns Some(socksParams)
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4)) == List(clearnetIPv4))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6)) == List(clearnetIPv6))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(tor)) == List(tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, tor)) == List(clearnetIPv4, tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv6, tor)) == List(clearnetIPv6, tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(clearnetIPv4, clearnetIPv6, tor)) == List(clearnetIPv4, clearnetIPv6, tor))
+      assert(ReconnectionTask.selectNodeAddresses(nodeParams, List(dnsHostname, tor)) == List(dnsHostname, tor))
     }
   }