net: make m_nodes_mutex non-recursive #32394
Open
+112
−100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32394. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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:
|
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
36 tasks
Concept ACK on that. |
This was referenced May 1, 2025
Draft
`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.
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.
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.
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: bitcoin#19303
|
|
This was referenced Jul 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only case of a recursive lock was a nested
ForNode()call to trimthe size of
lNodesAnnouncingHeaderAndIDsto<= 3. This need not benested, so take it out.
Before:
After:
lNodesAnnouncingHeaderAndIDsis protected bycs_mainwhich is lockedduring 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).