bittorrent-dht tests stall after 3.0.3 update

Not sure what was in the 3.0.3 update, but now bittorrent-dht (and webtorrent) tests are stalling. If I install k-bucket 3.0.2 then the tests run fine.

I'm looking into this now. But any ideas what the issue could be?

It looks like the code was substantially modified, so it's not easy to see where the behavior change might be. The effect in bittorrent-dht seems to be that announces fail.

I decided to just lock to version 3.0.2 for the time being.

Any help would be appreciated, @fanatid, @tristanls. Thanks!

@feross bittorrent-dht tests fails?
from my side all fine:

$ npm run test-live

> bittorrent-dht@7.3.1 test-live /home/kirill/coloredcoins/bittorrent-dht
> tape test/live/*.js

TAP version 13
# Find peers before ready is emitted
ok 1 Found at least one other DHT node
ok 2 Found at least one peer that has the file
ok 3 Found a peer in 578 ms

1..3
# tests 3
# pass  3

# ok

$ npm ls --depth=0 |grep k-bucket
├── k-bucket@3.0.3

@fanatid Most of our tests are in npm test not npm run test-live

Oh, sorry! Now I see that tests not passes.
What about k-rpc? As I understand, this package works fine?

k-rpc tests seem to be fine, but they're not as thorough as the bittorrent-dht tests.

The thing is, bittorrent-dht tests passed before the 3.0.3 update, so clearly there was some kind of behavior change in this commit. Now, maybe bittorrent-dht was relying on undocumented behavior of k-bucket or something, but it seems unlikely since we only use public APIs.

I've observed the following (after adding some log statements to bittorrent-dht client.js):

v3.0.3

# announce with implied port
START _closest new KBucket(...)
END _closest new KBucket(...)
START _closest onreply.table.add
END _closest onreply.table.add
START _put this._rpc.queryAll(table.closest(infoHash)...)
RESULT table.closest(infoHash) 
START _closest new KBucket(...)
END _closest new KBucket(...)
END _put this._rpc.queryAll(table.closest(infoHash)...)
START _closest onreply.table.add
END _closest onreply.table.add
... # hang

v3.0.2

# announce with implied port
START _closest new KBucket(...)
END _closest new KBucket(...)
START _closest onreply.table.add
END _closest onreply.table.add
START _put this._rpc.queryAll(table.closest(infoHash)...)
RESULT table.closest(infoHash) [object Object]
END _put this._rpc.queryAll(table.closest(infoHash)...)
START _addPeer this._peers.add [object Object] 6Jo6ugA1BVeDu7nuOukM++LLoB0= [object Object]
END _addPeer this._peers.add [object Object] 6Jo6ugA1BVeDu7nuOukM++LLoB0= [object Object]
ok 7 should be equivalent

It appears to me that table.closest(infoHash) in v3.0.3 is no longer returning a value (nothing is logged in RESULT table.closest(infoHash), whereas v3.0.2 it returns an object RESULT table.closest(infoHash) [object Object].

Which points at a change in behavior of kbucket.closest or kbucket.add.

I think I know why it's happened.

https://github.com/tristanls/k-bucket/blob/v3.0.2/index.js#L166
[1,2,3].slice(0, n) // => [1,2,3] -- where n = undefined
https://github.com/tristanls/k-bucket/blob/v3.0.3/index.js#L142
contacts.length < n // => false -- where contacts = [] and n = undefined

edit: started work on PR for extra tests

In the code n is documented as required:

k-bucket/index.js

Line 136 in eb85f46

// Return: Array of maximum of `n` closest contacts to the node id

, but that didn't make it to README documentation: https://github.com/tristanls/k-bucket#kbucketclosestid-n , so the documented API does not require n.

edit: As @fanatid points out below, the documentation is ok. If the code threw an error when n was not specified, this problem would not have happened.

I think README.md is ok. Optional argument stay in brackets: https://github.com/tristanls/k-bucket/tree/v3.1.0#kbucket_determinenodenode-id-bitindex

bittorrent-dht uses closest(id, n) method (https://github.com/tristanls/k-bucket#kbucketclosestid-n) without providing n: https://github.com/feross/bittorrent-dht/blob/299f0c117dbb42f52c3ffebd311b06a89670e7ea/client.js#L304.

I think making n in closest(id, n) optional is ok. With the behavior then being that all contacts are returned, sorted according to distance. This is what bittorrent-dht seems to expect. And this was the behavior in v3.0.2.

Then n will be Infinity by default, right?

n = Infinity by default sounds right to me.

@fanatid, I misunderstood your earlier comment. You're right about README being ok. I think the problem ended up being that no error was thrown when the second parameter was not provided, so, while the API was documented correctly, the code did not enforce the API contract and accepted closest calls without second parameter. This lead to dependent code relying on that behavior... and here we are. :)

I still think making n optional is an ok thing to do going forward. It seems the least disruptive.

@feross I think @fanatid resolved the issue in the 3.2.0 release

This is resolved now!

Thanks for the quick fix, @fanatid and @tristanls! I would have been happy changing the way that we call .closest() but this fix works too!