extend core proxy options and handling

Diapolo commented at 8:45 am on April 26, 2013: none

rework the proxy handling in init to cover more cases and work more thoroughly
add -proxy6 to allow setting a separate SOCKS5 proxy to reach peers via IPv6
rework proxy data-structures to allow recognition of the default proxy (-proxy) to give users the ability to see, which proxy (IPv6 / Tor) is derived from the default proxy and which was explicitly set
extend RPC call getnetwork info

2nd commit:

remove nameproxy, as SOCKS5 proxies support name resolution

Most proxy-setup is now done using the new ProxyInit() function.

bool ProxyInit(Network net, const std::string& strArg, bool fIsDefault) parameter description: net = network to setup proxy for (NET_IPV4, NET_IPV6 or NET_TOR) strArg = command-line argument to get values from (-proxy, -proxy6 or -onion) fIsDefault = is that proxy the default proxy (true) or a separate proxy (false)?

bool ProxyInit(Network net, const std::string& strArg, bool fIsDefault) does the following: -pre-check, if net is not limited and -no{proxy/proxy6/onion} was NOT specified –pre-check passed: try to SetProxy() and return false on error —pre-check passed: only for net == NET_TOR call SetReachable(); —pre-check passed: return true –pre-check failed: for default proxy (fIsDefault == true) a failed pre-check is okay, return true, otherwise false

default proxy = -proxy separate IPv6 proxy = -proxy6 separate Tor proxy = -onion

Proxy initialisation flow (happens via ProxyInit(), just the name proxy is special cased in the code): -try to setup default IPv4 proxy -try to setup separate Tor proxy, on failure try to setup Tor proxy via default proxy -try to setup separate IPv6 proxy, on failure try to setup IPv6 proxy via default proxy -try to setup default SOCKS5 name proxy

Errors initialising default proxy or IPv6/Tor proxies via default proxy lead to exit!

in src/init.cpp: in 2d329f9526 outdated

426@@ -426,6 +427,31 @@ void ThreadImport(std::vector<boost::filesystem::path> vImportFiles)
427     }
428 }
429 
430+// check proxies and (if they are valid) set them to be used by the client
431+bool ProxyInit(Network net, const std::string& strArg, int nSocksVersion, bool fIsBase)
432+{
433+    // if network is not limited and -no{proxy/proxy6/tor} was not specified
434+    if (!IsLimited(net) && mapArgs.count(strArg.c_str()) && !(mapArgs[strArg.c_str()] == "0")) {

sipa commented at 12:04 pm on April 26, 2013:

Why convert strArg to a c_str() here?

Diapolo commented at 3:49 pm on April 26, 2013:

Perhaps because I didn’t know I could use strArg here ^^. Will update of course.

Diapolo commented at 3:54 pm on April 26, 2013: none

Removed some .c_str() which were unneeded.

Diapolo commented at 5:48 pm on April 26, 2013: none

I left out changes needed in optionsmodel.cpp, that’s why build fails…

Diapolo commented at 5:56 pm on April 26, 2013: none

updated optionsmodel.cpp

in src/init.cpp: in 35c68a3482 outdated

426@@ -426,6 +427,31 @@ void ThreadImport(std::vector<boost::filesystem::path> vImportFiles)
427     }
428 }
429 
430+// check proxies and (if they are valid) set them to be used by the client
431+bool ProxyInit(Network net, const std::string& strArg, int nSocksVersion, bool fIsBase)
432+{
433+    // if network is not limited and -no{proxy/proxy6/tor} was not specified
434+    if (!IsLimited(net) && mapArgs.count(strArg) && !(mapArgs[strArg] == "0")) {

sipa commented at 11:55 pm on April 26, 2013:

Use “GetArg(strArg)” instead of “!(mapArgs[strArg] == “0”)”. Shorter and more flexible.

Diapolo commented at 3:35 pm on April 27, 2013:

@sipa After looking at GetArg(), couldn’t this all be replaced with: if (!IsLimited(net) && GetArg(strArg, 0)) {

sipa commented at 4:30 pm on April 27, 2013:

Indeed. I assumed the 0 was default.

Diapolo commented at 5:04 pm on April 27, 2013: none

Use GetArg(strArg, 0) instead of mapArgs.count(strArg) && !(mapArgs[strArg] == "0"), thanks sipa :).

Diapolo commented at 3:55 pm on April 28, 2013: none

Update name proxy setup to use GetArg("-proxy", 0).

Diapolo commented at 10:06 am on May 4, 2013: none

@sipa Further thoughts about this?

in src/init.cpp: in 2bcb7500a2 outdated

434@@ -434,6 +435,31 @@ void ThreadImport(std::vector<boost::filesystem::path> vImportFiles)
435     }
436 }
437 
438+// check proxies and (if they are valid) set them to be used by the client
439+bool ProxyInit(Network net, const std::string& strArg, int nSocksVersion, bool fIsBase)
440+{
441+    // if network is not limited and -no{proxy/proxy6/tor} was NOT specified
442+    if (!IsLimited(net) && GetArg(strArg, 0)) {

Diapolo commented at 11:43 am on May 9, 2013:

@sipa I need some input on GetArg(strArg, 0), which I added here because you suggested that earlier. I want to catch -noproxy or -proxy=0 here, but GetArg(strArg, 0) does more.

-proxy=127.0.0.1 for example will also cause this to fail, because we use this GetArg()-version:

What hapens now is that atoi64(mapArgs[strArg]); will not work for “127.0.0.1” right ;). Too bad I didn’t catch that earlier, as I only had the state -noproxy and -proxy=0 in mind, which indeed work here like they should.

Edit: I guess this should be (GetArg(strArg, "0") != "0"), right? -noproxy gets converted to proxy=0, which will return “0” here and every other proxy string will get into the if-clause then.

Diapolo commented at 10:39 am on May 11, 2013:

Pull Updated!

in src/netbase.h: in 8cd9884f16 outdated

130@@ -131,14 +131,20 @@ class CService : public CNetAddr
131             )
132 };
133 
134-typedef std::pair<CService, int> proxyType;
135+typedef struct proxyType {
136+    CService addrProxy;
137+    int nSocksVersion;
138+    bool fIsBase;

sipa commented at 4:02 pm on June 15, 2013:

This field seems never used. Why change/split this data structure?

Diapolo commented at 3:54 pm on June 16, 2013:

It’s intended to be used in getinfo and the GUI to know wether Tor or IPv6 proxies are “inherited” by -proxy or if they were specified via -proxy6 or -tor. I didn’t include these changes here as I didn’t want to waste my time for things that perhaps don’t get merged anyway.

laanwj commented at 2:26 pm on June 20, 2013:

The most important scenario to test with any proxy code change is tor, I suspect 99 percent of the users of proxy functionality use it for that. Please make sure that stays working securely.

I’d love to hear some of the use cases for all the exotic proxy options, I’m a bit in doubt whether all that complexity belongs in bitcoin instead of some network tunneling tool or the socks server, and though I’m not against merging this, I’m a bit afraid of forests of never-used and never-tested combinations.

gavinandresen commented at 4:21 pm on June 20, 2013: contributor

Needs a test plan to test all the combinations, then needs testers to, you know, actually test them all.

Diapolo commented at 6:03 am on June 21, 2013: none

@gavinandresen I’m fine with a test plan, as long as this has a real chance of getting merged. Is the code okay now or is ths not wanted anyway?

Diapolo commented at 6:43 am on June 26, 2013: none

Updated to include changes to getinfo. I have not yet had the time to work out a test-plan, but everyone, who helps testing is welcome anyway :).

Diapolo commented at 11:30 pm on August 17, 2013: none

Anyone willing to help me doing that test-plan stuff and/or check out if it works like it should :)?

gmaxwell commented at 1:51 am on August 19, 2013: contributor

I am the last guy who should be checking GUI changes, but I will gladly follow a test plan and report back on it. (Though I can only easily test Linux, and windows builds under wine).

Diapolo commented at 5:33 am on August 19, 2013: none

@gmaxwell I’m glad you are interested in helping me with this one. You currently don’t need to start Bitcoin-Qt to do so, as this can be tested completely via bitcoind. The only Bitcoin-Qt change is to make it compatible with a changed datastructure.

Should I start by listing possible combinations of all proxy switches and how they should behave?

Diapolo commented at 9:21 am on August 26, 2013: none

I’m going to describe the pull with some more details here.

Most proxy-setup is now done using the new ProxyInit() function.

ProxyInit(Network net, const std::string& strArg, int nSocksVersion, bool fIsBase) parameter description: net = network to setup proxy for (NET_IPV4, NET_IPV6 or NET_TOR) strArg = command-line argument to get values from (-proxy, -proxy6 or -tor) nSocksVersion = SOCKS version of the proxy fIsBase = is that proxy a base (true) or separate proxy (false)?

ProxyInit(Network net, const std::string& strArg, int nSocksVersion, bool fIsBase) does the following: -pre-check, if net is not limited and -no{proxy/proxy6/tor} was NOT specified –pre-check passed: try to SetProxy() and return false on error —pre-check passed: only for net == NET_TOR call SetReachable(); —pre-check passed: return true –pre-check failed: for base proxy (fIsBase == true) a failed pre-check is okay, return true, otherwise false

Diapolo commented at 12:47 pm on August 26, 2013: none

base proxy = -proxy separata IPv6 proxy = -proxy6 separate Tor proxy = -tor

Proxy initialisation flow (happens via ProxyInit(), just name proxy is special cased in the code): -try to setup base IPv4 proxy –if SOCKS4: —try to setup separate Tor proxy, on failure disable Tor via SetLimited() (SOCKS4 = no Tor support) —try to setup separate IPv6 proxy, on failure disable IPv6 via SetLimited() (SOCKS4 = no IPv6 support) –if SOCKS5 —try to setup separate Tor proxy, on failure try to setup Tor proxy via base proxy —try to setup separate IPv6 proxy, on failure try to setup IPv6 proxy via base proxy —try to setup base SOCKS5 name proxy

Errors initialising base proxy or Tor/IPv6 proxies via base proxy lead to exit!

Diapolo commented at 12:50 pm on August 26, 2013: none

@gmaxwell Perhaps you could take a look at what I’ve written, if the current proxy handling of this pull sounds correct, before I start doing the test-plan.

Diapolo commented at 12:52 pm on October 6, 2013: none

Updated: Included suggestions from @sipa and introduced getproxyinfo and made ProxyInit() return fBase.

gavinandresen commented at 1:40 am on October 21, 2013: contributor

Still needs a test plan.

Diapolo commented at 2:17 am on November 16, 2013: none

Updated:

greatly improved getproxyinfo call
rename fIsBase to fIsDefault and call -proxy the default proxy instead of the base proxy
add some missing #ifdef USE_IPV6 guards in the code

Diapolo commented at 11:25 am on June 11, 2014: none

Damn Error: Error: Disk space is low!…

I hope if we chose to remove SOCKS4 support this pull is much less complicated to test ;).

laanwj commented at 9:24 am on June 25, 2014: member

I’m tending toward NACK, as it over-complicates things and introduces options that are difficult to test.

No one is asking to use a special proxy server for IPv6 connections. To be honest I think it is overkill and IPv4/IPv6 can be lumped together proxy-wise.
There should be no changes to getinfo. It only exists or backwards compatibility: don’t remove proxy.
getproxyinfo looks like overkill. I’d rather have more condensed and useful information (for example, a sub-structure) integrated into getnetworkinfo.

Diapolo commented at 9:21 am on June 26, 2014: none

@laanwj I removed the changes to getinfo as we decided to not touch this anymore and deprecate it. I’m also fine with not adding getproxyinfo but add it to getnetworkinfo.

So last point is -proxy6, which can be discussed and which I think is even not the main point of this pull. Main point is to have a better and more clear proxy init, which this achieves IMHO. Perhaps I just remove -proxy6 for now, rework the RPC call and you take another look?

Diapolo commented at 12:28 pm on June 27, 2014: none

Removed getproxyinfo and extend getnetworkinfo instead.

in src/rpcnet.cpp: in 3d3643ceed outdated

386+    obj.push_back(Pair("version",                   (int)CLIENT_VERSION));
387+    obj.push_back(Pair("protocolversion",           (int)PROTOCOL_VERSION));
388+    obj.push_back(Pair("localservices",             strprintf("%016x", nLocalServices)));
389+    obj.push_back(Pair("timeoffset",                GetTimeOffset()));
390+    obj.push_back(Pair("connections",               (int)vNodes.size()));
391+    obj.push_back(Pair("proxy-default",             proxyIpv4.addrProxy.IsValid() ? proxyIpv4.addrProxy.ToStringIPPort() : string()));

laanwj commented at 6:50 am on July 3, 2014:

This would be better as a sub-object ‘proxy’ that contains the “ipv4”, “ipv6” and “onion” fields.

in src/rpcnet.cpp: in 3d3643ceed outdated

388+    obj.push_back(Pair("localservices",             strprintf("%016x", nLocalServices)));
389+    obj.push_back(Pair("timeoffset",                GetTimeOffset()));
390+    obj.push_back(Pair("connections",               (int)vNodes.size()));
391+    obj.push_back(Pair("proxy-default",             proxyIpv4.addrProxy.IsValid() ? proxyIpv4.addrProxy.ToStringIPPort() : string()));
392+    obj.push_back(Pair("proxy-ipv6",                proxyIpv6.addrProxy.IsValid() ? proxyIpv6.addrProxy.ToStringIPPort() : string()));
393+    obj.push_back(Pair("proxy-ipv6-from-default",   proxyIpv6.fIsDefault));

laanwj commented at 6:51 am on July 3, 2014:

I don’t think it is useful to report the from-default here.

Diapolo commented at 1:36 pm on July 17, 2014: none

Updated to include the changes after the SOCKS4 support removal and made proxy a separate object in getnetworkinfo.

Diapolo commented at 6:30 pm on July 19, 2014: none

@laanwj Can you take another look please? I also removed nameproxy, can you check the second commit.

in src/netbase.h: in e8bbf699f7 outdated

160@@ -161,15 +161,17 @@ class CService : public CNetAddr
161             )
162 };
163 
164-typedef CService proxyType;
165+typedef struct proxyType {
166+    CService addrProxy;
167+    bool fIsDefault;

laanwj commented at 2:20 pm on July 24, 2014:

I think we don’t need to store this flag? Only a few weeks ago (in 0127a9be) we changed ProxyType to be just a CService. I like to keep it that way if possible.

Diapolo commented at 4:01 pm on July 25, 2014:

I’m going to need this for GUI changes later on to visually tell a user, if the default proxy is used for a certain net, I would be glad to keep this. We can remove or improve it later again, if you see the GUI changes and have a better suggestion?

laanwj commented at 5:30 am on July 26, 2014:

Let me try to explain it differently. I want to move forward to an architecture in which details of how the options are configured are redundant after initialization. For the core to run it only needs configuration values. Speaking on a high level a list of (option, value) tuples goes in, and you get back a running node.

Part of this is separating out the option processing from Init(). This function could also return the data that you need for the UI.

But adding a fIsDefault on the core proxyType feels like a kludge.

Diapolo commented at 12:31 pm on July 28, 2014:

The third commit removes fIsDefault, like you suggested. Also this isn’t needed anymore for the GUI.

in src/init.cpp: in e8bbf699f7 outdated

855-    }
856+    // check for presence of default proxy to reach peers via IPv4
857+    if (!ProxyInit(NET_IPV4, "-proxy", true))
858+        return false; // errors with default proxy lead to exit
859+    // enable outgoing IPv6/Tor connections for default proxy (if no separate SOCKS5 proxy for IPv6/Tor will be used)
860+    if (!ProxyInit(NET_IPV6, "-proxy6", false))

laanwj commented at 2:35 pm on July 24, 2014:

An invalid -proxy6 or -onion address should also lead to exit, IMO.

Diapolo commented at 3:52 pm on July 25, 2014:

@laanwj The idea and logic here is that -proxy could work, so I don’t want to exit when -proxy6 or -tor fail to set a proxy.

As I wrote in the init description:

in src/rpcnet.cpp: in e8bbf699f7 outdated

421         {
422             Object rec;
423-            rec.push_back(Pair("address", item.first.ToString()));
424-            rec.push_back(Pair("port", item.second.nPort));
425-            rec.push_back(Pair("score", item.second.nScore));
426+            rec.push_back(Pair("address",   item.first.ToString()));

laanwj commented at 2:42 pm on July 24, 2014:

Don’t do these whitespace changes in this function, they will get undo’ed by @sipa’s reformatter anyway.

Diapolo commented at 3:49 pm on July 25, 2014:

Right, I’m going to revert these and when making changes and they are alligned, I’ll remove the alligning whitespaces.

laanwj commented at 2:42 pm on July 24, 2014: member

All in all this looks fine to me now, apart from above-mentioned remarks.

in src/net.cpp: in caea6b3986 outdated

1232@@ -1233,7 +1233,7 @@ void ThreadDNSAddressSeed()
1233     LogPrintf("Loading addresses from DNS seeds (could take a while)\n");
1234 
1235     BOOST_FOREACH(const CDNSSeedData &seed, vSeeds) {
1236-        if (HaveNameProxy()) {
1237+        if (HaveProxy(NET_IPV4)) {

Diapolo commented at 4:02 pm on July 25, 2014:

@laanwj Can it be a problem if someone is using -onlynet=IPv6 for example and we use this? Can you think of a better way to select the right proxy used for name resolution?

laanwj commented at 5:19 am on July 26, 2014:

There is no better way. With a name you don’t know if it is IPv4 or IPv6. Ofcourse you can know if it is onion/tor if it ends in .onion.

But as I said before there is no good reason for allowing different proxies for IPv4 and IPv6. This is not possible right now and I do not intend to add this functionality.

Having a ‘clearnet proxy’ (-proxy) and a ‘onion net proxy’ (-onion) is enough. This makes this code valid: we just define NET_IPV4 to be the ‘clearnet proxy’. Internally the proxy for NET_IPV6 should always be the same as that for NET_IPV4.

Diapolo commented at 12:33 pm on July 28, 2014:

Right, I’m going to remove -proxy6, how you described it makes sense! @laanwj 4th commit reverts -proxy6 :).

laanwj commented at 10:52 am on July 30, 2014:

We still have a potential problem here. If NET_IPV4 is set as limited, the HaveProxy(NET_IPV4) will not be set due to the condition in ProxyInit. So my idea mentioned above to always use IPV4 proxy falls apart.

May be better to bring back the query for NameProxy but not the setter counterparts. Just pick the first proxy of IPV4/IPV6 that is actually set.

See here for my take on it: https://github.com/laanwj/bitcoin/commit/def586ab569ef62e5e1af8ca17817a903eb39dad

cozz commented at 2:51 am on July 26, 2014: contributor

Just an idea: Only have -proxy, but allow it multiple times: -proxy=<ip:port[:net]> net=IPv4, IPv6 or onion. Not specifying net means IPv4 and IPv6 (default).

laanwj commented at 5:13 am on July 26, 2014: member

@cozz That’s not more clear to me than the current scheme IMO. Let’s not break backwards compatibility for no good reason.

extend core proxy options and handling

- rework the proxy handling in init to cover more cases and work more
  thoroughly
- add -proxy6 to allow setting a separate SOCKS5 proxy to reach peers via
  IPv6
- rework proxy data-structures to allow recognition of the default proxy
  (-proxy) to give users the ability to see, which proxy (IPv6 / Tor) is
  derived from the default proxy and which was explicitly set
- extend RPC call getnetwork info
- remove unneeded (int) casts before CLIENT_VERSION and PROTOCOL_VERSION
  in getinfo and getnetworkinfo RPC calls

6866dad580

remove nameproxy, as SOCKS5 proxies support name resolution 1cf91e264a

revert: add fIsDefault (squashme) 31a5646891

revert: add -proxy6 (squashme) bb977eaccc

BitcoinPullTester commented at 1:25 pm on July 28, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p2575_bb977eaccc5daeda5a05139f916fc4d00fe9f5ef/ 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 7:55 am on July 30, 2014: none

@laanwj If you can re-check the latest changes, I’m able to squash this into 2 commits and will rebase the GUI pull onto this.

laanwj referenced this in commit 825c2794e4 on Jul 30, 2014

laanwj referenced this in commit b1a025f524 on Jul 30, 2014

laanwj referenced this in commit e0447a7b2c on Jul 30, 2014

laanwj commented at 1:43 pm on July 30, 2014: member

See #4605 for my take on the RPC changes. I’ve added some more information per network.

laanwj added the label P2P on Jul 31, 2014

Diapolo commented at 8:55 am on August 4, 2014: none

@laanwj Did you close your nameproxy pull?

laanwj commented at 9:24 am on August 4, 2014: member

I never had it as pull, because I’m not sure how I want to handle that yet. I may submit it as pull as some point.

laanwj referenced this in commit 0590a54818 on Aug 4, 2014

Diapolo commented at 8:28 am on August 7, 2014: none

@laanwj If I remove the nameproxy changes are you fine with this then?

laanwj commented at 6:06 pm on August 12, 2014: member

@Diapolo Also the RPC changes can be removed (in favor of #4605) .So to just keep the initialization changes? I think that would be a good unit to split off.

Diapolo commented at 1:26 pm on August 14, 2014: none

@laanwj I’ll rework this when I’m back in a few days.

laanwj referenced this in commit 2649e2bfb2 on Aug 18, 2014

laanwj referenced this in commit aa8279513b on Aug 18, 2014

laanwj commented at 8:03 am on September 1, 2014: member

Closing this one for now. This thread has gotten too ‘messy’. If you intend to do a reworked version at some point that just changes initialization, I’d suggest opening a new pull.

laanwj closed this on Sep 1, 2014

wtogami referenced this in commit afb06c9de2 on Sep 10, 2014

wtogami referenced this in commit 6c8900b914 on Sep 19, 2014

wtogami referenced this in commit ab0ef75662 on Oct 2, 2014

wtogami referenced this in commit b35d4d2dfa on Nov 14, 2014

wtogami referenced this in commit 1d4ba9fe6d on Dec 23, 2014

reddink referenced this in commit ccae725132 on May 27, 2020

DrahtBot locked this on Sep 8, 2021

extend core proxy options and handling #2575