Skip to content

Commit

Permalink
fix: do not overwrite addresses on identify push when none are sent
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
achingbrain committed Nov 2, 2023
1 parent dd400cd commit 268c42b
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 1 deletion.
11 changes: 11 additions & 0 deletions packages/libp2p/src/identify/identify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
61 changes: 60 additions & 1 deletion packages/libp2p/test/identify/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -15,6 +15,7 @@ import drain from 'it-drain'
import * as lp from 'it-length-prefixed'
import { pipe } from 'it-pipe'
import { pbStream } from 'it-protobuf-stream'
import { pushable } from 'it-pushable'
import pDefer from 'p-defer'
import sinon from 'sinon'
import { stubInterface } from 'sinon-ts'
Expand All @@ -31,8 +32,10 @@ import { DefaultIdentifyService } from '../../src/identify/identify.js'
import { identifyService, type IdentifyServiceInit, Message } from '../../src/identify/index.js'
import { Identify } from '../../src/identify/pb/message.js'
import { DefaultTransportManager } from '../../src/transport-manager.js'
import type { Connection } from '@libp2p/interface/connection'
import type { IncomingStreamData } from '@libp2p/interface-internal/registrar'
import type { TransportManager } from '@libp2p/interface-internal/transport-manager'
import type { Duplex, Source } from 'it-stream-types'

const listenMaddrs = [multiaddr('/ip4/127.0.0.1/tcp/15002/ws')]

Expand Down Expand Up @@ -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)
}

Check warning on line 519 in packages/libp2p/test/identify/index.spec.ts

View check run for this annotation

Codecov / codecov/patch

packages/libp2p/test/identify/index.spec.ts#L519

Added line #L519 was not covered by tests
}

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)
}

Check warning on line 546 in packages/libp2p/test/identify/index.spec.ts

View check run for this annotation

Codecov / codecov/patch

packages/libp2p/test/identify/index.spec.ts#L546

Added line #L546 was not covered by tests
}

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'])
})
})

0 comments on commit 268c42b

Please sign in to comment.