-
Notifications
You must be signed in to change notification settings - Fork 35.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: log connections failures via SOCKS5 with less severity #30064
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree these don't need to be logged at high severity, this matches our general idea that network problems on the open internet are normal and there's no reason to scream at the user for them.
ACK 7524aaf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cr utACK 7524aaf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, a0be62c
(#25203) from two years ago contains these changes and further ones, perhaps look if other ones there could be done here.
src/netbase.cpp
Outdated
@@ -544,9 +545,9 @@ template<typename... Args> | |||
static void LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args) { | |||
std::string error_message = tfm::format(fmt, args...); | |||
if (manual_connection) { | |||
LogPrintf("%s\n", error_message); | |||
LogPrintLevel(BCLog::NET, BCLog::Level::Info, "%s\n", error_message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I had added Manual
and considered it an error (or perhaps warning), what do you think: a0be62c#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R541
LogPrintLevel(BCLog::NET, BCLog::Level::Info, "%s\n", error_message); | |
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Manual %s\n", error_message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the change in this line should be explained better / maybe be its own commit. LogConnectFailure
is not related to SOCKS5 but also reached through ConnectDirectly()
, so it's not clear to me from the commit msg or PR description why the log is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped this hunk altogether. It was more or less cosmetic, giving a category to a category-less message - for manual connection failures: category=none/severity=info -> category=net/severity=info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
src/netbase.cpp
Outdated
@@ -544,9 +545,9 @@ template<typename... Args> | |||
static void LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args) { | |||
std::string error_message = tfm::format(fmt, args...); | |||
if (manual_connection) { | |||
LogPrintf("%s\n", error_message); | |||
LogPrintLevel(BCLog::NET, BCLog::Level::Info, "%s\n", error_message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the change in this line should be explained better / maybe be its own commit. LogConnectFailure
is not related to SOCKS5 but also reached through ConnectDirectly()
, so it's not clear to me from the commit msg or PR description why the log is changed.
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node). Related: bitcoin#29759
7524aaf
to
f3cfbd6
Compare
The changes from #25203 look sound, but I want to keep this PR small, just to address the noisy and inconsistent logging due to connection failures via the SOCKS5 proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK f3cfbd6
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node).
Related: #29759