Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vasild
Copy link
Contributor

@vasild vasild commented May 8, 2024

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

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
@DrahtBot
Copy link
Contributor

DrahtBot commented May 8, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK laanwj, kristapsk
Concept ACK jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

Copy link
Member

@laanwj laanwj left a 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

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

cr utACK 7524aaf

Copy link
Contributor

@jonatack jonatack left a 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) contains these changes and further ones, perhaps look if other ones there could be done here.

@@ -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);
Copy link
Contributor

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

Suggested change
LogPrintLevel(BCLog::NET, BCLog::Level::Info, "%s\n", error_message);
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Manual %s\n", error_message);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants