Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesPublish a BEP explaining how to add support for WebTorrent #881
Conversation
|
|
||
| { | ||
| "announce": "", | ||
| "info_hash": "", |
This comment has been minimized.
This comment has been minimized.
yciabaud
Aug 8, 2016
•
Author
Contributor
I changed the scrape response format from "infoHash" to "info_hash" to be consistent with the other messages.
This comment has been minimized.
This comment has been minimized.
wI2L
Nov 6, 2016
•
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.
| } | ||
|
|
||
|
|
||
| multi-scrape response:: |
This comment has been minimized.
This comment has been minimized.
| error response:: | ||
|
|
||
| { | ||
| "error": "" |
This comment has been minimized.
This comment has been minimized.
yciabaud
Aug 8, 2016
•
Author
Contributor
Here I changed from "failure reason" to "error" to be cleaner.
This comment has been minimized.
This comment has been minimized.
alxhotel
Aug 29, 2016
Member
Looking at the UDP tracker BEP, I've found it uses a type of action called error whenever an error is return. And then a message attribute for the error message.
It could make sense adding it (at least the action attribute), just to be consistent in the response.
This comment has been minimized.
This comment has been minimized.
yciabaud
Aug 29, 2016
Author
Contributor
I agree with you, it is definitely more consistent with the other messages. Pushing it right now!
| "info_hash": "", | ||
| "offer_id": "", | ||
| "peer_id": "", | ||
| "sdp": "" |
This comment has been minimized.
This comment has been minimized.
yciabaud
Aug 8, 2016
•
Author
Contributor
I changed from "answer" to "sdp" as this property can be either an offer or a response.
This comment has been minimized.
This comment has been minimized.
wI2L
Oct 11, 2016
•
I am thinking that a message like this one is not really related to Bittorrent annoucing, and that maybe we could use two new action events offer and answer. It would simplify server side parsing of messages and ensure that we know exactly how to deal with sdp.
EDIT: It seems to be either offer or answer in the payloads returned by tracker.openwebtorrent.com, and this field is an object containing the fields type and sdp. type seems to be consistent with the name of the field.
This comment has been minimized.
This comment has been minimized.
lmatteis
commented
Aug 20, 2016
•
|
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? |
This comment has been minimized.
This comment has been minimized.
|
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? |
This comment has been minimized.
This comment has been minimized.
|
The point is to make clients support webrtc by giving them a standard documentation ;) |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
Thank you for reviewing @DiegoRBaquero. |
This comment has been minimized.
This comment has been minimized.
austlane
commented
Aug 28, 2016
|
@yciabaud Thanks so much for writing this up. Looks detailed and well done! |
This comment has been minimized.
This comment has been minimized.
|
Thank you very much @austlane, lets hope we will push that to bittorrent soon! |
| { | ||
| "action": "error" | ||
| "message": "" | ||
| } |
This comment has been minimized.
This comment has been minimized.
yciabaud
Aug 29, 2016
Author
Contributor
I changed the error response message for it to look like the other messages like in other beps.
This comment has been minimized.
This comment has been minimized.
wI2L
Oct 11, 2016
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.
This comment has been minimized.
This comment has been minimized.
wI2L
commented
Oct 11, 2016
|
@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 |
This comment has been minimized.
This comment has been minimized.
|
@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. |
This comment has been minimized.
This comment has been minimized.
wI2L
commented
Oct 11, 2016
•
|
@yciabaud Ok. I have added comments on some. EDIT: Messages are text, but encoding of |
| "uploaded": 0, | ||
| "downloaded": 0, | ||
| "left": 0, | ||
| "event": "", |
This comment has been minimized.
This comment has been minimized.
wI2L
Oct 11, 2016
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": "" |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks @wI2L for these reviews, I agree that the types of the fields must be added to the BEP. |
This comment has been minimized.
This comment has been minimized.
wI2L
commented
Oct 12, 2016
|
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 |
This comment has been minimized.
This comment has been minimized.
Miserlou
Oct 23, 2016
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": "", |
This comment has been minimized.
This comment has been minimized.
wI2L
Nov 6, 2016
•
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": "", |
This comment has been minimized.
This comment has been minimized.
wI2L
Nov 6, 2016
According to https://github.com/feross/bittorrent-tracker/blob/master/server.js#L677-L680, there is no such field announce.
This comment has been minimized.
This comment has been minimized.
|
Is there anything missing so this can be sent? |
This comment has been minimized.
This comment has been minimized.
wI2L
commented
May 24, 2017
|
@DiegoRBaquero Depends, would you like with @feross and @yciabaud to address my proposals I have made in the PR here: yciabaud#1 |
This comment has been minimized.
This comment has been minimized.
stale
bot
commented
Aug 2, 2018
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This comment has been minimized.
This comment has been minimized.
|
Why is this blocked ? It seems important for webtorrent and isn't most of the work already done ? |
This comment has been minimized.
This comment has been minimized.
stale
bot
commented
Feb 6, 2019
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This comment has been minimized.
This comment has been minimized.
|
Unlocking and re-opening this. |
yciabaud commentedAug 8, 2016
•
edited
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