Since SOCKS5 takes 2 round trips, SOCKS4 doesn't support IPv6 and the proxy has to support DNS resolving in HTTP CONNECT, I would like to implement the proxy protocol of HAProxy to support IPv6 and the proxy doesn't have to resolve domain names by itself.
Nick, I'm giving you this review but since it's bigger than all the other reviews we had this week feel free to do it over multiple review periods. Feel free to remove yourself if you don't have time to do this , and we will give it somewhere else.
First question: on your note above, what do you mean by "the proxy has to support DNS resolving in HTTP CONNECT"? We want all the DNS resolution to happen in Tor. Tor should really only be passing IP addresses; does it pass hostnames to HTTP CONNECT proxies?
Second question: is it actually a problem in practice that SOCKS5 takes two round trips?
First question: on your note above, what do you mean by "the proxy has to support DNS resolving in HTTP CONNECT"? We want all the DNS resolution to happen in Tor. Tor should really only be passing IP addresses; does it pass hostnames to HTTP CONNECT proxies?
Yes Tor will be passing only IP addresses.
I meant that if I want to deploy my own proxy server and if it's HTTP CONNECT, it's supposed to support DNS resolving because it will be probably used by some other proxy client (which is not Tor). So it's more reasonable for me to deploy a haproxy instead of HTTP CONNECT proxy.
Second question: is it actually a problem in practice that SOCKS5 takes two round trips?
I'm not sure. I don't have much experience on this :D. But in theory, according to the speed of light, if the server is in the US and the client is in Thailand, it will take at least 80ms more if it's two round trips.
Okay, this is looking better and better! I think it will be ready for inclusion in 0.4.3.x.
I added a suggestion to rename the "WANT_ACK" state.
The other request I have is to see if you can write tests for more functions. Right now, only around 1/4 of the new lines in your patch have test coverage. Coveralls didn't add its report to the ticket for some reason, but you can see the coverage report at https://coveralls.io/builds/25644803
If there's something you can't test, or think a test isn't necessary for, please let me know and we can talk about it.
The coverage is higher now. The rest of the new lines that is not covered are not actually new lines. They are from refactoring and belongs to socks4 and socks5.
Looking much better! I'm glad to see the test coverage here.
I've left a couple of small comments on the ticket. Also, this needs a "changes" file, as described in doc/HACKING/CodingStandards.md. After that's done, I think this will be ready to merge.
If I get your go-ahead to fix the lintChanges issue and make the connection_start_reading() call unconditional, I will fix those, make sure CI still passes, and merge.