Accept spec compliant BEP44 mutable get replies with salt

[chr15m]

@mafintosh this supercedes PR #176 and addresses #167 without the other cruft introduced by that patch set.

Summary

• The BEP0044 specification does not include salt in a mutable dht.get response.
• In the wild we observe non-webtorrent DHT nodes complying with spec and failing to return salt in the response.
• bittorrent-dht expects remote nodes to be non-spec compliant and return salt.

Because of this bittorrent-dht treats most correct mutable-dht.get responses as invalid as it is expecting salt to come back and match, but real-world DHT nodes do not return it.

This patch allows the user to specify the expected salt as a dht.get option and uses it in validating remote responses even if they do not include the salt field. This allows bittorrent-dht to at least accept valid responses from other nodes instead of rejecting them as it does currently.

The user will always know the salt value that they expect at call time because it is required to compute the correct hash value anyway.

Let me know if you need more information or any changes. Thanks!

Fixes #167.

[chr15m]

@mafintosh I've created a small project which reliably replicates the buggy condition and demonstrates that this patch fixes the situation: https://github.com/chr15m/bittorrent-dht-salt-bug

Hope this helps!

[mafintosh]

If it isn't spec compliant to return the salt let's not do it at all. Great work :)

[chr15m]

@mafintosh ok cool. Should I update the PR to make it completely spec compliant throughout?

[mafintosh]

@chr15m that'd be cool. ping me when you want a review

[chr15m]

@mafintosh updated PR to a) not send salt in response to mutable get, and b) not expect salt in response from mutable get. User is now required to supply salt as an option if it was used to compute the hash.

The tests pass but note that there is not actually test coverage of this part of the code.

[chr15m]

@mafintosh sorry to bug you I know you must be very busy - did you get a chance to review this PR? Is there anything I can do to help?

[mh-cbon]

seems good, it allows the get executor to set the required salt. It removes the salt from the get response.

Great job.

[feross]

@mafintosh Do you think we should merge this?

[chr15m]

[mafintosh]

Buffer.from insted of Buffer

[mafintosh]

@feross LGTM, added a tiny comment about using the new buffer api, but no biggie

[chr15m]

@mafintosh excellent, thank you, will fix the PR(s).

[chr15m]

@mafintosh updated PR.

[feross]

LGTM

[feross]

Released as 8.4.0

[robertkowalski]

hmmm... a few libs we have built rely on the behaviour, thats quite a breaking change.

[robertkowalski]

what we observe now is that data we did store with salts that we try to receive by hash (we just track hashes) is not possible any more and we get empty results

[robertkowalski]

seems there was a bug: #199

i'm chatting with mathias about it, lets see

[chr15m]

@robertkowalski if you put some console.logs into bittorrent-dht/client.js to log every response from the network, and try some DHT requests with salt before this patch is applied, you will see that most replies from the real BitTorrent network are ignored. Without this patch you are effectively limiting your application to use only a subset of the real DHT composed of clients which break the spec and return salt (e.g. the buggy WebTorrent clients). With the old codebase most DHT replies will be discarded by bittorrent-dht, despite being correct and valid.

The most robust solution is to re-architect your app such that the salt field is remembered as well as the hash. The reason to do this is it will make your application more robust as it will use all DHT nodes, not just those which break from spec and return the salt parameter. I found that mutable get requests were faster and more reliable after this since you get a higher level of redundancy from the real world DHT (more valid responses from storage nodes).

That said, I don't think it would be a terrible thing for WebTorrent to maintain backwards compatibility with the broken previous version for a while if this was documented. In that instance the documentation should tell users of the library that they are only using a broken subset of the DHT and that mutable get/put will be less reliable, only using a subset of the DHT.

The way to maintain backwards compatibility would be:

1. Continue to return salt in get replies (even though the spec does not specify to return this field).
2. If the user has not specified salt during their get and clients send salt in their response then use the returned salt.

This change would allow apps like @robertkowalski to continue to function in a limited capacity (using only a subset of the DHT responses) until they upgrade their model, and also allow people who want full use of the DHT to do so by including the salt option during get requests.

The most important thing is that the user is able to continue to manually override the salt value in opts to get better DHT performance from spec compliant nodes.

@mafintosh let me know if you want a patch to support this backwards compatability of the previous buggy behaviour.