UDP Trackers #102
UDP Trackers #102
Conversation
The first part of the work is now complete! I'm following https://www.bittorrent.org/beps/bep_0015.html to follow the specification perfectly.
Haven't been able to fully test this, though I expect it should work fine. There are some minor quirks. For instance, I was unsure of how large the arrays for the announce message should be, so I made it 30 bytes (the bep was kind of unclear on this).
Kind of realized that I didn't very the transaction ID, so I did that and added a Result
UdpTracker struct and Tracker struct have been combined into one, with no breaking changes! (External to the library). Internally, there's now a udp bool, that's just equal to true or false, indicating whether a tracker uses UDP or not. I still can't download from UDP only trackers for some damn reason, and there's still issues with finding working UDP trackers and connecting to them, but now, the program can technically work.
This is a bit easier to understand than the tuple used previously.
So when I first started working on this months ago, I didn't realize that the torrent protocol wanted signed ints, so basically, I just changed all the unsigned ints into signed (where it was wanted by the protocol), and edited some of my old code to be nicer.
|
Hi, could someone please help me to understand if my code is correct or not? I feel as though I've fixed all of the issues, and have a properly working tracker udp protocol, but am having difficulties downloading some UDP only torrents. |
There was a problem hiding this comment.
Thank you for working on this, @billyb2!
I haven't tested the actual code yet, I'll do that as a next step.
I think it would make more sense if the different tracker types were distinct types. HttpTracker and UdpTracker. That makes the distinction a bit clearer. I'd then make a Tracker (object safe) trait and use Box<dyn Tracker> instances throughout the code. Since trackers are generally not on the hot path and are called infrequently, the small price of the dynamic dispatch is of no concern here, but the code becomes clearer as a result.
What do you think?
Then, I think the parsing of the binary protocol could be achieved with tokio frames. That would make it easier to see at a glance what the announce logic does.
| #[derive(Clone)] | ||
| pub struct TrackerUrl { | ||
| pub url: Url, | ||
| pub protocol: NetProtocol, |
There was a problem hiding this comment.
I don't think this is necessary. The Url type already contains the scheme.
There was a problem hiding this comment.
The Url struct is reqwest only, so it only works for HTTP, not for UDP
| @@ -0,0 +1,2 @@ | |||
| [build] | |||
| jobs = 1 | |||
There was a problem hiding this comment.
I didn't mean to include that, I usually build with 1 thread since I'm doing homework or playing games while I program.
| addrs.shuffle(&mut thread_rng()); | ||
|
|
||
| //TODO: Make an error for not finding an actual IPV4 address | ||
| let addr = *addrs.iter().find(|a| a.is_ipv4()).unwrap(); |
There was a problem hiding this comment.
I understand this is a draft, so I would just like to put a note here as a reminder to take out the unwrap once the PR nears completion.
There was a problem hiding this comment.
Oh yea, of course. I definitely won't resolve this until I actually fix it.
|
|
||
| //Supporting around a few hundred peers, just for the test | ||
|
|
||
| let mut response_buf: [u8; MAX_NUM_PEERS] = [0; MAX_NUM_PEERS]; |
There was a problem hiding this comment.
We should support an arbitrary number of peers.
There was a problem hiding this comment.
I could probably just turn it into a vector if you'd like
| .await | ||
| .unwrap(); | ||
|
|
||
| //The magic protocol id number |
There was a problem hiding this comment.
Could you please leave a white space between the last forward slash and the first character of the comment? Same goes for the other places.
(Currently not ready, still working out some stuff).
This pull request should add the ability to connect to UDP trackers, which is a major stepping point for the crate.