From 4078082dac944c6feb64e9e45e5275d7f0bf6ae4 Mon Sep 17 00:00:00 2001
From: Joel Gustafson <joelg@mit.edu>
Date: Fri, 14 Apr 2023 03:07:25 -0400
Subject: [PATCH] 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 <alex@achingbrain.net>
---
 src/connection-manager/dial-queue.ts |  5 +++--
 test/circuit-relay/relay.node.ts     | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/connection-manager/dial-queue.ts b/src/connection-manager/dial-queue.ts
index bf5a637a5f..237688cce6 100644
--- a/src/connection-manager/dial-queue.ts
+++ b/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
diff --git a/test/circuit-relay/relay.node.ts b/test/circuit-relay/relay.node.ts
index 857f4e51e5..d58031b7b9 100644
--- a/test/circuit-relay/relay.node.ts
+++ b/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()}`)