identify: clarify that listenAddrs should be external addresses
#597
Conversation
| The addresses on which the peer is deemed to be reachable by other peers. | ||
| These are also sometimes referred to as _external_ addresses. | ||
| For backwards-compatibility reasons, implementations MAY also include addresses of local interfaces. |
There was a problem hiding this comment.
I am completely open to rewording this to ensure it is backwards-compatible, suggestions more than welcome!
| These are the addresses on which the peer is listening as multi-addresses. | ||
| The addresses on which the peer is deemed to be reachable by other peers. | ||
| These are also sometimes referred to as _external_ addresses. | ||
| For backwards-compatibility reasons, implementations MAY also include addresses of local interfaces. |
There was a problem hiding this comment.
There's no value in including local addresses.
| For backwards-compatibility reasons, implementations MAY also include addresses of local interfaces. |
There was a problem hiding this comment.
I assume what you meant to say is that peers need to be able to handle local interface addresses that are sent here. But that's true anyway, you always need to be able to handle nonsense that a peer sends you.
|
I've always been extremely reluctant to do that because it kind of breaks private networks (say, running a network of several nodes on your localhost machine). The text in this PR says "the addresses on which the peer is deemed to be reachable by other peers", but for this reason that is not the same thing as external addresses, like the PR implies it is. One way to solve this could be to still return local addresses if the peer that is sending the Besides saving a few hundred bytes of bandwidth every minute, which really seems more than negligible to me, what is the motivation behind this change? |
|
I agree with @tomaka. What "external" means should depend on what peer you're talking to. In fact, we implemented this in go-libp2p: libp2p/go-libp2p#2300 (and some follow-up PRs). Using listen addresses is bad terminology though. You could be listening on 0.0.0.0, but that's never an acceptable address to advertise (127.0.0.1 would be, if you're connected to another node on localhost). |
I think that is indeed good design. Each node has the responsibility to filter out nonsense. No information can be trusted, so we verify.
One aspect I do not see mentioned is privacy/security. Members of our community do not like leaking their private networking information. My node gives out a bunch of listen addresses, like my WLAN, Ethernet, and private VPN subnet setup, etc. I think the spec should at least acknowledge the possibility of wanting to be selective with what is shared. |
|
Let's try not to continue discussion on this closed PR, but I'll respond here since I'm hoping I can solve your problem.
I don't think this is an issue the spec needs to define. Although I could see a version of the spec making recommendations. This sounds like an implementation decision. Maybe bring up the ability to filter the advertised listenAddrs as a feature request to your preferred libp2p implementation? It seems like a reasonable request that wouldn't be hard to implement. |
## Description Implements #4010, which was closed. It was closed because it appeared that the Identify specification doesn't dictate this feature. But, in the discussion on the specs repo (libp2p/specs#597) it is mentioned that this might very well be an implementation detail. This PR introduces a `hide_listen_addrs` flag that will prevent our listen addresses to be included, effectively only sharing our external addresses. <!-- Please write a summary of your changes and why you made them. This section will appear as the commit message after merging. Please craft it accordingly. For a quick primer on good commit messages, check out this blog post: https://cbea.ms/git-commit/ Please include any relevant issues in here, for example: Related https://github.com/libp2p/rust-libp2p/issues/ABCD. Fixes https://github.com/libp2p/rust-libp2p/issues/XYZ. --> ## Notes & open questions An alternative implementation would be to allow us to filter the addresses we are sending out, by providing a closure I imagine. <!-- Any notes, remarks or open questions you have to make about the PR which don't need to go into the final commit message. --> ## Change checklist <!-- Please add a Changelog entry in the appropriate crates and bump the crate versions if needed. See <https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#development-between-releases>--> - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have added tests that prove my fix is effective or that my feature works - [x] A changelog entry has been made in the appropriate crates --------- Co-authored-by: Darius Clark <[email protected]>
## Description Implements libp2p#4010, which was closed. It was closed because it appeared that the Identify specification doesn't dictate this feature. But, in the discussion on the specs repo (libp2p/specs#597) it is mentioned that this might very well be an implementation detail. This PR introduces a `hide_listen_addrs` flag that will prevent our listen addresses to be included, effectively only sharing our external addresses. <!-- Please write a summary of your changes and why you made them. This section will appear as the commit message after merging. Please craft it accordingly. For a quick primer on good commit messages, check out this blog post: https://cbea.ms/git-commit/ Please include any relevant issues in here, for example: Related https://github.com/libp2p/rust-libp2p/issues/ABCD. Fixes https://github.com/libp2p/rust-libp2p/issues/XYZ. --> ## Notes & open questions An alternative implementation would be to allow us to filter the addresses we are sending out, by providing a closure I imagine. <!-- Any notes, remarks or open questions you have to make about the PR which don't need to go into the final commit message. --> ## Change checklist <!-- Please add a Changelog entry in the appropriate crates and bump the crate versions if needed. See <https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#development-between-releases>--> - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have added tests that prove my fix is effective or that my feature works - [x] A changelog entry has been made in the appropriate crates --------- Co-authored-by: Darius Clark <[email protected]>
When taken literally, the spec currently demands to include the listen addresses in this field. That is pretty useless unless the node is listening on an interface that is directly connected to the public Internet.
In most cases, a node will be behind NAT which might have port-forwarding configured to be externally reachable. In either case, the local listening addresses are not going to help other peers in connecting.
Please consider this change in isolation of other PRs / issues around the identify spec. Ideally, the entire spec would be overhauled because it e.g. doesn't mention peer records (also see #347). However, not all implementations support peer records in identify and effort on extending the spec has stalled due to various issues.
It would be great if we could just clarify this one element in isolation without opening the can of worms that the spec overhaul is. Thank you!
cc interest group: @vyzo @yusefnapora @tomaka @richardschneider @Stebalien @bigs
Related: libp2p/rust-libp2p#4010.