Fix Socks5() connect failures to be less noisy and less unnecessarily scary #8033

wtogami commented at 1:18 AM on May 10, 2016: contributor

Make failures to connect via Socks5() more informative and less unnecessarily scary.

The "ERROR" was printed far too often during normal operation for what was not an error.
Makes the Socks5() connect failure similar to the IP connect failure in debug.log.

Before: 2016-05-09 00:15:00 ERROR: Proxy error: host unreachable

After: 2016-05-09 00:15:00 Socks5() connect to t6xj6wilh4ytvcs7.onion:18333 failed: host unreachable"

SOCKS5 connecting and connected messages with -debug=net.

They were too noisy and not necessary for normal operation.

Make failures to connect via Socks5() more informative and less unnecessarily scary.

* The "ERROR" was printed far too often during normal operation for what was not an error.
* Makes the Socks5() connect failure similar to the IP connect failure in debug.log.

Before:
`2016-05-09 00:15:00 ERROR: Proxy error: host unreachable`

After:
`2016-05-09 00:15:00 Socks5() connect to t6xj6wilh4ytvcs7.onion:18333 failed: host unreachable"`

00678bdb0a

SOCKS5 connecting and connected messages with -debug=net.

They were too noisy and not necessary for normal operation.

0d9af79e50

pstratem commented at 1:55 AM on May 10, 2016: contributor

utACK 0d9af79e5084dc2fb6a73abd9dd113dda4e993b4

We need another PR to honor logips=0

wtogami commented at 1:58 AM on May 10, 2016: contributor

Honoring logips=0 is beyond the scope of this PR, various other things need to be fixed too.

wtogami renamed this:
~~Fix Socks5() connect failures to be less noisy and unnecessarily scary~~
Fix Socks5() connect failures to be less noisy and less unnecessarily scary
on May 10, 2016

laanwj commented at 7:42 AM on May 10, 2016: member

ping @theuni

laanwj added the label P2P on May 10, 2016

midnightmagic commented at 10:01 PM on May 10, 2016: contributor

yes plz! :-)

in src/netbase.cpp:None in 0d9af79e50 outdated

 390 | -            case 0x05: return error("Proxy error: connection refused");
 391 | -            case 0x06: return error("Proxy error: TTL expired");
 392 | -            case 0x07: return error("Proxy error: protocol error");
 393 | -            case 0x08: return error("Proxy error: address type not supported");
 394 | -            default:   return error("Proxy error: unknown");
 395 | +            case 0x01: LogPrintf("Socks5() connect to %s:%d failed: general failure\n", strDest, port); break;

laanwj commented at 9:44 AM on May 12, 2016:

I'd prefer assigning the error to a string here (or make a string Socks5ErrorString(code) function), instead of repeating the Socks5() connect to %s:%d failed on every line.

wtogami commented at 2:21 AM on May 13, 2016:

@phantomcircuit informs me that other non-error cases here print scary "ERROR:" messages outside of if (pchRet2[1] != 0x00), so it is not straight forward to make a function that decodes only the Socks5ErrorString(). I originally thought about adding a function similar to error() but then I was told all of this needs to be rewritten for the network rewrite so don't worry about it for now.

theuni commented at 7:15 PM on May 15, 2016:

Yes, this has been rewritten anyway. Adding to what @laanwj said, it'd be useful to have the code in the return so that the caller could react appropriately (for ex "address type not supported" should be a hint not to try that type again). In the refactored code, this returns a general type error (connection failed/proxy failed/etc), as well as a specific failure type.

laanwj commented at 9:07 AM on May 18, 2016:

It's not like I'm asking for much, you can literally take this over:

std::string Socks5ErorString(int err)
{
    switch(err) {
        case 0x01: return "general failure";
        case 0x02: return "connection not allowed";
        case 0x03: return "network unreachable";
        case 0x04: return "host unreachable";
        case 0x05: return "connection refused";
        case 0x06: return "TTL expired";
        case 0x07: return "protocol error";
        case 0x08: return "address type not supported";
        default:   return "unknown";
    }
}

Then replace the switch-and-LogPrintf here with:

LogPrintf("Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1]));

Leave the rest the same. It shouldn't change behavior at all from what you're doing now but is cleaner.

theuni commented at 7:15 PM on May 15, 2016: member

No strong feelings either way about the errors. Switching the debug stuff to "net" is obviously a good change, though.

pstratem commented at 5:33 AM on May 17, 2016: contributor

This is certainly not perfect, but is am improvement.

utACK 0d9af79e5084dc2fb6a73abd9dd113dda4e993b4

Make Socks5() InterruptibleRecv() timeout/failures informative.

Before:
2016-05-16 06:10:45 ERROR: Error reading proxy response

After:
2016-05-16 06:10:45 Socks5() connect to k7s5d6jqig4ej4v4.onion:18333 failed: InterruptibleRecv() timeout or other failure

94fd1d8d53

wtogami commented at 7:47 AM on May 17, 2016: contributor

94fd1d8 makes the last remaining common non-ERROR more informative and less scary.

MarcoFalke commented at 8:04 AM on May 17, 2016: member

utACK 94fd1d8

Use Socks5ErrorString() to decode error responses from socks proxy. bf9266e017

wtogami commented at 6:31 AM on May 19, 2016: contributor

Followed @laanwj suggestion to add Socks5ErrorString() to decode error responses from socks proxy.

Note the separate LogPrintf in 94fd1d8d53adceb80e5a41cc6438c7704aeac0f7 is outside of what Socks5ErrorString() can decode.

Is this sufficient, do you want a squash?

laanwj commented at 6:44 AM on May 19, 2016: member

Looks good to me, thanks. Yes the case in https://github.com/bitcoin/bitcoin/commit/94fd1d8d53adceb80e5a41cc6438c7704aeac0f7 is separate, that doesn't matter, it's not an error returned from the proxy server but a local network error (so it wouldn't belong in Socks5ErrorString).

laanwj merged this on May 19, 2016

laanwj closed this on May 19, 2016

laanwj referenced this in commit 18436d8896 on May 19, 2016

codablock referenced this in commit 4ae0037ca5 on Sep 16, 2017

codablock referenced this in commit d0399c19e9 on Sep 19, 2017

codablock referenced this in commit 1897ccc646 on Dec 21, 2017

random-zebra referenced this in commit d9cbbad1dc on Dec 14, 2020

DrahtBot locked this on Sep 8, 2021