doc: better document NetEventsInterface and the deletion of "CNode"s #32278

vasild commented at 4:06 PM on April 15, 2025: contributor

Document the requirements around the NetEventsInterface's methods and the lifetime of CNode objects in CConnman::m_nodes.

DrahtBot commented at 4:06 PM on April 15, 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/32278.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	w0xlt

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:

#34181 (refactor: [p2p] Make ProcessMessage private again, Use references when non-null by maflcko)

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:

no-referenced -> unreferenced [“no-referenced” is not a valid adjective; use “unreferenced” (or “no longer referenced”) to convey that the CNode has no references]

<sup>drahtbot_id_5_m</sup>

DrahtBot added the label Docs on Apr 15, 2025

vasild force-pushed on Apr 16, 2025

vasild commented at 4:16 AM on April 16, 2025: contributor

1091faf8ff...0f26ac35e2 and 0f26ac35e2...e44e669378: further elaborate how we safely access CNodes in m_nodes and how we avoid CNode destruction while it is being referenced.

w0xlt commented at 5:52 PM on April 17, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32278/commits/e44e669378f2c63b09eda68797039b73047febff

Clear improvement in documentation.

DrahtBot added the label CI failed on Apr 28, 2025

DrahtBot removed the label CI failed on May 2, 2025

achow101 requested review from laanwj on Oct 22, 2025

achow101 requested review from theuni on Oct 22, 2025

achow101 requested review from sipa on Oct 22, 2025

achow101 requested review from ajtowns on Oct 22, 2025

in src/net.h:1475 in e44e669378

1470 | +     * CNode objects keep a reference count that is used to avoid a destruction
1471 | +     * while the object is used. All users of elements from `m_nodes` must either
1472 | +     * do so while holding `m_nodes_mutex` or by incrementing the reference before
1473 | +     * use and decrement it after use. To make sure that a new reference is not added
1474 | +     * to a no-referenced CNode just before it is destroyed we use the following
1475 | +     * mechanism (without a help from the standard library):

laanwj commented at 1:48 PM on October 23, 2025:

What do you mean with "(without a help from the standard library)"?

ajtowns commented at 6:03 PM on October 29, 2025:

I'd phrase this as:

Destroying objects from this list works as follows (see DisconnectNodes(), DeleteNode()):

we will close the socket, release locks, and reset global info relating to the connection
the node will be moved from m_nodes to m_nodes_disconnected, preventing future calls to ProcessMessages() and SendMessages() for this node, and decrementing the reference count
we will wait for GetRefCount() to return <= 0 indicating there are no other references to this node in use (and hence no current calls to ProcessMessages or SendMessages)
the node will be removed from m_nodes_disconnected
FinalizeNode() will be called to indicate the node is about to be destroyed
the CNode is deleted

vasild commented at 3:45 PM on November 13, 2025:

What do you mean with "(without a help from the standard library)"?

I meant without using shared_ptr, but surely we do use the standard library for other things. Removed this sentence altogether. @ajtowns I took that wording, thanks! Since I copied it right away, I added you as co-author of the commit. Let me know if you do not want that.

in src/net.h:1006 in e44e669378

1001 | @@ -1002,18 +1002,31 @@ class CNode
1002 |  };
1003 |  
1004 |  /**
1005 | - * Interface for message handling
1006 | + * Interface for message handling.
1007 | + * For a given `CNode`, the methods must be called in the following order:

ajtowns commented at 5:51 PM on October 29, 2025:

I think this would be better phrased as requirements for implementers of the interface, rather than the user of the interface. ie: "For a given CNode, the methods will be called in the following order:". (Both CNode and NetEventsInterface are interfaces the net module offers, so the appropriate documentation is how they work and what you need to do to implement NetEventsInterface, not how net.cpp needs to be implemented)

vasild commented at 3:45 PM on November 13, 2025:

Changed.

doc: better document NetEventsInterface and the deletion of "CNode"s

Document the requirements around the `NetEventsInterface`'s methods and
the lifetime of `CNode` objects in `CConnman::m_nodes`.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

110548c9ea

in src/net.h:1502 in e44e669378 outdated

1484 | +     */
1485 |      std::vector<CNode*> m_nodes GUARDED_BY(m_nodes_mutex);
1486 | -    std::list<CNode*> m_nodes_disconnected;
1487 |      mutable RecursiveMutex m_nodes_mutex;
1488 | +
1489 | +    std::list<CNode*> m_nodes_disconnected;

ajtowns commented at 9:28 PM on October 29, 2025:

Perhaps document how we prevent concurrent access to this while you're at it? (We can't use GUARDED_BY(m_nodes_mutex) since that results in lock order issues with cs_main)

AIUI, accesses are:

DisconnectNodes() via ThreadSocketHandler() which adds nodes and removes them in normal operation
StopNodes() via Stop() which runs it after StopThreads(), which ensures threadSocketHandler is joined before returning. Stop() is invoked in ~CConnman() and init.cpp:Shutdown()

[EDIT: Adding a m_disconnected_nodes_mutex that is locked in ThreadSocketHandler() and StopNodes() would also be feasible afaics, though if we're doing a rewrite just documenting why it works now also seems fine]

vasild commented at 3:46 PM on November 13, 2025:

Good idea! Added some brief explanation amongst those lines, thanks!

vasild force-pushed on Nov 13, 2025

vasild commented at 3:42 PM on November 13, 2025: contributor

e44e669378...110548c9ea: take suggestions

DrahtBot added the label Needs rebase on Feb 7, 2026

DrahtBot commented at 1:47 AM on February 7, 2026: contributor

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