refactor/p2p: Decouple CNode from PeerManagerImpl::MaybeDiscourageAndDisconnect #33983

1. 
   kevkevinpal commented at 2:56 pm on December 1, 2025: contributor

   Description

   Refactor MaybeDiscourageAndDisconnect to accept individual parameters (has_no_ban, is_manual, is_local_addr, is_inbound_onion, peer_addr) instead of taking a CNode& reference directly.

   This decouples the function from CNode while preserving all original logic paths and behavior.

   This change is just a refactor, so no tests were modified

   Linked issue

   This is a part of #33958. Let me know if this is not needed and I can close this PR

2. 
   DrahtBot commented at 2:56 pm on December 1, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33983.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. refactor/p2p: Decouple CNode from PeerManagerImpl::MaybeDiscourageAndDisconnect

   Refactor MaybeDiscourageAndDisconnect to accept individual parameters (has_no_ban, is_manual, is_local_addr, is_inbound_onion, peer_addr)
   instead of taking a CNode& reference directly.
   This decouples the function from CNode while preserving all original logic paths and behavior.

   635fc6a814
4. in src/net_processing.cpp:4967 in 15ce12c609 outdated

   4973+    if (is_local_addr) {
   4974         // We disconnect local peers for bad behavior but don't discourage (since that would discourage
   4975         // all peers on the same local address)
   4976         LogDebug(BCLog::NET, "Warning: disconnecting but not discouraging %s peer %d!\n",
   4977-                 pnode.m_inbound_onion ? "inbound onion" : "local", peer.m_id);
   4978-        pnode.fDisconnect = true;
   

   ---

   ---

   

   dergoegge commented at 3:16 pm on December 1, 2025:

   This should not be removed

   ---

   

   kevkevinpal commented at 3:27 pm on December 1, 2025:

   I added a should_disconnect parameter and after this function is called we do the following

   0if (should_disconnect) {
   1    pto->fDisconnect = true;
   2}
   3return true;
   

5. kevkevinpal force-pushed on Dec 1, 2025
6. DrahtBot added the label CI failed on Dec 1, 2025
7. 
   dergoegge commented at 3:20 pm on December 1, 2025: member

   In my opinion this is too fine grained of a step and not worth a PR on its own, as it will cause more churn down the line (e.g. the signature of MaybeDiscourageAndDisconnect will likely have to change again).
8. 
   kevkevinpal commented at 3:24 pm on December 1, 2025: contributor

   In my opinion this is too fine grained of a step and not worth a PR on its own, as it will cause more churn down the line (e.g. the signature of MaybeDiscourageAndDisconnect will likely have to change again).

   Gotcha, should I attempt to decouple more/all functions in PeerManagerImpl. I was worried about attempting to make too large a PR, but I guess I went the other extreme, haha

9. 
   maflcko commented at 3:30 pm on December 1, 2025: member

   Can you explain this a bit better?

   An (ugly) initial POC can be seen here: https://github.com/theuni/bitcoin/tree/multiprocess_p2p. Note that it will likely not be rebased because it was not intended to be used as-is. Instead, chunks will be extracted and PR’d separately.

   Did you cherry-pick a commit from this branch? If not, why not? If yes, why did the author change?

10. 
    kevkevinpal commented at 3:40 pm on December 1, 2025: contributor

    Can you explain this a bit better?

    An (ugly) initial POC can be seen here: https://github.com/theuni/bitcoin/tree/multiprocess_p2p. Note that it will likely not be rebased because it was not intended to be used as-is. Instead, chunks will be extracted and PR’d separately.

    Did you cherry-pick a commit from this branch? If not, why not? If yes, why did the author change?

    I did not cherry-pick commits from this branch but now looking at it, I prefer the approach here instead. Will close this PR

11. kevkevinpal closed this on Dec 1, 2025

    ---

Contributors
kevkevinpal DrahtBot dergoegge maflcko

Labels
CI failed