WIP: fixing getclosestpeers full-peerset bug #52
Conversation
|
Aside from the limited size peerset not being quite right, the main issue here is that we're pulling K peers from the routing table, not alpha (as specified by the paper) |
whyrusleeping
force-pushed
the
fix/closest-peers-issue
branch
from
cbb5c0c
to
2dea899
Compare
| @@ -312,19 +312,18 @@ func (dht *IpfsDHT) betterPeersToQuery(pmes *pb.Message, p peer.ID, count int) [ | |||
|
|
|||
| // no node? nil | |||
| if closer == nil { | |||
| log.Warning("no closer peers to send:", p) | |||
There was a problem hiding this comment.
Sure it should be warning? Won't this condition be end of every search?
There was a problem hiding this comment.
No, this can only happen if the routing table is empty, and i think having an empty routing table while trying to respond to a peer who definitely has you in their routing table might be worthy of a warning log
dht.go
Outdated
| var filtered []peer.ID | ||
| for _, clp := range closer { | ||
|
|
||
| // == to self? thats bad | ||
| if p == dht.self { |
There was a problem hiding this comment.
p is a argument in this case, are you sure about that?
|
CI is failed because i need to update go-libp2p-peer |
|
|
||
| // == to self? thats bad | ||
| if clp == dht.self { | ||
| log.Warning("attempted to return self! this shouldn't happen...") |
There was a problem hiding this comment.
Wouldn't that happen if we are in the closest 20 (?) peers, or is it filtered out too?
There was a problem hiding this comment.
Its filtered out, we shouldnt actually ever hit this.
|
Apart from test failure. |
whyrusleeping
force-pushed
the
fix/closest-peers-issue
branch
from
2670aef
to
24b7731
Compare
whyrusleeping
force-pushed
the
fix/closest-peers-issue
branch
3 times, most recently
from
922d029
to
c05335f
Compare
First, we use Alpha instead of K as the number of peers we grab from the routing table (as per the kademlia paper). Second, we don't use a size limited set for the 'GetClosestPeers' query. We're going to process more than K peers before we find the K closest peers. Third, Change GetClosestPeers to actually return the K Closest peers, not a hodge podge of peers that it found on the way to finding the closest peers.
whyrusleeping
force-pushed
the
fix/closest-peers-issue
branch
from
c05335f
to
00b46e0
Compare
So far i've reproduced the issue in a test. The problem is that the 'limited size peerset' is incorrect. It gets filled up with peers from our routing table. Which also brings up the fact that we shouldnt be returning peers from our routing table immediately, they arent super likely to always be the closest peers to our query.