-
Notifications
You must be signed in to change notification settings - Fork 438
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
Conversation
Some multiaddrs like `/dnsaddr/bootstrap.ipfs.io` resolve to multiple PeerIds. E.g: ```console % dig _dnsaddr.bootstrap.libp2p.io TXT ; <<>> DiG 9.10.6 <<>> _dnsaddr.bootstrap.libp2p.io TXT ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 53473 ;; flags: qr rd ra; QUERY: 1, ANSWER: 4, AUTHORITY: 0, ADDITIONAL: 1 ;; OPT PSEUDOSECTION: ; EDNS: version: 0, flags:; udp: 1232 ;; QUESTION SECTION: ;_dnsaddr.bootstrap.libp2p.io. IN TXT ;; ANSWER SECTION: _dnsaddr.bootstrap.libp2p.io. 261 IN TXT "dnsaddr=/dnsaddr/am6.bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb" _dnsaddr.bootstrap.libp2p.io. 261 IN TXT "dnsaddr=/dnsaddr/ny5.bootstrap.libp2p.io/p2p/QmQCU2EcMqAqQPR2i9bChDtGNJchTbq5TbXJJ16u19uLTa" _dnsaddr.bootstrap.libp2p.io. 261 IN TXT "dnsaddr=/dnsaddr/sg1.bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt" _dnsaddr.bootstrap.libp2p.io. 261 IN TXT "dnsaddr=/dnsaddr/sv15.bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN" ``` This can cause problems when dialing those addresses with a PeerId since resolving the dnsaddr returns addresses with embedded PeerIds that aren't for the node you are trying to dial so filter those out. The errors these cause are non-fatal but do consume resources dialing nodes we aren't going to be able to connect to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
return peerId.equals(addrPeerId) | ||
} | ||
|
||
return true |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
const addrPeerId = addr.multiaddr.getPeerId() | ||
if (peerId != null && addrPeerId != null) { | ||
return peerId.equals(addrPeerId) | ||
} |
There was a problem hiding this comment.
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
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) | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, comment regarding recent convo with peerId missing from some multiaddrs (issue incoming after breakfast)
// - 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related: #1961
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Some multiaddrs like
/dnsaddr/bootstrap.ipfs.io
resolve to multiple PeerIds.E.g:
This can cause problems when dialing those addresses with an explicit PeerId since resolving the dnsaddr returns addresses with embedded PeerIds that aren't for the node you are trying to dial so we need to filter those out.
The errors these cause are non-fatal but do consume resources dialing nodes we aren't going to be able to connect to.