So, in general we really dont want to override things, it gets confusing (like right now as im trying to figure out what that special case was for). So here: https://github.com/ipfs/go-ipfs/pull/1001/files#r27631064 we can probably just opt towards removing the special case entirely. I still need to spend some time groking the test to convince myself its right, it may take me a day or so to allocate that time.

@jbrady42, I'd like to see what happens if we always just set closer to the closer peers, i.e.:

closest = dht.betterPeersToQuery(pmes, p, CloserPeerCount)

Also, would you mind maybe writing one or two more tests to exercise this a bit? maybe a few different network configurations? I think this will work, and i'm glad we're working towards relay (Yay!!!), but I just want to be super careful not to break anything in the DHT.

Thanks for the feedback. @whyrusleeping, I tried out your suggesting above but did not get any results. It looks like peers are being filtered out in betterPeersToQuery because they are not closer than self. Another possible issue here is the number of connected peers returned is limited by the CloserPeerCount. Even with the filter disabled it only gets 4 results, so there’s a chance it is connected to the peer we are looking for, but just not reporting it.

In the meantime, I’ll work on some more test network topologies and add them soon.

Ah, you are very right about that, we do filter out peers that arent closer... In that case, I think your solution makes sense, but as @jbenet points out you will need to use the routing tables peer list.

in response to the discussion on irc, the routing table will only have peers that we have interacted with through the DHT. The connect method you are using in the tests to connect the dhts together should be sending out pings, which registers the other peer in the routing table. If that does not seem to be the case, you may have found a bug.

the routing table will only have peers that we have interacted with through the DHT.

When connecting to someone, the network triggers a notification, which calls dht.Update(peer) -- the timing on this can be tricky in tests -- given goroutine scheduler. So theoretically the dht should consider all peers we connect to now. but yeah there may be a bug somewhere.

Btw @jbrady42 -- messing with the dht is pretty hard, mostly because our code needs to be way better. by all means go for it, but if your aim is to help (not necessarily attack the dht) may be easier to start elsewhere while you get used to the codebase.

Yeah I agree with you there. I’m not specifically going after the dht. My goal is to get the relay hooked in. I figured this was a good place to start since it can be used in a search for potential relay hosts, and was basically done already.

Regarding the mismatch in peer count on the tests, I have done some testing with the number and it seems to work consistently up to about 30 nodes. After that it comes up a few short some of the time, but not every time. It does what it is supposed to basically, so if there is an issue I think it is elsewhere. For the test, I’m adding another net configuration, but think it's better to turn down the number of nodes to around 20 so it does not fail with a mismatch.

@jbrady42 thanks for working on this! the relay is something i've wanted to have in for a while and this will get us that much closer to it.

closing due to inactivity, please reopen as necessary

note: all pull requests older than three weeks may be closed in an effort to keep our open pull requests more focused.