Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support SHA-256 hashes in xt #43

Merged
merged 3 commits into from
May 12, 2021
Merged

Support SHA-256 hashes in xt #43

merged 3 commits into from
May 12, 2021

Conversation

bershanskiy
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Added ability to parse and encode BitTorrent V2 xt hashes. These are SHA-256 hashes written in multi-hash format.

Which issue (if any) does this pull request address?
This improves compatibility with BitTorrent V2.

Is there anything you'd like reviewers to focus on?
This adds two new attributes to the output object, infoHashV1 (which is an alias to infoHash) and infoHashV2. If you would like to store the data in some other way, I'll gladly edit this patch accordingly.

You might find the following sources helpful:

Copy link
Member

@feross feross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this PR!

  1. I'm not a fan of the infoHashV1 duplication. Can you update this to leave infoHash intact and just add a new infoHashV2 key?

  2. Can you add new tests to verify that this change works as expected?

@bershanskiy
Copy link
Contributor Author

I don't have time to work on it today, but I think I'll be able to do this tomorrow.

I'm not a fan of the infoHashV1 duplication. Can you update this to leave infoHash intact and just add a new infoHashV2 key?

I'm not a fan of duplication either, but do you think infoHash is descriptive enough? Should I add a getter/setter alias for it called infoHashV1? That would avoid duplication, but the result would not be a simple object (JSON.stringify-able, though).

Can you add new tests to verify that this change works as expected?

Yes, of course!

@feross
Copy link
Member

feross commented Nov 17, 2020

I'm not a fan of duplication either, but do you think infoHash is descriptive enough?

I think it's clear to have two keys -- infoHash and infoHashV2. Especially because we'll document it README.md. No need to introduce a getter and make the object no longer serializable.

Looking forward to your updated PR! Thanks for sending this in :)

Support for parsing/encoding URLs with 'xt=urn:btmh:1220*' argument containing
SHA-256 hesh (32-byte value encoded in hex, with 1220 multi-hash prefix).

BEP 52 specification: http://bittorrent.org/beps/bep_0052.html
@bershanskiy
Copy link
Contributor Author

I force-pushed a single commit with a descriptive message. Should you need it, the original commit you reviewed earlier is available on my v2-old branch.

@bershanskiy
Copy link
Contributor Author

@feross I the PR according to your comments. Should I update README.md to document this new feature?

@bershanskiy
Copy link
Contributor Author

I just realized that the hash in the test cases urn:btih:d2474e86c95b19b8bcfdb92bc12c9d44667cfa36 identifies an actual torrent. (I thought it was fake because of fake trackers like *.example*.com.) I can download the file and update the SHA-256 hash if you like, but I don't see any need in that (since the tests don't load the book file anyway).

@leoherzog
Copy link

FWIW, if I run that "Leaves of Grass" epub file from the test magnet through the Python reference from the v2 blog post, I get:

$ python3 bep_0052_torrent_creator-1.py Leaves\ of\ Grass\ by\ Walt\ Whitman.epub
v1 infohash 0299869ce9148034ac1d4b8fd1016a2a451faeb2
v2 infohash 5a303ae25d9f1fad9be55a77a259db39e60998e0e50dead7d2802e9f22630e00

Seems to be a different v1 hash as well? Dunno what's up with that.

@bershanskiy
Copy link
Contributor Author

@leoherzog

Seems to be a different v1 hash as well? Dunno what's up with that.

That's because of different "block" or "piece" size (which creates a totally different torrent structure and hence totally different hashes). d2474e86c95b19b8bcfdb92bc12c9d44667cfa36 uses 23 pieces 16 KiB each, while 0299869ce9148034ac1d4b8fd1016a2a451faeb2 uses 6 pieces 64 KiB each.

There is no problem with that, since it's just a semi-arbitrary choice permitted by the specification. Yes, ideally people would use the same block size to have matching hashes and create one larger swarms (instead of multiple tiny swarms) leading to more efficient peer choice.

@bershanskiy
Copy link
Contributor Author

@feross Is there anything I should do to advance this PR?

index.js Outdated Show resolved Hide resolved
@bershanskiy
Copy link
Contributor Author

@DiegoRBaquero Thanks for review. Should I rebase, squash and force-push to simplify history?

Copy link
Member

@DiegoRBaquero DiegoRBaquero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, LGTM!

@DiegoRBaquero DiegoRBaquero merged commit 48f08a1 into webtorrent:master May 12, 2021
@bershanskiy bershanskiy deleted the bittorrent-v2 branch May 12, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants