enhance proxy support / network init #1781

Diapolo · 2012-09-03T20:42:00Z

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 · 2012-09-03T20:43:35Z

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.

laanwj · 2012-09-04T19:40:34Z

src/init.cpp

-        "  -tor=<ip:port>         " + _("Use proxy to reach tor hidden services (default: same as -proxy)") + "\n"
+        "  -proxy=<ip:port>       " + _("Connect through SOCKS proxy") + "\n" +
+        "  -socks=<n>             " + _("Select SOCKS version for -proxy (4 or 5, default: 5)") + "\n" +
+        "  -proxy6=<ip:port>      " + _("Use separate SOCKS5 proxy to reach IPv6 peers (default: -proxy if no -socks=4)") + "\n" +


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?

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.

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.

@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...

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?

Now I got it yes, my brain told me hey, when SUCKS4 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 · 2012-09-05T01:31:12Z

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

Diapolo · 2012-09-05T20:58:03Z

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

BitcoinPullTester · 2012-09-07T07:21:10Z

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 · 2012-09-08T07:47:37Z

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

BitcoinPullTester · 2012-10-21T12:10:12Z

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

sipa · 2012-10-28T23:37:10Z

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 · 2012-10-29T06:41:02Z

@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 · 2012-11-01T01:26:14Z

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

- 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 · 2012-11-04T16:28:44Z

@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 · 2012-11-04T18:53:36Z

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 · 2012-11-06T00:16:08Z

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 · 2012-11-13T05:57:33Z

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

Diapolo · 2012-11-13T07:25:25Z

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

jgarzik · 2012-11-16T02:43:39Z

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

Diapolo · 2012-11-16T07:04:56Z

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

laanwj reviewed Sep 4, 2012
View reviewed changes

Diapolo mentioned this pull request Sep 12, 2012

Qt: extend proxy settings - core dev help needed and wanted :) #1721

Closed

Diapolo closed this Nov 16, 2012

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance proxy support / network init #1781

enhance proxy support / network init #1781

Diapolo commented Sep 3, 2012

Diapolo commented Sep 3, 2012

laanwj Sep 4, 2012

Diapolo Sep 4, 2012

laanwj Sep 4, 2012

sipa Sep 4, 2012

laanwj Sep 4, 2012

Diapolo Sep 4, 2012

BitcoinPullTester commented Sep 5, 2012

Diapolo commented Sep 5, 2012

BitcoinPullTester commented Sep 7, 2012

BitcoinPullTester commented Sep 8, 2012

BitcoinPullTester commented Oct 21, 2012

sipa commented Oct 28, 2012

Diapolo commented Oct 29, 2012

BitcoinPullTester commented Nov 1, 2012

Diapolo commented Nov 4, 2012

sipa commented Nov 4, 2012

BitcoinPullTester commented Nov 6, 2012

luke-jr commented Nov 13, 2012

Diapolo commented Nov 13, 2012

jgarzik commented Nov 16, 2012

Diapolo commented Nov 16, 2012

enhance proxy support / network init #1781

enhance proxy support / network init #1781

Conversation

Diapolo commented Sep 3, 2012

Diapolo commented Sep 3, 2012

laanwj Sep 4, 2012

Choose a reason for hiding this comment

Diapolo Sep 4, 2012

Choose a reason for hiding this comment

laanwj Sep 4, 2012

Choose a reason for hiding this comment

sipa Sep 4, 2012

Choose a reason for hiding this comment

laanwj Sep 4, 2012

Choose a reason for hiding this comment

Diapolo Sep 4, 2012

Choose a reason for hiding this comment

BitcoinPullTester commented Sep 5, 2012

Diapolo commented Sep 5, 2012

BitcoinPullTester commented Sep 7, 2012

BitcoinPullTester commented Sep 8, 2012

BitcoinPullTester commented Oct 21, 2012

sipa commented Oct 28, 2012

Diapolo commented Oct 29, 2012

BitcoinPullTester commented Nov 1, 2012

Diapolo commented Nov 4, 2012

sipa commented Nov 4, 2012

BitcoinPullTester commented Nov 6, 2012

luke-jr commented Nov 13, 2012

Diapolo commented Nov 13, 2012

jgarzik commented Nov 16, 2012

Diapolo commented Nov 16, 2012