extend core proxy options and handling

Diapolo commented at 2:43 pm on September 8, 2014: none

rework the proxy handling in init to cover more cases and work more thoroughly
extend RPC call getnetwork info to see if IPv6 and Tor proxy is the default proxy

See #2575 for discussion!

Diapolo commented at 2:44 pm on September 8, 2014: none

@laanwj As you suggested, a new and clean pull and empty discussion ;).

Diapolo commented at 12:46 pm on September 23, 2014: none

@laanwj Can you please have a look again, I hope we can sort this out now finally :).

BitcoinPullTester commented at 1:00 pm on September 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4871_2711cda5b8c54efb2164d7bb3593b3db50f8e15c/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Diapolo commented at 11:29 am on October 6, 2014: none

Rebased and removed an obsolete c_str().

Diapolo commented at 8:33 am on October 10, 2014: none

@laanwj This and the GUI part are pulls that are not cosmetic, but need review or ACKs.

in src/rpcnet.cpp: in fd55abfd0f outdated

360         obj.push_back(Pair("name", GetNetworkName(network)));
361         obj.push_back(Pair("limited", IsLimited(network)));
362         obj.push_back(Pair("reachable", IsReachable(network)));
363         obj.push_back(Pair("proxy", proxy.IsValid() ? proxy.ToStringIPPort() : string()));
364+        if (network != NET_IPV4)
365+            obj.push_back(Pair("proxy-from-default", proxy == proxyDefault));

laanwj commented at 8:54 am on October 10, 2014:

As said before - I’m still not convinced that proxy-from-default is useful information. Clients requesting from RPC just want to know the currently effective state, it shouldn’t matter how it came that way. For troubleshooting command line argument interaction there’s always debug.log.

Diapolo commented at 8:46 am on October 14, 2014:

Alright, removing this will finally make the pull get merged then?

laanwj commented at 8:51 am on October 14, 2014:

The main problem here is that we don’t have testcases with proxy, so this needs to be manually tested. Can you list what you tested?

Diapolo commented at 12:12 pm on November 28, 2014: none

Updated to include changes that were merged in #5358. @gmaxwell Mind reviewing this now?

laanwj added the label P2P on Dec 3, 2014

fanquake commented at 8:34 am on January 23, 2015: member

Needs rebase

Diapolo commented at 10:28 am on January 23, 2015: none

Rebased, needs review / testing.

laanwj commented at 9:55 am on March 13, 2015: member

To be clear what this is blocked on: We need (RPC) tests for the proxy functionality, before we can merge changes like this.

Diapolo commented at 1:11 pm on April 23, 2015: none

@laanwj Rebased to include the recent -proxyrandomize change. Also as a proxy test was added, this should be less controversial if tests pass I guess :).

extend proxy handling and processing

- rework the proxy handling in init to cover more cases and work more
  thoroughly

0ac426b87c

in src/rpcnet.cpp: in 0565554e4a outdated

377         obj.push_back(Pair("limited", IsLimited(network)));
378         obj.push_back(Pair("reachable", IsReachable(network)));
379         obj.push_back(Pair("proxy", proxy.IsValid() ? proxy.proxy.ToStringIPPort() : string()));
380         obj.push_back(Pair("proxy_randomize_credentials", proxy.randomize_credentials));
381+        if (network != NET_IPV4)
382+            obj.push_back(Pair("proxy-from-default", proxy.proxy == proxyDefault.proxy));

laanwj commented at 3:56 pm on May 6, 2015:

Let’s remove proxy-from-default, it’s not useful information and will always be true anyway according to the above code.

Diapolo commented at 1:32 pm on May 14, 2015:

I’ll remove it :).

Diapolo commented at 1:38 pm on May 14, 2015: none

@laanwj Removed changes to rpcnet.cpp.

Diapolo commented at 1:24 pm on May 31, 2015: none

@laanwj This is passing proxy tests and I removed what you didn’t like. Anything I can do to get this forward finally?

laanwj commented at 7:16 am on June 10, 2015: member

I’m terribly sorry sorry for holding this up so long, but I’m not convinced that the new logic is any better.

The logical flow here sounds to me:

Parse default -proxy, apply to all nets, as well as the name proxy
Parse onion-specific proxy -onion, apply to NET_TOR. This overrides the default set in the first step, if any

All errors during parsing of either setting should be fatal, to avoid that the user is running with different settings than they expect.

This is nearer to the original code than yours. I certainly think the code could be simplified and made more clear, what do you think about:

 0    bool proxyRandomize = GetBoolArg("-proxyrandomize", true);
 1    // -proxy sets a proxy for all outgoing network traffic
 2    // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
 3    std::string proxyArg = GetArg("-proxy", "");
 4    if (proxyArg != "" && proxyArg != "0") {
 5        proxyType addrProxy = proxyType(CService(proxyArg, 9050), proxyRandomize);
 6        if (!addrProxy.IsValid())
 7            return InitError(strprintf(_("Invalid -proxy address: '%s'"), proxyArg));
 8
 9        SetProxy(NET_IPV4, addrProxy);
10        SetProxy(NET_IPV6, addrProxy);
11        SetProxy(NET_TOR, addrProxy);
12        SetNameProxy(addrProxy);
13        SetReachable(NET_TOR); // by default, -proxy sets onion as reachable, unless -noonion later
14    }
15
16    // -onion can override normal proxy for .onion addresses
17    // -noonion (or -onion=0) disables connecting to .onion entirely
18    // An empty string is used to just not override the onion proxy
19    std::string onionArg = GetArg("-onion", "");
20    if (onionArg != "") {
21        if (onionArg == "0") { // Handle -noonion/-onion=0
22            SetReachable(NET_TOR, false); // set onions as unreachable
23        } else {
24            proxyType addrOnion;
25            addrOnion = proxyType(CService(onionArg, 9050), proxyRandomize);
26            if (!addrOnion.IsValid())
27                return InitError(strprintf(_("Invalid -onion address: '%s'"), onionArg));
28            SetProxy(NET_TOR, addrOnion);
29            SetReachable(NET_TOR);
30        }
31    }

This provides a straightforward flow, gets rid of .count() (which is what you needed for the GUI, as it makes it possible to override an earlier provided proxy option to nothing), as well as comments the different cases.

laanwj commented at 7:47 am on June 12, 2015: member

Closing in favor of #6272

laanwj closed this on Jun 12, 2015

Diapolo deleted the branch on Jun 12, 2015

MarcoFalke locked this on Sep 8, 2021

extend core proxy options and handling #4871