-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Publish a BEP explaining how to add support for WebTorrent #881
base: beps
Are you sure you want to change the base?
Conversation
|
||
{ | ||
"announce": "", | ||
"info_hash": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the scrape response format from "infoHash" to "info_hash" to be consistent with the other messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I "think" (still not sure) that the response is created here : https://github.com/feross/bittorrent-tracker/blob/master/server.js#L698-L701, and that the "infoHash" field is used as a key in the forEachloop when creating a new entry in files list.
To me, it seems that either for a single or multi scrape request the response will contains a field list
where the keys are the info_hash
and the value an object with the stats of the related swarm.
I'm a bit confused. So trackers supporting this would be able to track IP addresses of "normal" clients and also WebRTC contacts, right? That's cool but if WebRTC tabs can't connect to current BitTorrent client nodes (because most torrent clients don't support WebRTC), what's the point? |
The point of this BEP is to document the process of signalling webrtc peers on a tracker to connect each other. Aside from the tracker implementation, the clients supporting this would become hybrid clients and could use webrtc to connect to browser clients. This would help a lot to make bittorrent content available in webtorrent network. Do you understand now? Do you have an idea to make it clearer for a reader to understand what we are aiming? |
The point is to make clients support webrtc by giving them a standard documentation ;) |
Just read it all. It's very technical to the point of a BEP. Is there any place where I can see the guideline for a BEP? Looking good @yciabaud |
Thank you for reviewing @DiegoRBaquero. |
@yciabaud Thanks so much for writing this up. Looks detailed and well done! |
Thank you very much @austlane, lets hope we will push that to bittorrent soon! |
{ | ||
"action": "error" | ||
"message": "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the error response message for it to look like the other messages like in other beps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I think that both failure reason
and an extra error field are necesseray. The first one wpuld stay for legacy reasons and would be used only for Bittorrent protocol related failures (either announce or scrape) while another extra field (or object, with code/message ?) could describe a signalling error.
@yciabaud Just to clarify, is this BEP based on the actual implementation ? I couldn't find the error message you describe in the source code of the bittorrent-tracker project, instead it send back a |
@wI2L I made some changes that I have commented inline in this PR. You are right, ATM the errors are labelled with I changed a bit some of the payloads too, all changes are inline commented. |
@yciabaud Ok. I have added comments on some. EDIT: Messages are text, but encoding of |
"uploaded": 0, | ||
"downloaded": 0, | ||
"left": 0, | ||
"event": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analyzing a bit the payloads from wss://tracker.openwebtorrent.com/
, this field is not present for announce done at regular intervals. However, for consistency with the BT protocol it should be mentionned that a missing event, or an event that is represented by an empty string is one of those made at regular intervals.
"offer_id": "", | ||
"peer_id": "", | ||
"to_peer_id": "", | ||
"answer": "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answer
is an object with the fields type
and sdp
.
Thanks @wI2L for these reviews, I agree that the types of the fields must be added to the BEP. |
np @yciabaud |
-------- | ||
The websocket tracker uses JSON payloads reflecting the HTTP request parameters | ||
and an additionnal action property used to switch between announce and other | ||
actions (ex. scrape). If the announce URL of the torrent contains the ws or wss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I STRONGLY recommend that we formally drop support for insecure websockets. It's almost 2017, there is no reason to introduce a potential security vulnerability or vector for surveillance. Especially with Let's Encrypt available for free, there is simply no excuse for running an insecure public service. WebRTC itself already disallows insecure connections, we should incorporate that momentum into this BEP.
I suggest we change this explicitly disallow 'ws' and only allow 'wss' here.
|
||
{ | ||
"ih1": { | ||
"announce": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/feross/bittorrent-tracker/blob/master/server.js#L691-L695 and https://github.com/feross/bittorrent-tracker/blob/master/server.js#L698-L701, there is no shuch field announce
.
scrape response:: | ||
|
||
{ | ||
"announce": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/feross/bittorrent-tracker/blob/master/server.js#L677-L680, there is no such field announce
.
Is there anything missing so this can be sent? |
@DiegoRBaquero Depends, would you like with @feross and @yciabaud to address my proposals I have made in the PR here: yciabaud#1 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Why is this blocked ? It seems important for webtorrent and isn't most of the work already done ? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Unlocking and re-opening this. |
This PR proposes a BEP document as discussed in #168
I would like to have some feedback on the content and on some changes I made in the messages.
I changed some properties to have a cleaner protocol, if we keep it, we will have to update bittorrent-tracker to fit with this document.
You can view the html output on github: https://github.com/yciabaud/webtorrent/blob/486a94d2b651e7aed86ba27b82e679f5d1d1e700/bep_webrtc.rst
@feross @DiegoRBaquero @bbarrows