From 81753badee6320a2846b65ab73d2a58429742f6a Mon Sep 17 00:00:00 2001
From: achingbrain <alex@achingbrain.net>
Date: Thu, 2 Nov 2023 14:45:16 +0000
Subject: [PATCH] fix: do not overwrite addresses on identify push when none
 are sent

During identify-push in response to a protocol update go-libp2p does not send a signed peer record or the current list of listen addresses.

Protobuf does not let us differentiate between an empty list and no list items at all because the field is just repeated - an absence of repeated fields doesn't mean the list was omitted.

When the listen address list is empty, preserve any existing addresses.
---
 packages/libp2p/src/identify/identify.ts    | 11 ++++
 packages/libp2p/test/identify/index.spec.ts | 61 ++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/packages/libp2p/src/identify/identify.ts b/packages/libp2p/src/identify/identify.ts
index 906e5fddb8..3c53398a57 100644
--- a/packages/libp2p/src/identify/identify.ts
+++ b/packages/libp2p/src/identify/identify.ts
@@ -490,6 +490,17 @@ export class DefaultIdentifyService implements Startable, IdentifyService {
       peer.metadata.set('ProtocolVersion', uint8ArrayFromString(message.protocolVersion))
     }
 
+    // if the remote has not sent addresses, preserve the existing ones.
+    // this can happen during an identify-push triggered by a supported protocol
+    // change but the protobuf format makes it impossible to know if the remote
+    // was telling us they have no addresses (unlikely) or if they meant to send
+    // a partial update and omit the list entirely
+    if (peer.addresses.length === 0) {
+      // @ts-expect-error addresses is not optional
+      delete peer.addresses
+    }
+
+    log('patching %p with', peer)
     await this.peerStore.patch(connection.remotePeer, peer)
 
     const result: IdentifyResult = {
diff --git a/packages/libp2p/test/identify/index.spec.ts b/packages/libp2p/test/identify/index.spec.ts
index 63134a7d0c..1b244eb353 100644
--- a/packages/libp2p/test/identify/index.spec.ts
+++ b/packages/libp2p/test/identify/index.spec.ts
@@ -3,7 +3,7 @@
 
 import { TypedEventEmitter } from '@libp2p/interface/events'
 import { start, stop } from '@libp2p/interface/startable'
-import { mockConnectionGater, mockRegistrar, mockUpgrader, connectionPair } from '@libp2p/interface-compliance-tests/mocks'
+import { mockConnectionGater, mockRegistrar, mockUpgrader, connectionPair, mockStream } from '@libp2p/interface-compliance-tests/mocks'
 import { createEd25519PeerId } from '@libp2p/peer-id-factory'
 import { PeerRecord, RecordEnvelope } from '@libp2p/peer-record'
 import { PersistentPeerStore } from '@libp2p/peer-store'
@@ -33,6 +33,9 @@ import { Identify } from '../../src/identify/pb/message.js'
 import { DefaultTransportManager } from '../../src/transport-manager.js'
 import type { IncomingStreamData } from '@libp2p/interface-internal/registrar'
 import type { TransportManager } from '@libp2p/interface-internal/transport-manager'
+import { pushable } from 'it-pushable'
+import type { Duplex, Source } from 'it-stream-types'
+import type { Connection } from '@libp2p/interface/src/connection/index.js'
 
 const listenMaddrs = [multiaddr('/ip4/127.0.0.1/tcp/15002/ws')]
 
@@ -504,4 +507,60 @@ describe('identify', () => {
     }])
     expect(peer.id.publicKey).to.equalBytes(remoteComponents.peerId.publicKey)
   })
+
+  it('should not overwrite addresses when none are sent', async () => {
+    const localIdentify = new DefaultIdentifyService(localComponents, defaultInit)
+    await start(localIdentify)
+
+    const duplex: Duplex<any, Source<any>, any> = {
+      source: pushable(),
+      sink: async (source) => {
+        await drain(source)
+      }
+    }
+
+    duplex.source.push(lp.encode.single(Identify.encode({
+      listenAddrs: [
+        multiaddr('/ip4/127.0.0.1/tcp/4001').bytes
+      ],
+      protocols: [
+        '/proto/1'
+      ]
+    })))
+
+    await localIdentify._handlePush({
+      stream: mockStream(duplex),
+      connection: stubInterface<Connection>({
+        remotePeer: remoteComponents.peerId
+      })
+    })
+
+    const peerData = await localComponents.peerStore.get(remoteComponents.peerId)
+    expect(peerData.addresses[0].multiaddr.toString()).to.equal('/ip4/127.0.0.1/tcp/4001')
+    expect(peerData.protocols).to.deep.equal(['/proto/1'])
+
+    const updateDuplex: Duplex<any, Source<any>, any> = {
+      source: pushable(),
+      sink: async (source) => {
+        await drain(source)
+      }
+    }
+
+    updateDuplex.source.push(lp.encode.single(Identify.encode({
+      protocols: [
+        '/proto/2'
+      ]
+    })))
+
+    await localIdentify._handlePush({
+      stream: mockStream(updateDuplex),
+      connection: stubInterface<Connection>({
+        remotePeer: remoteComponents.peerId
+      })
+    })
+
+    const updatedPeerData = await localComponents.peerStore.get(remoteComponents.peerId)
+    expect(updatedPeerData.addresses[0].multiaddr.toString()).to.equal('/ip4/127.0.0.1/tcp/4001')
+    expect(updatedPeerData.protocols).to.deep.equal(['/proto/2'])
+  })
 })