identify: only store _reported_ multiaddrs #574
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We still tell the remote host about the observed addr but we don't store it. That way, we give them a chance to decide if they want to actually use and advertise it. Ideally, we'd distinguish between local information and signed routing information but we don't do that yet. Hopefully this will prevent nodes behind relays from telling everyone else to connect to their peers through these relays.
ghost
assigned 
vyzo
approved these changes
Apr 5, 2019
|
Actually, this is a bit interesting... We only add this remote address if it has the same protocol sequence as an address advertised by the peer. That means we shouldn't add the address here unless the peer adds the relay to their observed address set. So, this may still may prevent the address explosion issue however, it won't fix the actual problem. To do that, we just need to remove observed relay addresses. |
This is should prevent us from, e.g., announcing relay addresses _just_ because a peer tells us we're reachable through a relay.
vyzo
reviewed
Apr 5, 2019
| if !HasConsistentTransport(c.RemoteMultiaddr(), ids.Host.Addrs()) { | ||
| log.Debugf("ignoring observed multiaddr that doesn't match the transports of any addresses we're announcing", c.RemoteMultiaddr()) | ||
| return | ||
| } |
There was a problem hiding this comment.
so what happens if we are behind a NAT and advertise a relay addr, and connect to a peer through a different relay (one which we don't advertise)? It seems that we might pollute our advertised set to include this relay too.
|
Let's discuss in #575. I'm not sure if this is right. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We still tell the remote host about the observed addr but we don't store it. That way, we give them a chance to decide if they want to actually use and advertise it.
Ideally, we'd distinguish between local information and signed routing information but we don't do that yet.
Hopefully this will prevent nodes behind relays from telling everyone else to connect to their peers through these relays.