net: make m_nodes_mutex non-recursive #32394

1. 
   vasild commented at 12:04 pm on May 1, 2025: contributor

   The only case of a recursive lock was a nested ForNode() call to trim the size of lNodesAnnouncingHeaderAndIDs to <= 3. This need not be nested, so take it out.

   Before:

   0// we know we will push one new element at the back
   1if (size >= 3) pop from front
   2push at the back
   

   After:

   0// maybe push at the back
   1if (size > 3) pop from the front
   

   lNodesAnnouncingHeaderAndIDs is protected by cs_main which is locked during the entire operation.

   Partially resolves: #19303

   ---

   This PR includes #32326 (first 3 commits in this PR). If that PR gets merged first, then the size of this will be reduced. If this gets merged first then that can be closed (will be fully merged via this one).

2. net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr)

   `CConnman::AlreadyConnectedToAddress()` is the only caller of
   `CConnman::FindNode(CNetAddr)`, so merge the two in one function and
   rename it to `IsConnectedToAddr()` to show that it compares just the
   address and not the port.
   
   The unit test that checked whether `AlreadyConnectedToAddress()` ignores
   the port is now unnecessary because now the function takes a `CNetAddr`
   argument. It has no access to the port.

   f8201774b5
3. net: avoid recursive m_nodes_mutex lock in DisconnectNode()

   Have `CConnman::DisconnectNode()` iterate `m_nodes` itself instead of
   using `FindNode()`. This avoids recursive mutex lock and drops the only
   caller of `FindNode()` which used the return value for something else
   than a boolean found/notfound.

   78e0345867
4. net: change FindNode() to not return a node and rename it

   All callers of `CConnman::FindNode()` use its return value `CNode*` only
   as a boolean null/notnull. So change that method to return `bool`.
   
   This removes the dangerous pattern of handling a `CNode` object (the
   return value of `FindNode()`) without holding `CConnman::m_nodes_mutex`
   and without having that object's reference count incremented for the
   duration of the usage.
   
   Also rename the method to better describe what it does.

   4f635d100d
5. net: make m_nodes_mutex non-recursive

   The only case of a recursive lock was a nested `ForNode()` call to trim
   the size of `lNodesAnnouncingHeaderAndIDs` to `<= 3`. This need not be
   nested, so take it out.
   
   Before:
   ```
   // we know we will push one new element at the back
   if (size >= 3) pop from front
   push at the back
   ```
   
   After:
   ```
   // maybe push at the back
   if (size > 3) pop from the front
   ```
   
   `lNodesAnnouncingHeaderAndIDs` is protected by `cs_main` which is locked
   during the entire operation.
   
   Partially resolves: https://github.com/bitcoin/bitcoin/issues/19303

   98bba932bf
6. 
   DrahtBot commented at 12:04 pm on May 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/32394.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	hebasto

   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:

   • #32278 (doc: better document NetEventsInterface and the deletion of “CNode"s by vasild)
   • #32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
   • #32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
   • #30988 (Split CConnman by vasild)
   • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
   • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

   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.

7. DrahtBot added the label P2P on May 1, 2025
8. 
   Bonobirho99 commented at 1:21 pm on May 1, 2025: none

   .
9. 
   hebasto commented at 2:18 pm on May 1, 2025: member

   Partially resolves: #19303

   Concept ACK on that.

Contributors
vasild DrahtBot Bonobirho99 hebasto

Labels
P2P