net: Avoid discouraging the onion proxy when one inbound onion misbehaves #21190

1. 
   MarcoFalke commented at 3:20 pm on February 15, 2021: member
2. net: Avoid discouraging the onion proxy when one inbound onion misbehaves

   Can be reviewed with --ignore-all-space

   d02e4863a5
3. 
   MarcoFalke commented at 3:21 pm on February 15, 2021: member

   Stolen from @vasild from #20845#pullrequestreview-569138300
4. vasild approved
5. 
   vasild commented at 3:27 pm on February 15, 2021: member

   ACK d02e4863a5a0f92b5024a33a6ae49c74d695b668
6. in src/net_processing.cpp:4032 in d02e4863a5

   4031+        // We disconnect local or onion peers for bad behavior but don't discourage
   4032+        // (since that would discourage all peers on the same address)
   4033+        if (pnode.m_inbound_onion) {
   4034+            LogPrint(BCLog::NET, "Warning: disconnecting but not discouraging onion peer %d!\n", peer_id);
   4035+        } else {
   4036+            LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
   

   ---

   ---

   

   jonatack commented at 3:34 pm on February 15, 2021:

   Seems this ought to be BCLog::NET too -> ok, just saw #20845, it may well be a clearer change to rebase this onto 20845 if it is merged

   ---

   

   MarcoFalke commented at 12:30 pm on February 16, 2021:

   Thanks, combined with #20845
7. in src/net_processing.cpp:4029 in d02e4863a5

   4028-        // all peers on the same local address)
   4029-        LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
   4030+    if (pnode.addr.IsLocal() || pnode.m_inbound_onion) {
   4031+        // We disconnect local or onion peers for bad behavior but don't discourage
   4032+        // (since that would discourage all peers on the same address)
   4033+        if (pnode.m_inbound_onion) {
   

   ---

   ---

   

   jonatack commented at 3:39 pm on February 15, 2021:

   micro-nit, could save a conditional and a level of nesting by testing for IsLocal() and then for m_inbound_onion, though I could see the argument for choosing to group similar cases
8. 
   jonatack commented at 3:39 pm on February 15, 2021: member

   utACK d02e4863a5a0f92b5024a33a6ae49c74d695b66

   A similar change is proposed to the eviction logic in #20197.

9. DrahtBot added the label P2P on Feb 15, 2021
10. 
    DrahtBot commented at 7:16 pm on February 15, 2021: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20845 (net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers by MarcoFalke)
    • #20721 (Net: Move ping data to net_processing by jnewbery)

    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.

11. 
    DrahtBot commented at 11:56 am on February 16, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

12. DrahtBot added the label Needs rebase on Feb 16, 2021
13. 
    MarcoFalke commented at 12:39 pm on February 16, 2021: member

    It seems unlikely that one uses a non-local onion proxy, so I am going to close this for now. Someone else can pick this up. Maybe after #20845 is merged…
14. MarcoFalke closed this on Feb 16, 2021

    ---

15. MarcoFalke deleted the branch on Feb 16, 2021
16. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke vasild jonatack DrahtBot

Labels
P2P Needs rebase