Fix Socks5() connect failures to be less noisy and less unnecessarily scary #8033
Conversation
…essarily 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"`
They were too noisy and not necessary for normal operation.
|
utACK 0d9af79 We need another PR to honor logips=0 |
|
Honoring logips=0 is beyond the scope of this PR, various other things need to be fixed too. |
|
ping @theuni |
|
yes plz! :-) |
| case 0x07: return error("Proxy error: protocol error"); | ||
| case 0x08: return error("Proxy error: address type not supported"); | ||
| default: return error("Proxy error: unknown"); | ||
| case 0x01: LogPrintf("Socks5() connect to %s:%d failed: general failure\n", strDest, port); break; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
No strong feelings either way about the errors. Switching the debug stuff to "net" is obviously a good change, though. |
|
This is certainly not perfect, but is am improvement. utACK 0d9af79 |
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
|
94fd1d8 makes the last remaining common non-ERROR more informative and less scary. |
|
utACK 94fd1d8 |
|
Looks good to me, thanks. |
…unnecessarily scary bf9266e Use Socks5ErrorString() to decode error responses from socks proxy. (Warren Togami) 94fd1d8 Make Socks5() InterruptibleRecv() timeout/failures informative. (Warren Togami) 0d9af79 SOCKS5 connecting and connected messages with -debug=net. (Warren Togami) 00678bd Make failures to connect via Socks5() more informative and less unnecessarily scary. (Warren Togami)
…d less unnecessarily scary bf9266e Use Socks5ErrorString() to decode error responses from socks proxy. (Warren Togami) 94fd1d8 Make Socks5() InterruptibleRecv() timeout/failures informative. (Warren Togami) 0d9af79 SOCKS5 connecting and connected messages with -debug=net. (Warren Togami) 00678bd Make failures to connect via Socks5() more informative and less unnecessarily scary. (Warren Togami)
d46b369 [net] Avoid initialization to a value that is never read Prior to this commit the value stored to `s` at initialization was never read (in the case of STRERROR_R_CHAR_P). (furszy) f62d152 Revert "Use async name resolving to improve net thread responsiveness" (furszy) a1b0017 Use Socks5ErrorString() to decode error responses from socks proxy. (furszy) fdec2e3 Make Socks5() InterruptibleRecv() timeout/failures informative. Before: ERROR: Error reading proxy response (furszy) 2027d66 SOCKS5 connecting and connected messages with -debug=net. They were too noisy and not necessary for normal operation. (furszy) 097322c 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. (furszy) Pull request description: Made a set of back ports to make Socks5 connection failures less noisy and less unnecessarily scary. Plus added a straightforward removal of `getaddrinfo_a` which was never available at configure time. Back ported PRs list: bitcoin#8033 bitcoin#9229 bitcoin#9539 ACKs for top commit: Fuzzbawls: utACK d46b369 random-zebra: utACK d46b369 and merging... Tree-SHA512: 5b79309a2bc9ffca686e00b431d8205c72c353a56693b7f1e7a0a7245b158032e9243a49fea10a606cb2329726b3908fd5187ac6c8e57ddd0e9025a9811950ec
Make failures to connect via Socks5() more informative and less unnecessarily scary.
Before:
2016-05-09 00:15:00 ERROR: Proxy error: host unreachableAfter:
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.