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

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Aug 10, 2023

Some multiaddrs like /dnsaddr/bootstrap.ipfs.io resolve to multiple PeerIds.

E.g:

% 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 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.

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.
Copy link

@whizzzkid whizzzkid left a 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

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

Comment on lines +321 to +324
const addrPeerId = addr.multiaddr.getPeerId()
if (peerId != null && addrPeerId != null) {
return peerId.equals(addrPeerId)
}

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.

Copy link
Member

@SgtPooki SgtPooki left a 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) {
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

Copy link

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

lgtm!

@achingbrain achingbrain merged commit a31b420 into master Aug 13, 2023
19 checks passed
@achingbrain achingbrain deleted the fix/filter-out-bad-dnsaddrs branch August 13, 2023 07:09
This was referenced Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants