identify: be more careful about the addresses we store #577
Conversation
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. This should reduce the address explosion issue where peers learn about multiple (bad) observed addresses for the same peer. It should also give peers more control over how they can be dialed.
ghost
assigned 
Stebalien
force-pushed
the
fix/only-reported-addrs
branch
from
679c214
to
8e9aa12
Compare
| if !HasConsistentTransport(maddr, 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.
Do we care about the expansion of relay addrs we are advertising? Some of them maybe transient, because we happened to connect to some relay in order to talk to another NATed peer. But this is not a relay we are keeping connected because we selected it (and soon RESERVE capacity), but rather it's an incidental connection.
To be more specific here: If we are NATed/relayed and we connect to another NATed/relayed host, we will end up adding the other peer's relay into our observed address set and end up advertising that together with our primary relay connections.
There was a problem hiding this comment.
IMO, we should just get better at pushing updates. I actually like automatically discovering new relays like this as it helps spread the load a bit.
In the meantime, you're probably right. We don't want to advertise these addresses. Luckily, the default relay address filter should filter out all those addresses at the next layer up.
There was a problem hiding this comment.
Yeah, I have big hopes with that patch.
The issue with using the random relays we happened to be connected is that these connections are not expected to be stable.
It's great that we are spreading the load though.
|
@whyrusleeping thoughts here? This is a subtle little change that may have pervasive effects. |
| lmaddrs = append(lmaddrs, c.RemoteMultiaddr()) | ||
| } | ||
| // NOTE: Do not add `c.RemoteMultiaddr()` to the peerstore if the remote | ||
| // peer doesn't tell us to do so. Otherwise, we'll advertise it. |
There was a problem hiding this comment.
Proposal: Don't even record the remote peer's address in the peerstore if they aren't advertising it.
Rational: If we record it, we'll share it. I'm worried this is causing us to gossip bad ephemeral addresses around the network. I'm still seeing some addr-splosion peers.
Objections?
There was a problem hiding this comment.
This seems like a good idea, though I hope we're not forgetting the reason we added these in the first place.
I can't think of any good reason to be doing this right now, but let's make sure that the rationale for this decision is well documented (including what happens when we advertise these addresses without the authors consent)
There was a problem hiding this comment.
I've extended the comment with more rational so we don't run into this situation again.
…announcing This is should prevent us from, e.g., announcing relay addresses _just_ because a peer tells us we're reachable through a relay.
Stebalien
force-pushed
the
fix/only-reported-addrs
branch
from
8e9aa12
to
bcbf7a5
Compare
Note: I don't believe this is related to #575 but, IMO, it's still a good idea.