fix: encapsulate /p2p-circuit multiaddrs when dialing a known peerid (#…

…1680)

The logic for creating a dial target tries to ensure multiaddrs are encapsulated with `/p2p/${peerId}` IF the dial target's peer id is known. It uses `addr.getPeerId() != null` to check if this is already the case.

```
> multiaddr("/ip4/127.0.0.1/tcp/4001/p2p/12Dfoo...").getPeerId()
'12Dfoo...'
```

However, circuit relay addresses return the **relay server's** peer id if they don't end in `/p2p/${destinationPeerId}`.

```
> multiaddr("/ip4/127.0.0.1/tcp/4001/p2p/12D3bar.../p2p-circuit").getPeerId()
'12D3bar...'
```

Since the dialer then thinks the multiaddr already has a peer id, it doesn't encapsulate with `/p2p/${destinationPeerId}`, which ultimately results in failed dials to circuit relay destinations (`Circuit relay dial failed as addresses did not have peer id`). If I understand things right, the desired behavior is for circuit relay addresses to get encapsulated with the destination peer id.

---------

Co-authored-by: achingbrain <[email protected]>

src/connection-manager/dial-queue.ts

@@ -337,7 +337,7 @@ export class DialQueue {
       throw new CodeError('dial with more addresses than allowed', codes.ERR_TOO_MANY_ADDRESSES)
     }
 
-    // append peer id to multiaddrs if it is not already present
+    // ensure the peer id is appended to the multiaddr
     if (peerId != null) {
       const peerIdMultiaddr = `/p2p/${peerId.toString()}`
       dedupedMultiaddrs = dedupedMultiaddrs.map(addr => {
@@ -349,7 +349,8 @@ export class DialQueue {
           return addr
         }
 
-        if (addressPeerId == null) {
+        // append peer id to multiaddr if it is not already present
+        if (addressPeerId !== peerId.toString()) {
           return {
             multiaddr: addr.multiaddr.encapsulate(peerIdMultiaddr),
             isCertified: addr.isCertified

test/circuit-relay/relay.node.ts

@@ -17,6 +17,7 @@ import type { Libp2p } from '@libp2p/interface-libp2p'
 import { pbStream } from 'it-pb-stream'
 import { HopMessage, Status } from '../../src/circuit-relay/pb/index.js'
 import { Circuit } from '@multiformats/mafmt'
+import { multiaddr } from '@multiformats/multiaddr'
 
 describe('circuit-relay', () => {
   describe('flows with 1 listener', () => {
@@ -392,6 +393,25 @@ describe('circuit-relay', () => {
       await local.dial(ma)
     })
 
+    it('should be able to dial a peer from its relayed address without peer id', async () => {
+      // discover relay and make reservation
+      await remote.dial(relay1.getMultiaddrs()[0])
+      await usingAsRelay(remote, relay1)
+
+      // get the relayed multiaddr without the remote's peer id
+      const ma = getRelayAddress(remote)
+      const maWithoutPeerId = multiaddr(`${ma.toString().split('/p2p-circuit')[0]}/p2p-circuit`)
+      expect(maWithoutPeerId.getPeerId()).to.not.equal(remote.peerId.toString())
+
+      // ensure this is the only address we have for the peer
+      await local.peerStore.addressBook.set(remote.peerId, [
+        maWithoutPeerId
+      ])
+
+      // dial via peer id so we load the address from the address book
+      await local.dial(remote.peerId)
+    })
+
     it('should not stay connected to a relay when not already connected and HOP fails', async () => {
       // dial the remote through the relay
       const relayedMultiaddr = relay1.getMultiaddrs()[0].encapsulate(`/p2p-circuit/p2p/${remote.peerId.toString()}`)