Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(libp2p): filter out dnsaddrs for different peers #1954

Merged
merged 2 commits into from
Aug 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions packages/libp2p/src/connection-manager/dial-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,22 @@ export class DialQueue {
))
.flat()

// filter out any multiaddrs that we do not have transports for
const filteredAddrs = resolvedAddresses.filter(addr => Boolean(this.transportManager.transportForMultiaddr(addr.multiaddr)))
const filteredAddrs = resolvedAddresses.filter(addr => {
// filter out any multiaddrs that we do not have transports for
if (this.transportManager.transportForMultiaddr(addr.multiaddr) == null) {
return false
}

// if the resolved multiaddr has a PeerID but it's the wrong one, ignore it
// - this can happen with addresses like bootstrap.libp2p.io that resolve
// to multiple different peers
const addrPeerId = addr.multiaddr.getPeerId()
if (peerId != null && addrPeerId != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its p2p-circuit addrPeerId will likely be null always

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related: #1961

return peerId.equals(addrPeerId)
}
Comment on lines +321 to +324

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of the null checks look redundant. Also can the equality check be the other way? just for the sake of convention

Suggested change
const addrPeerId = addr.multiaddr.getPeerId()
if (peerId != null && addrPeerId != null) {
return peerId.equals(addrPeerId)
}
const addrPeerId = addr.multiaddr.getPeerId()
if (addrPeerId != null) {
return addrPeerId.equals(peerId)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to null guard on peerId again, if that's what you mean, because the type is PeerId | undefined - execution of the code takes place in a callback so TSC can't guarantee it's not been set back to undefined before the callback is invoked.


return true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the default be true? Can this be restructured such that it's only true when all conditions are met and defaults to false otherwise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to fallback to true and filter out the false on the way

})

// deduplicate addresses
const dedupedAddrs = new Map<string, Address>()
Expand Down
50 changes: 49 additions & 1 deletion packages/libp2p/test/connection-manager/dial-queue.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { mockConnection, mockDuplex, mockMultiaddrConnection } from '@libp2p/interface-compliance-tests/mocks'
import { createEd25519PeerId } from '@libp2p/peer-id-factory'
import { multiaddr } from '@multiformats/multiaddr'
import { multiaddr, resolvers } from '@multiformats/multiaddr'
import { expect } from 'aegir/chai'
import delay from 'delay'
import pDefer from 'p-defer'
Expand Down Expand Up @@ -280,4 +280,52 @@ describe('dial queue', () => {
// Dial attempt finished without connection
expect(signals['/ip4/127.0.0.1/tcp/1233']).to.have.property('aborted', true)
})

it('should ignore DNS addresses for other peers', async () => {
const remotePeer = await createEd25519PeerId()
const otherRemotePeer = await createEd25519PeerId()
const ma = multiaddr(`/dnsaddr/example.com/p2p/${remotePeer}`)
const maStr = `/ip4/123.123.123.123/tcp/2348/p2p/${remotePeer}`
const resolvedAddresses = [
`/ip4/234.234.234.234/tcp/4213/p2p/${otherRemotePeer}`,
maStr
]

let resolvedDNSAddrs = false
let dialedBadAddress = false

// simulate a DNSAddr that resolves to multiple different peers like
// bootstrap.libp2p.io
resolvers.set('dnsaddr', async (addr) => {
if (addr.equals(ma)) {
resolvedDNSAddrs = true
return resolvedAddresses
}

return []
})

dialer = new DialQueue(components, {
maxParallelDials: 50
})
components.transportManager.transportForMultiaddr.returns(stubInterface<Transport>())

const connection = mockConnection(mockMultiaddrConnection(mockDuplex(), remotePeer))

components.transportManager.dial.callsFake(async (ma, opts = {}) => {
if (ma.toString() === maStr) {
await delay(100)
return connection
}

dialedBadAddress = true
throw new Error('Could not dial address')
})

await expect(dialer.dial(ma)).to.eventually.equal(connection)
expect(resolvedDNSAddrs).to.be.true('Did not resolve DNSAddrs')
expect(dialedBadAddress).to.be.false('Dialed address with wrong peer id')

resolvers.delete('dnsaddr')
})
})
2 changes: 1 addition & 1 deletion packages/libp2p/test/connection-manager/direct.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ describe('dialing (direct, WebSockets)', () => {

const remotePeerId = peerIdFromString(remoteAddr.getPeerId() ?? '')
await localComponents.peerStore.patch(remotePeerId, {
multiaddrs: Array.from({ length: 11 }, (_, i) => multiaddr(`/ip4/127.0.0.1/tcp/1500${i}/ws/p2p/12D3KooWHFKTMzwerBtsVmtz4ZZEQy2heafxzWw6wNn5PPYkBxJ5`))
multiaddrs: Array.from({ length: 11 }, (_, i) => multiaddr(`/ip4/127.0.0.1/tcp/1500${i}/ws/p2p/${remotePeerId.toString()}`))
})

await expect(connectionManager.openConnection(remotePeerId))
Expand Down