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

   • #32326 (net: improve the interface around FindNode() and avoid a recursive mutex lock by vasild)
   • #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)
   • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
   • #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.

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • Search for a “preferred” network, a reachable network to which we -> * Search for a “preferred” network, a reachable network to which we can connect. [incomplete sentence]
3. DrahtBot added the label P2P on May 1, 2025
4. 
   Bonobirho99 commented at 1:21 pm on May 1, 2025: none

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

   Partially resolves: #19303

   Concept ACK on that.

6. DrahtBot added the label Needs rebase on May 20, 2025
7. 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.

   41b88f6fc9
8. 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.

   7e50f8bc67
9. 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.

   39d2fc0e17
10. 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

    22d4c57a99
11. vasild force-pushed on May 21, 2025
12. 
    vasild commented at 11:33 am on May 21, 2025: contributor

    98bba932bf...22d4c57a99: rebase due to conflicts
13. DrahtBot removed the label Needs rebase on May 21, 2025

Contributors
vasild DrahtBot Bonobirho99 hebasto

Labels
P2P