net: log connections failures via SOCKS5 with less severity #30064

pull vasild wants to merge 1 commits into bitcoin:master from vasild:consistently_log_connection_failures changing 1 files +3 −2
  1. vasild commented at 2:47 pm on May 8, 2024: contributor

    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

  2. DrahtBot commented at 2:47 pm on May 8, 2024: contributor

    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 mzumsande, tdb3, achow101
    Concept ACK jonatack, BrandonOdiwuor
    Stale ACK laanwj, kristapsk

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

    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.

  3. DrahtBot added the label P2P on May 8, 2024
  4. laanwj approved
  5. laanwj commented at 8:05 am on May 10, 2024: member
    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 7524aaf58f5dbe077d4997b3a5dcf635a027a659
  6. kristapsk approved
  7. kristapsk commented at 8:29 am on May 10, 2024: contributor
    cr utACK 7524aaf58f5dbe077d4997b3a5dcf635a027a659
  8. in src/netbase.cpp:548 in 7524aaf58f outdated
    544@@ -544,9 +545,9 @@ template<typename... Args>
    545 static void LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args) {
    546     std::string error_message = tfm::format(fmt, args...);
    547     if (manual_connection) {
    548-        LogPrintf("%s\n", error_message);
    549+        LogPrintLevel(BCLog::NET, BCLog::Level::Info, "%s\n", error_message);
    


    jonatack commented at 10:31 pm on May 23, 2024:

    In this case, I had added Manual and considered it an error (or perhaps warning), what do you think: https://github.com/bitcoin/bitcoin/pull/25203/commits/a0be62c9043caaebf64b3178b9bea080fa200ec7#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R541

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

    mzumsande commented at 4:55 pm on May 28, 2024:
    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.

    vasild commented at 5:10 am on May 31, 2024:
    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.
  9. jonatack commented at 10:37 pm on May 23, 2024: member
    Concept ACK, a0be62c (#25203) from two years ago contains these changes and further ones, perhaps look if other ones there could be done here.
  10. BrandonOdiwuor commented at 7:25 pm on May 27, 2024: contributor
    Concept ACK
  11. mzumsande commented at 5:01 pm on May 28, 2024: contributor
    Concept ACK
  12. net: log connections failures via SOCKS5 with less severity
    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: https://github.com/bitcoin/bitcoin/issues/29759
    f3cfbd65f5
  13. vasild force-pushed on May 31, 2024
  14. vasild commented at 5:10 am on May 31, 2024: contributor

    7524aaf58f...f3cfbd65f5: drop the changes to LogConnectFailure(). They were somewhat related, but not directly to the SOCKS5 stuff.

    perhaps look if other ones there could be done here

    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.

  15. mzumsande commented at 3:09 pm on May 31, 2024: contributor
    Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
  16. DrahtBot requested review from jonatack on May 31, 2024
  17. DrahtBot requested review from laanwj on May 31, 2024
  18. DrahtBot requested review from kristapsk on May 31, 2024
  19. DrahtBot requested review from BrandonOdiwuor on May 31, 2024
  20. tdb3 approved
  21. tdb3 commented at 8:09 pm on July 20, 2024: contributor
    Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
  22. achow101 commented at 9:23 pm on August 5, 2024: member
    ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
  23. achow101 merged this on Aug 5, 2024
  24. achow101 closed this on Aug 5, 2024

  25. vasild deleted the branch on Aug 12, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me