Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 4, 2021

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).

@maflcko maflcko force-pushed the 2101-netLogDisconnect branch from fa5436d to fae5c7c Compare January 4, 2021 14:44
@practicalswift
Copy link
Contributor

ACK fae5c7c92b012bb3ce6fe6d1edee4a2448fdf646

@DrahtBot
Copy link
Contributor

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

@jonatack
Copy link
Member

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

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fae5c7c92b012bb3ce6fe6d1edee4a2448fdf646

As a further improvement maybe the following would make sense:

[patch] distinguish onion from local

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
diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 413e3c034..ecd615995 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -3780,16 +3780,20 @@ bool PeerManager::MaybeDiscourageAndDisconnect(CNode& pnode)
     if (pnode.IsManualConn()) {
         // We never disconnect or discourage manual peers for bad behavior
         LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id);
         return false;
     }
 
-    if (pnode.addr.IsLocal()) {
-        // We disconnect local peers for bad behavior but don't discourage (since that would discourage
-        // all peers on the same local address)
-        LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
+    if (pnode.addr.IsLocal() || pnode.IsInboundOnion()) {
+        // We disconnect local or onion peers for bad behavior but don't discourage
+        // (since that would discourage all peers on the same address)
+        if (pnode.IsInboundOnion()) {
+            LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging onion peer %d!\n", peer_id);
+        } else {
+            LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
+        }
         pnode.fDisconnect = true;
         return true;
     }
 
     // Normal case: Disconnect the peer and discourage all nodes sharing the address
     LogPrint(BCLog::NET, "Disconnecting and discouraging peer %d!\n", peer_id);

@practicalswift
Copy link
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 :)

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 28, 2021
…n and manual peers

Github-Pull: bitcoin#20845
Rebased-From: fae5c7c92b012bb3ce6fe6d1edee4a2448fdf646
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 12, 2021

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

Conflicts

No conflicts as of last run.

@maflcko maflcko changed the title net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers net: Avoid discouraging the onion proxy when one inbound onion misbehaves Feb 15, 2021
@maflcko maflcko changed the title net: Avoid discouraging the onion proxy when one inbound onion misbehaves net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers Feb 15, 2021
@maflcko maflcko force-pushed the 2101-netLogDisconnect branch from fae5c7c to fa55159 Compare February 16, 2021 12:29
@maflcko
Copy link
Member Author

maflcko commented Feb 16, 2021

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.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fa55159

@practicalswift
Copy link
Contributor

ACK fa55159

Nice to see fa comeback after ffff detour.

@maflcko maflcko merged commit 4b6ca4a into bitcoin:master Feb 22, 2021
@maflcko maflcko deleted the 2101-netLogDisconnect branch February 22, 2021 08:47
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2021
…nnect except for noban and manual peers

fa55159 net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers (MarcoFalke)

Pull request description:

  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).

ACKs for top commit:
  practicalswift:
    ACK fa55159
  vasild:
    ACK fa55159

Tree-SHA512: de233bf57334580f9b91f369fafd131d71c5ae25db25b09cc8fa8cbf34c0648f083c52260a6a912238751467e3c3c5f5d2309c145710753058d44a0003f88f4f
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants