enhance proxy support / network init #1781

Diapolo commented at 8:42 PM on September 3, 2012: none

add -proxy6 to specify a SOCKS5 proxy for reaching IPv6 peers
enhance control flow for proxy init code
new init order Tor -> IPv6 -> Base proxy
remove redundant .IsValid() checks
don't set Tor proxy, when IsLimited(NET_TOR)
don't allow SOCKS versions != 4 or != 5
disable the usage of -proxy for Tor and IPv6, when SOCKS4 is used
update parameter help messages to reflect changes and tweak them to be more clear

Diapolo commented at 8:43 PM on September 3, 2012: none

Based upon this changes I'm planning to extend the Bitcoin-Qt GUI with the same proxy network options, but I want the core to get those first.

in src/init.cpp:None in c57082dc8c outdated

 225 | @@ -226,9 +226,10 @@ bool static Bind(const CService &addr, bool fError = true) {
 226 |          "  -dbcache=<n>           " + _("Set database cache size in megabytes (default: 25)") + "\n" +
 227 |          "  -dblogsize=<n>         " + _("Set database disk log size in megabytes (default: 100)") + "\n" +
 228 |          "  -timeout=<n>           " + _("Specify connection timeout in milliseconds (default: 5000))") + "\n" +
 229 | -        "  -proxy=<ip:port>       " + _("Connect through socks proxy") + "\n" +
 230 | -        "  -socks=<n>             " + _("Select the version of socks proxy to use (4-5, default: 5)") + "\n" +
 231 | -        "  -tor=<ip:port>         " + _("Use proxy to reach tor hidden services (default: same as -proxy)") + "\n"
 232 | +        "  -proxy=<ip:port>       " + _("Connect through SOCKS proxy") + "\n" +
 233 | +        "  -socks=<n>             " + _("Select SOCKS version for -proxy (4 or 5, default: 5)") + "\n" +
 234 | +        "  -proxy6=<ip:port>      " + _("Use separate SOCKS5 proxy to reach IPv6 peers (default: -proxy if no -socks=4)") + "\n" +

laanwj commented at 7:40 PM on September 4, 2012:

I'd simply say default: -proxy

Sure, socks4 cannot do ipv6 and tor, but that should mean that those are disabled when a v4 proxy is used. Not that those go around the proxy?

Diapolo commented at 7:52 PM on September 4, 2012:

Yes, when a SOCKS4 proxy is specified, -proxy will not be used for Tor and IPv6, which is what I wanted to clarify with if no -socks=4.

laanwj commented at 7:58 PM on September 4, 2012:

Yes but adding that doesn't fully define the behavior, which is why I asked. There are two possibilities:

If you use socks=4, a socks4 proxy is used for ipv4, and tor and ipv6 go directly, around the proxy
if you use socks=4, a socks4 proxy is used for ipv4, and tor and ipv6 are disabled (unless you specify a proxy for them manually)

I think (2) makes most sense. You don't expect connections to suddenly go around the proxy because you specified another socks version.

sipa commented at 7:58 PM on September 4, 2012:

@laanwj means that if -socks=4 is specified, and -proxy, and no -proxy6 or -tor, outgoing tor and ipv6 connections should be disabled, instead of being unproxied. Sounds reasonable to me, but extra special cases...

laanwj commented at 7:59 PM on September 4, 2012:

Yes it sucks. Maybe we should simply get rid of socks4 :p

I mean, this is 2012, who still uses socks4 these days? I'd keep the proxy type option for when someone wants to implement "HTTP CONNECT" proxies, which would be somewhat more useful in some cases (corporate firewalls etc). But socks4?

Diapolo commented at 8:05 PM on September 4, 2012:

Now I got it yes, my brain told me hey, when S<b>U</b>CKS4 and -proxy, SetLimited(Tor and IPv6), which currently is not the case ^^. But I like the idea, as really no one expects a non-proxy connection, when -proxy was set. If you want me to integrate that change that's good, as it makes sense (even as another special-case).

BitcoinPullTester commented at 1:31 AM on September 5, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c57082dc8c69934939ef7c7129d6dd9e46ee4fa1 for binaries and test log.

Diapolo commented at 8:58 PM on September 5, 2012: none

Updated and re-simplified the helptext for -proxy6 and -tor + disabled the usage of -proxy for Tor and IPv6, when SOCKS4 is used.

BitcoinPullTester commented at 7:21 AM on September 7, 2012: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/988841c75fdf5f3844a3f99149c156d371883bd0 for test log.

This pull does not merge cleanly onto current master

BitcoinPullTester commented at 7:47 AM on September 8, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/45ae329eb105f3412174ec080bd88edb390db643 for binaries and test log.

BitcoinPullTester commented at 12:10 PM on October 21, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/58bae3491657cd288a682bb21616e00f9be0d4e5 for binaries and test log.

sipa commented at 11:37 PM on October 28, 2012: member

This looks like a lot of code for something that will be rarely used. Can't you combine the tor/ipv6/ipv4 specific code into one loop over NET_TOR, NET_IPV6, NET_IPV4, with maybe a small bit of specific code for each?

Diapolo commented at 6:41 AM on October 29, 2012: none

@sipa Did you look if the code-flow is valid? It took me quite some time, so it would be nice if feature-wise this could get an ACK and I try to compress the code, to shorten it :).

Btw. I'm using a client with this codebase since a few weeks with Tor hs enabled, looking good.

BitcoinPullTester commented at 1:26 AM on November 1, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d2874a4b999a18ddb6dd9fc09d1c5b8f4ed10c8c for binaries and test log.

enhance proxy support / network init

- add -proxy6 to specify a SOCKS5 proxy for reaching IPv6 peers
- enhance control flow for proxy init code
- new init order Tor -> IPv6 -> Base proxy
- remove redundant .IsValid() checks
- don't set Tor proxy, when IsLimited(NET_TOR)
- don't allow SOCKS versions != 4 or != 5
- disable the usage of -proxy for Tor and IPv6, when SOCKS4 is used
- update parameter help messages to reflect changes and tweak them to
  be more clear

d513e73239

Diapolo commented at 4:28 PM on November 4, 2012: none

@sipa I have re-worked this pull to be a little shorter, it contains no loop though. Can you have a look at the functionality now?

sipa commented at 6:53 PM on November 4, 2012: member

I still don't really like this - it seems overly complex for functionality only a fraction of users will need.

I wonder whether it's not easier to have some way to overload -proxy if you really want network-specific rules. Something like -proxy ipv6:ip:port, maybe?

BitcoinPullTester commented at 12:16 AM on November 6, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/d513e73239b03774cf81c32a329f2464824057f7 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

luke-jr commented at 5:57 AM on November 13, 2012: member

What @sipa said. Also, this needs a rebase.

Diapolo commented at 7:25 AM on November 13, 2012: none

I have changes for this in the pipe, which greatly reduce the code complexity and special casing.

jgarzik commented at 2:43 AM on November 16, 2012: contributor

Would rather wait for a use case, than preemptively add this now

Diapolo commented at 7:04 AM on November 16, 2012: none

I'm going to re-work and re-open when finished :).

Diapolo closed this on Nov 16, 2012

DrahtBot locked this on Sep 8, 2021