net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers #20845

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2101-netLogDisconnect changing 1 files +2 −1
  1. MarcoFalke commented at 2:43 pm on January 4, 2021: member
    The goal is to avoid local peers (e.g. untrusted peers on the local network or inbound onion peers via a local onion proxy) filling the debug log (and thus the disk).
  2. MarcoFalke force-pushed on Jan 4, 2021
  3. in src/net_processing.cpp:3789 in fae5c7c92b outdated 
    3785@@ -3786,7 +3786,7 @@ bool PeerManager::MaybeDiscourageAndDisconnect(CNode& pnode)
3786     if (pnode.addr.IsLocal()) {
3787         // We disconnect local peers for bad behavior but don't discourage (since that would discourage
3788         // all peers on the same local address)
3789-        LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
3790+        LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
    vasild commented at 3:38 pm on January 4, 2021:

    We can use pnode.ConnectedThroughNetwork() == NET_ONION to check for an onion peer.

    Actually, if the tor daemon is running on another machine or running on the same machine but bitcoind<->tord communication is happening via some of the external (real) addresses, then incoming tor connections will not have pnode.addr equal to 127.0.0.1 and thus pnode.addr.IsLocal() will be false.

    MarcoFalke commented at 3:48 pm on January 4, 2021:

    thus pnode.addr.IsLocal() will be false.

    That seems like something that can be improved unrelated to a logging-only change?

    We can use pnode.ConnectedThroughNetwork() == NET_ONION to check for an onion peer.

    Onion peers are just an example of peers that may use the local proxy. I haven’t tested, but it should be possible to use a proxy for ipv4 and ipv6 as well.

    vasild commented at 8:57 am on January 5, 2021:

    That seems like something that can be improved unrelated to a logging-only change?

    Yes.

    Onion peers are just an example of peers that may use the local proxy.

    You mean incoming peers to use a local proxy? I guess that would be possible, though maybe unusual.

    MarcoFalke commented at 12:49 pm on February 16, 2021:
    Added a check for pnode.m_inbound_onion for local onion proxies. I’ll leave non-local onion proxies for a follow-up.
  4. MarcoFalke added the label P2P on Jan 4, 2021
  5. MarcoFalke added the label Utils/log/libs on Jan 4, 2021
  6. practicalswift commented at 5:36 pm on January 9, 2021: contributor
    ACK fae5c7c92b012bb3ce6fe6d1edee4a2448fdf646
  7. DrahtBot commented at 11:45 am on January 13, 2021: member

    🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

  8. jonatack commented at 7:49 pm on January 14, 2021: member
    What I’d love to see is for #20576 to be implemented.
  9. vasild approved
  10. vasild commented at 10:26 am on January 15, 2021: member

    ACK fae5c7c92b012bb3ce6fe6d1edee4a2448fdf646

    As a further improvement maybe the following would make sense:

    Achieve two things:

    • Recognize incoming onion peers even if the tor proxy is not running on 127.0.0.1
    • Distinguish onion from normal local peers in the logging, if the tor proxy is running on 127.0.0.1
     0diff --git i/src/net_processing.cpp w/src/net_processing.cpp
 1index 413e3c034..ecd615995 100644
 2--- i/src/net_processing.cpp
 3+++ w/src/net_processing.cpp
 4@@ -3780,16 +3780,20 @@ bool PeerManager::MaybeDiscourageAndDisconnect(CNode& pnode)
 5     if (pnode.IsManualConn()) {
 6         // We never disconnect or discourage manual peers for bad behavior
 7         LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id);
 8         return false;
 9     }
10 
11-    if (pnode.addr.IsLocal()) {
12-        // We disconnect local peers for bad behavior but don't discourage (since that would discourage
13-        // all peers on the same local address)
14-        LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
15+    if (pnode.addr.IsLocal() || pnode.IsInboundOnion()) {
16+        // We disconnect local or onion peers for bad behavior but don't discourage
17+        // (since that would discourage all peers on the same address)
18+        if (pnode.IsInboundOnion()) {
19+            LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging onion peer %d!\n", peer_id);
20+        } else {
21+            LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
22+        }
23         pnode.fDisconnect = true;
24         return true;
25     }
26 
27     // Normal case: Disconnect the peer and discourage all nodes sharing the address
28     LogPrint(BCLog::NET, "Disconnecting and discouraging peer %d!\n", peer_id);
  11. practicalswift commented at 10:35 am on January 15, 2021: contributor

    What I’d love to see is for #20576 to be implemented.

    This PR and #20576 are not mutually exclusive AFAICT: I think both are worth doing :)

  12. luke-jr referenced this in commit d4430a7d56 on Jan 28, 2021
  13. DrahtBot commented at 2:53 pm on February 12, 2021: member

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

    Conflicts

    No conflicts as of last run.

  14. MarcoFalke renamed this:
    net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers
    net: Avoid discouraging the onion proxy when one inbound onion misbehaves
    on Feb 15, 2021
  15. MarcoFalke removed the label Utils/log/libs on Feb 15, 2021
  16. MarcoFalke renamed this:
    net: Avoid discouraging the onion proxy when one inbound onion misbehaves
    net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers
    on Feb 15, 2021
  17. MarcoFalke added the label Utils/log/libs on Feb 15, 2021
  18. DrahtBot added the label Needs rebase on Feb 16, 2021
  19. net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers fa55159b9e
  20. MarcoFalke force-pushed on Feb 16, 2021
  21. MarcoFalke commented at 12:52 pm on February 16, 2021: member
    Rebased and added a check for pnode.m_inbound_onion for local onion proxies. I’ll leave non-local onion proxies for a follow-up.
  22. DrahtBot removed the label Needs rebase on Feb 16, 2021
  23. vasild approved
  24. vasild commented at 5:03 pm on February 16, 2021: member
    ACK fa55159b9ede4a915f8ef9e5b90e3e99eadedf91
  25. practicalswift commented at 7:18 pm on February 16, 2021: contributor

    ACK fa55159b9ede4a915f8ef9e5b90e3e99eadedf91

    Nice to see fa comeback after ffff detour.

  26. MarcoFalke merged this on Feb 22, 2021
  27. MarcoFalke closed this on Feb 22, 2021
  28. MarcoFalke deleted the branch on Feb 22, 2021
  29. sidhujag referenced this in commit fda8589af5 on Feb 22, 2021
  30. luke-jr referenced this in commit c9ddaec2db on Jun 27, 2021
  31. DrahtBot locked this on Aug 16, 2022


MarcoFalke vasild practicalswift DrahtBot jonatack

Labels
P2P Utils/log/libs

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-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me