fix: only check for non-wildcard ports

If we are passed a wildcard port, start a UDP mux listener on a
ephemeral port, otherwise check the existing listeners before trying
to start a new one.

To be reused an existing listener must be owned by the current peer
and be of the other IP family (e.g. you cannot listen on
`[ip4-host]:1234` twice but you can listen on `[ip4-host:1234]` and
`[ip6-host]:1234`.

Fixes #3049

Author: achingbrain

packages/transport-webrtc/src/private-to-public/listener.ts

@@ -1,5 +1,5 @@
-import { isIPv4, isIPv6 } from '@chainsafe/is-ip'
-import { InvalidPeerIdError, TypedEventEmitter } from '@libp2p/interface'
+import { isIPv4 } from '@chainsafe/is-ip'
+import { InvalidParametersError, InvalidPeerIdError, TypedEventEmitter } from '@libp2p/interface'
 import { getThinWaistAddresses } from '@libp2p/utils/get-thin-waist-addresses'
 import { multiaddr, fromStringTuples } from '@multiformats/multiaddr'
 import { WebRTCDirect } from '@multiformats/multiaddr-matcher'
@@ -83,53 +83,53 @@ export class WebRTCDirectListener extends TypedEventEmitter<ListenerEvents> impl
   }
 
   async listen (ma: Multiaddr): Promise<void> {
-    const { host, port } = ma.toOptions()
-
-    // have to do this before any async work happens so starting two listeners
-    // for the same port concurrently (e.g. ipv4/ipv6 both port 0) results in a
-    // single mux listener. This is necessary because libjuice binds to all
-    // interfaces for a given port so we we need to key on just the port number
-    // not the host + the port number
-    let existingServer = UDP_MUX_LISTENERS.find(s => s.port === port)
-
-    // if the server has not been started yet, or the port is a wildcard port
-    // and there is already a wildcard port for this address family, start a new
-    // UDP mux server
-    const wildcardPorts = port === 0 && existingServer?.port === 0
-    const sameAddressFamily = (existingServer?.isIPv4 === true && isIPv4(host)) || (existingServer?.isIPv6 === true && isIPv6(host))
-    let createdMuxServer = false
-
-    if (existingServer == null || (wildcardPorts && sameAddressFamily)) {
-      this.log('starting UDP mux server on %s:%p', host, port)
-      existingServer = this.startUDPMuxServer(host, port)
-      UDP_MUX_LISTENERS.push(existingServer)
-      createdMuxServer = true
-    }
+    const { host, port, family } = ma.toOptions()
+
+    let udpMuxServer: UDPMuxServer | undefined
+
+    if (port !== 0) {
+      // libjuice binds to all interfaces (IPv4/IPv6) for a given port so if we
+      // want to listen on a specific port, and there's already a mux listener
+      // for that port for the other family started by this node, we should
+      // reuse it
+      udpMuxServer = UDP_MUX_LISTENERS.find(s => s.port === port)
+
+      // make sure the port is free for the given family
+      if (udpMuxServer != null && ((udpMuxServer.isIPv4 && family === 4) || (udpMuxServer.isIPv6 && family === 6))) {
+        throw new InvalidParametersError(`There is already a listener for ${host}:${port}`)
+      }
 
-    if (!existingServer.peerId.equals(this.components.peerId)) {
-      // this would have to be another in-process peer so we are likely in a
-      // testing environment
-      throw new InvalidPeerIdError(`Another peer is already performing UDP mux on ${host}:${existingServer.port}`)
+      // check that we own the mux server
+      if (udpMuxServer != null && !udpMuxServer.peerId.equals(this.components.peerId)) {
+        throw new InvalidPeerIdError(`Another peer is already performing UDP mux on ${host}:${port}`)
+      }
     }
 
-    this.stunServer = await existingServer.server
-    const address = this.stunServer.address()
+    // start the mux server if we don't have one already
+    if (udpMuxServer == null) {
+      this.log('starting UDP mux server on %s:%p', host, port)
+      udpMuxServer = this.startUDPMuxServer(host, port, family)
+      UDP_MUX_LISTENERS.push(udpMuxServer)
+    }
 
-    if (!createdMuxServer) {
-      this.log('reused existing UDP mux server on %s:%p', host, address.port)
+    if (family === 4) {
+      udpMuxServer.isIPv4 = true
+    } else if (family === 6) {
+      udpMuxServer.isIPv6 = true
     }
 
+    this.stunServer = await udpMuxServer.server
     this.listeningMultiaddr = ma
     this.safeDispatchEvent('listening')
   }
 
-  private startUDPMuxServer (host: string, port: number): UDPMuxServer {
+  private startUDPMuxServer (host: string, port: number, family: 4 | 6): UDPMuxServer {
     return {
       peerId: this.components.peerId,
       owner: this,
       port,
-      isIPv4: isIPv4(host),
-      isIPv6: isIPv6(host),
+      isIPv4: family === 4,
+      isIPv6: family === 6,
       server: Promise.resolve()
         .then(async (): Promise<StunServer> => {
           // ensure we have a certificate

packages/transport-webrtc/test/transport.spec.ts

@@ -114,8 +114,8 @@ describe('WebRTCDirect Transport', () => {
       return this.skip()
     }
 
-    const ipv4 = multiaddr('/ip4/127.0.0.1/udp/0')
-    const ipv6 = multiaddr('/ip6/::1/udp/0')
+    const ipv4 = multiaddr('/ip4/127.0.0.1/udp/37287')
+    const ipv6 = multiaddr('/ip6/::1/udp/37287')
 
     await Promise.all([
       listener.listen(ipv4),
@@ -208,4 +208,70 @@ describe('WebRTCDirect Transport', () => {
       expect(ma.toString()).to.include('/udp/12346/webrtc-direct/certhash/u', 'did not add certhash to all WebRTC Direct addresses')
     }
   })
+
+  it('can start listeners for two nodes on wildcard socket addresses', async function () {
+    if ((!isNode && !isElectronMain) || !supportsIpV6()) {
+      return this.skip()
+    }
+
+    const otherTransport = new WebRTCDirectTransport({
+      ...components,
+      peerId: peerIdFromPrivateKey(await generateKeyPair('Ed25519'))
+    })
+    const otherTransportIp4Listener = otherTransport.createListener({
+      upgrader
+    })
+    const otherTransportIp6Listener = otherTransport.createListener({
+      upgrader
+    })
+
+    const ip6Listener = transport.createListener({
+      upgrader
+    })
+
+    const ipv4 = multiaddr('/ip4/0.0.0.0/udp/0')
+    const ipv6 = multiaddr('/ip6/::/udp/0')
+
+    await Promise.all([
+      listener.listen(ipv4),
+      otherTransportIp4Listener.listen(ipv4),
+      ip6Listener.listen(ipv6),
+      otherTransportIp6Listener.listen(ipv6)
+    ])
+
+    assertAllMultiaddrsHaveSamePort(listener.getAddrs())
+    assertAllMultiaddrsHaveSamePort(otherTransportIp4Listener.getAddrs())
+    assertAllMultiaddrsHaveSamePort(otherTransportIp6Listener.getAddrs())
+    assertAllMultiaddrsHaveSamePort(ip6Listener.getAddrs())
+
+    await listener.close()
+    await otherTransportIp4Listener.close()
+    await otherTransportIp6Listener.close()
+    await ip6Listener.close()
+  })
+
+  it('can start multiple wildcard listeners', async function () {
+    if ((!isNode && !isElectronMain) || !supportsIpV6()) {
+      return this.skip()
+    }
+
+    const otherListener = transport.createListener({
+      upgrader
+    })
+
+    const ipv4 = multiaddr('/ip4/0.0.0.0/udp/0')
+
+    await Promise.all([
+      listener.listen(ipv4),
+      otherListener.listen(ipv4)
+    ])
+
+    assertAllMultiaddrsHaveSamePort(listener.getAddrs())
+    assertAllMultiaddrsHaveSamePort(otherListener.getAddrs())
+
+    expect(listener.getAddrs()[0].toOptions().port).to.not.equal(otherListener.getAddrs()[0].toOptions().port, 'wildcard listeners did not listen on different ports')
+
+    await listener.close()
+    await otherListener.close()
+  })
 })