RT connectivity changes #482
Conversation
aschmahmann
force-pushed
the
feat/mode-switching
branch
from
5577ec7
to
a65ba4e
Compare
aschmahmann
force-pushed
the
feat/rt-connectivity-changes
branch
from
072adf5
to
31d039b
Compare
|
(looks good otherwise) |
| @@ -17,7 +17,7 @@ var DefaultBootstrapPeers []multiaddr.Multiaddr | |||
|
|
|||
| // Minimum number of peers in the routing table. If we drop below this and we | |||
| // see a new peer, we trigger a bootstrap round. | |||
| var minRTRefreshThreshold = 4 | |||
| var minRTRefreshThreshold = 10 | |||
There was a problem hiding this comment.
@aarshkshah1992 do you think there are reasons people may want to be able to configure this for themselves? No need to prematurely expose things, but when we start changing the constants around it's generally worth an evaluation
There was a problem hiding this comment.
This is too internal a value for users to want to/to be able to manipulate. IMO, we should not expose this as a functional option.
dht.go
Outdated
| return dht, nil | ||
| } | ||
|
|
||
| func makeRoutingTable(h host.Host, cfg *opts.Options) (*kb.RoutingTable, error) { |
There was a problem hiding this comment.
Any reason we're using *opts.Options instead of opts.Options? Seems like we may as well be consistent with makeDHT, so I'd make them both use either pointer or value semantics. Since we're only doing gets on a small data structure I figured use value semantics, but I could be swayed if you think this is better.
There was a problem hiding this comment.
I don't care either ways. Let's go with opts.Options.
| dht.routingTable.Update(p) | ||
| // peerFound signals the routingTable that we've found a peer that | ||
| // supports the DHT protocol. | ||
| func (dht *IpfsDHT) peerFound(ctx context.Context, p peer.ID) { |
There was a problem hiding this comment.
@aarshkshah1992 and I discussed this a bit offline. As long as the routing table has a bounded queue size for connected peers it is possible that a peer we are connected to will not be added to either the routing table or the replacement queue. If that occurs then we have no event (other than fixLowPeers if the entire routing table is almost completely empty) that cause us to re-add existing peers to the routing table/replacement queue.
Some fixes for this include:
- Maintaining state: e.g. allowing the replacement cache to contain an unbounded number of connected peers
- Polling state: loop through our connected peers more frequently (e.g. periodically, when a bucket goes from full to nothing in the replacement cache, etc.)
There was a problem hiding this comment.
I don't like the idea of an unbounded replacement cache as it will cause leaks for Cpls that don't see missing peers often.
2 sounds like a good idea. We should add a connected peer that is neither in the replacement cache nor in the RT to the cache when we drop a peer from the cache i.e. make it event driven. The DHT could pass in a callback to the RT to get a candidate for this purpose.
WDYT @Stebalien ? Should we do this right away ?
There was a problem hiding this comment.
I'd do this but punt it to a future patch. Could you file an issue?
My only concern is that we don't want to frequently loop over the entire list of connections if we happen to not know of any DHT server peers.
There was a problem hiding this comment.
Here's the issue:
Stebalien
force-pushed
the
feat/rt-connectivity-changes
branch
from
dcc2a47
to
cf37fca
Compare
@Stebalien @aschmahmann @willscott
For #283, #434 & libp2p/go-libp2p#779.
Corresponding Routing Table PR:
libp2p/go-libp2p-kbucket#54
Closed #465 in favour of this.