don't add peers with only private addresses to your routing table #360
Conversation
dht.go
Outdated
| return true | ||
| } | ||
|
|
||
| for _, a := range dht.host.Peerstore().Addrs(p) { |
There was a problem hiding this comment.
Did we already deal with advertising tons of relay addresses? If not this can get quite expensive.
There was a problem hiding this comment.
oooh, I think we're mostly past that, but should we check the number of addresses here? If we have a ton, we probably don't want that peer in the routing table...
|
Also, this makes some tests fail with |
Yeah, because now peers being included in the routing table depends on identify having finished. Now theres a race between identify and the rest of the code handling adding peers to the routing table |
dht.go
Outdated
| } | ||
|
|
||
| func (dht *IpfsDHT) shouldAddPeerToRoutingTable(p peer.ID) bool { | ||
| return dht.hasOutboundConnToPeer(p) || |
There was a problem hiding this comment.
Based on new discoveries, I think we should ignore if:
- we don't have addresses for the peer; after merging identify: Update addr advertise logic to exclude localhost addrs selectively go-libp2p#657, peers that are not routable (e.g. no NAT mapping or pinhole) nor connected to a relay will advertise 0 addrs for themselves.
- if we only have localhost addresses for the peer (old, non-routable peers without the above patch).
- if remote multiaddr != any of peer's advertised addrs
- likely to be an inbound connection via an ephemeral NAT port mapping
- this correctly covers the LAN connection case, i.e. if we're on LAN, we will be connected to their advertised address, so we'll add them to the routing table. IMPORTANT: we should not return these peers in queries from peers who are not also in the LAN.
- if the remote advertises relay addresses (consequence of being behind a NAT, we don't want them in the routing table).
- if they don't support the DHT server protocol (upgraded peers).
dht.go
Outdated
| } | ||
|
|
||
| func (dht *IpfsDHT) UpdateConn(ctx context.Context, c network.Conn) { | ||
| logger.Event(ctx, "updatePeer", c.RemotePeer()) |
dht.go
Outdated
| } | ||
|
|
||
| var hasPublicAddr bool | ||
| for _, a := range dht.host.Peerstore().Addrs(c.RemotePeer()) { |
dht.go
Outdated
| return false | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
Assuming we want to return true ^^, we could reach here if the peer has a public address, yet with an ephemeral port mapping. I'm starting to think the c.RemoteMultiaddr() in addrs check is not that bad.
| } | ||
|
|
||
| func (dht *IpfsDHT) peerIsOnSameSubnet(c network.Conn) bool { | ||
| return manet.IsPrivateAddr(c.RemoteMultiaddr()) |
There was a problem hiding this comment.
How is this checking if we're on the same subnet as the peer? How about a manet.IsPrivateAddr(c.RemoteMultiaddr()) && c.RemoteMultiaddr() in addrs check here?
There was a problem hiding this comment.
if we are connected to someone, and the connection is a private IP address, we must be on the same subnet as them, as private IPs are never routed outside of their subnet
| // we established this connection, so they're definitely diallable. | ||
| return true | ||
| } | ||
| if dht.peerIsOnSameSubnet(c) { |
There was a problem hiding this comment.
if we do this here, we don't return false for peers with no addresses at all, or peers with relay addresses.
There was a problem hiding this comment.
peers with relay addrs are caught in L299. let me look at the 0-addr case.
There was a problem hiding this comment.
wait no, they're covered iff we're connected to them via a relay, but not if we are directly connected but they advertise relay addrs.
| } | ||
|
|
||
| func (dht *IpfsDHT) shouldAddPeerToRoutingTable(c network.Conn) bool { | ||
| if isRelayAddr(c.RemoteMultiaddr()) { |
There was a problem hiding this comment.
We should only check this for outbound connections. If we're behind a relay and operating in client mode, we still want to use relayed peers in our DHT.
No description provided.