extend core proxy options and handling by Diapolo · Pull Request #2575 · bitcoin/bitcoin

Diapolo · 2013-04-26T08:45:01Z

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!

sipa · 2013-04-26T12:04:05Z

src/init.cpp

Why convert strArg to a c_str() here?

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

Diapolo · 2013-04-26T15:54:38Z

Removed some .c_str() which were unneeded.

Diapolo · 2013-04-26T17:48:14Z

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

Diapolo · 2013-04-26T17:56:27Z

updated optionsmodel.cpp

sipa · 2013-04-26T23:55:48Z

src/init.cpp

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

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

Indeed. I assumed the 0 was default.

Diapolo · 2013-04-27T17:04:38Z

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

Diapolo · 2013-04-28T15:55:50Z

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

Diapolo · 2013-05-04T10:06:49Z

@sipa Further thoughts about this?

Diapolo · 2013-05-09T11:43:52Z

src/init.cpp

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

int64 GetArg(const std::string& strArg, int64 nDefault) { if (mapArgs.count(strArg)) return atoi64(mapArgs[strArg]); return nDefault; }

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.

Pull Updated!

gavinandresen · 2013-06-20T16:21:01Z

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

Diapolo · 2013-06-21T06:03:48Z

@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 · 2013-06-26T06:43:32Z

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 · 2013-08-17T23:30:25Z

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

gmaxwell · 2013-08-19T01:51:58Z

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 · 2013-08-19T05:33:43Z

@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 · 2013-08-26T09:21:56Z

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 · 2013-08-26T12:47:55Z

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 · 2013-08-26T12:50:57Z

@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 · 2013-10-06T12:52:10Z

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

gavinandresen · 2013-10-21T01:40:52Z

Still needs a test plan.

Diapolo · 2013-11-16T02:17:13Z

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 · 2014-06-11T11:25:32Z

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 · 2014-06-25T09:24:45Z

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 · 2014-06-26T09:21:49Z

@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 · 2014-06-27T12:28:10Z

Removed getproxyinfo and extend getnetworkinfo instead.

laanwj · 2014-07-03T06:50:52Z

src/rpcnet.cpp

This would be better as a sub-object 'proxy' that contains the "ipv4", "ipv6" and "onion" fields.

Diapolo · 2014-07-17T13:36:36Z

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

- 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

BitcoinPullTester · 2014-07-28T13:25:35Z

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 · 2014-07-30T07:55:16Z

@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 · 2014-07-30T13:43:57Z

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

Diapolo · 2014-08-04T08:55:00Z

@laanwj Did you close your nameproxy pull?

laanwj · 2014-08-04T09:24:40Z

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.

This commit adds per-network information to the getnetworkinfo RPC call: - Is the network limited? - Is the network reachable - Which proxy is used for this network, if any Inspired by bitcoin#2575.

Diapolo · 2014-08-07T08:28:14Z

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

laanwj · 2014-08-12T18:06:59Z

@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 · 2014-08-14T13:26:08Z

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

This commit adds per-network information to the getnetworkinfo RPC call: - Is the network limited? - Is the network reachable - Which proxy is used for this network, if any Inspired by bitcoin#2575.

laanwj · 2014-09-01T08:03:34Z

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.

This commit adds per-network information to the getnetworkinfo RPC call: - Is the network limited? - Is the network reachable - Which proxy is used for this network, if any Inspired by bitcoin#2575. Conflicts: src/init.cpp src/rpcnet.cpp Rebased-from: aa82795 bitcoin#4605

This commit adds per-network information to the getnetworkinfo RPC call: - Is the network limited? - Is the network reachable - Which proxy is used for this network, if any Inspired by bitcoin#2575. (cherry picked from commit aa82795) # Conflicts: # src/rpcnet.cpp

sipa reviewed Apr 26, 2013
View reviewed changes

Diapolo reviewed May 9, 2013
View reviewed changes

laanwj mentioned this pull request Jun 1, 2014

Proxy does not respect onlynet #4266

Closed

laanwj reviewed Jul 3, 2014
View reviewed changes

src/rpcnet.cpp Outdated

Copy link

Member

laanwj Jul 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better as a sub-object 'proxy' that contains the "ipv4", "ipv6" and "onion" fields.

Philip Kaufmann added 4 commits July 28, 2014 14:20

remove nameproxy, as SOCKS5 proxies support name resolution

1cf91e2

revert: add fIsDefault (squashme)

31a5646

revert: add -proxy6 (squashme)

bb977ea

laanwj mentioned this pull request Jul 30, 2014

Add detailed network info to getnetworkinfo RPC #4605

Merged

laanwj added the P2P label Jul 31, 2014

laanwj closed this Sep 1, 2014

Diapolo mentioned this pull request Sep 8, 2014

extend core proxy options and handling #4871

Closed

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

Conversation

Diapolo commented Apr 26, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 26, 2013

Uh oh!

Diapolo commented Apr 26, 2013

Uh oh!

Diapolo commented Apr 26, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 27, 2013

Uh oh!

Diapolo commented Apr 28, 2013

Uh oh!

Diapolo commented May 4, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavinandresen commented Jun 20, 2013

Uh oh!

Diapolo commented Jun 21, 2013

Uh oh!

Diapolo commented Jun 26, 2013

Uh oh!

Diapolo commented Aug 17, 2013

Uh oh!

gmaxwell commented Aug 19, 2013

Uh oh!

Diapolo commented Aug 19, 2013

Uh oh!

Diapolo commented Aug 26, 2013

Uh oh!

Diapolo commented Aug 26, 2013

Uh oh!

Diapolo commented Aug 26, 2013

Uh oh!

Diapolo commented Oct 6, 2013

Uh oh!

gavinandresen commented Oct 21, 2013

Uh oh!

Diapolo commented Nov 16, 2013

Uh oh!

Diapolo commented Jun 11, 2014

Uh oh!

laanwj commented Jun 25, 2014

Uh oh!

Diapolo commented Jun 26, 2014

Uh oh!

Diapolo commented Jun 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jul 17, 2014

Uh oh!

BitcoinPullTester commented Jul 28, 2014

Uh oh!

Diapolo commented Jul 30, 2014

Uh oh!

laanwj commented Jul 30, 2014

Uh oh!

Diapolo commented Aug 4, 2014

Uh oh!

laanwj commented Aug 4, 2014

Uh oh!

Diapolo commented Aug 7, 2014

Uh oh!

laanwj commented Aug 12, 2014

Uh oh!

Diapolo commented Aug 14, 2014