remove SOCKS4 support from core and GUI #4327

Diapolo commented at 11:23 AM on June 11, 2014: none

now we support SOCKS5 only

laanwj commented at 12:10 PM on June 11, 2014: member

...why? I think it makes sense to be able to support different proxy types.

Diapolo commented at 12:45 PM on June 11, 2014: none

SOCKS5 exists since 1996, so SOCKS4 is even older... I remember @sipa expressed he would like to get rid of it some time ago, so I decided to do it. Also most current SOCKS servers should handle SOCKS5 just fine (+ it allows a much cleaner proxy setup in our code).

laanwj commented at 12:49 PM on June 11, 2014: member

True - I can't think of anything that still uses SOCKS4. SOCKS5 is also safer as it allows DNS lookups over the proxy.

laanwj commented at 3:40 PM on June 11, 2014: member

I've posted a question regarding SOCKS4 usage on the development mailing list. If no one responds I take that as no one cares and we can go ahead.

Diapolo commented at 6:30 AM on June 16, 2014: none

@laanwj Should I add a deprecation warning like we have for -debugnet and soon -tor?

laanwj commented at 6:33 AM on June 16, 2014: member

I haven't had any replies to my post, so I assume it's not very important. No one is crying that SOCKS4 is going to go.

Anyhow, how is the error handling? it should at least fail with an error if the user still specifies a SOCKS4 proxy, instead of silently doing anything nefarious like bypass the proxy.

Diapolo commented at 6:40 AM on June 16, 2014: none

Right, that was my question... I'll add a check to catch SOCKS4.

Diapolo commented at 6:49 AM on June 16, 2014: none

Added commit to error out, when -socks is detected.

in src/init.cpp:None in 0e0bff8fc0 outdated

 551 | @@ -553,6 +552,9 @@ bool AppInit2(boost::thread_group& threadGroup)
 552 |      // Check for -debugnet (deprecated)
 553 |      if (GetBoolArg("-debugnet", false))
 554 |          InitWarning(_("Warning: Deprecated argument -debugnet ignored, use -debug=net"));
 555 | +    // Check for -socks (deprecated) - as this is a privacy risk to continue, exit here
 556 | +    if (mapArgs.count("-socks"))
 557 | +        return InitError(_("Error: Deprecated argument -socks found. SOCKS4 isn't' supported anymore, the default is now SOCKS5."));

laanwj commented at 7:16 AM on June 16, 2014:

This message is not correct:

SOCKS4 is not deprecated, that would imply it is still there but will go away later, support is removed completely by this pull
SOCKS5 was always the default, but it is now the only option

Diapolo commented at 7:22 AM on June 16, 2014:

It reads -socks argument is deprecated and we don't have SOCKS4 support anymore. I agree the SOCKS5 sentence can be removed.

What about Error: Deprecated argument -socks found. Setting SOCKS version isn't possible anymore, only SOCKS5 is supported.

laanwj commented at 7:38 AM on June 16, 2014:

But deprecation is never an error, just a warning. It means to warn that something will go away in the future. It doesn't fit in this sentence.

Diapolo commented at 8:22 AM on June 16, 2014:

Then it also doesn't fit for -debugnet, nor -tor... will rephrase.

Diapolo commented at 12:05 PM on July 2, 2014: none

Not sure why this fails, can't find the error. @laanwj Can you take a look at the log: http://jenkins.bluematt.me/pull-tester/p4327_e6887911c08dae8d4e606d9c4ec5f03a1976cf3b/test.log

laanwj commented at 7:07 AM on July 4, 2014: member

Random pulltester failure if you ask me...

remove SOCKS4 support from core and GUI

- now we support SOCKS5 only

0127a9be14

error out, when we detect -socks argument a339a37b28

BitcoinPullTester commented at 7:31 AM on July 7, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4327_a339a37b28176135c7fe5adbd3b50da77a45373d/ 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.

laanwj commented at 8:55 AM on July 7, 2014: member

After this we can assume that any proxy (if set) can accept domain names, so we can get rid of the separate NameProxy and just use whatever proxy is set for IPv4/IPv6. Not necessary in this pull of course.

ghost commented at 9:03 AM on July 7, 2014: none

ACK

laanwj merged this on Jul 7, 2014

laanwj closed this on Jul 7, 2014

laanwj referenced this in commit 4252f2928b on Jul 7, 2014

Diapolo deleted the branch on Jul 7, 2014

wtogami referenced this in commit 0e521e27e6 on Sep 9, 2014

wtogami referenced this in commit 9a6258e292 on Sep 19, 2014

wtogami referenced this in commit a721dadec0 on Sep 19, 2014

wtogami referenced this in commit 26e6bb2dd2 on Oct 2, 2014

wtogami referenced this in commit 0904668c48 on Oct 2, 2014

wtogami referenced this in commit f31ed9d86d on Nov 14, 2014

wtogami referenced this in commit 910e7a6996 on Nov 14, 2014

wtogami referenced this in commit f878aafcf8 on Dec 23, 2014

wtogami referenced this in commit fa412799bd on Dec 23, 2014

MarcoFalke locked this on Sep 8, 2021