#32278 doc: better document NetEventsInterface and the deletion of "CNode"s
https://github.com/bitcoin/bitcoin/pull/32278 · · +61/-6 in 1 files, 1 commits · labels: Docs
Goal
- Clarify peer connection lifecycles and interface calling orders in networking headers
- Helps developers avoid introducing use-after-free or race condition bugs during networking refactors
This pull request adds detailed explanatory comments to `src/net.h`. It documents the expected call sequence for `NetEventsInterface` methods (`InitializeNode`, `ProcessMessages`/`SendMessages`, `FinalizeNode`) and describes how `CNode` reference counting and deletion operate across `m_nodes` and `m_nodes_disconnected`.
Problem: The lifecycle and concurrency rules governing `CNode` deletion and `NetEventsInterface` invocations were poorly documented, making it easier for developers to introduce use-after-free errors, memory leaks, or thread-safety bugs when modifying networking code.
Category: Documentation (#6 of 9)
P3 · developer docs
- P3 because it documents complex concurrency and lifetime invariants in core networking headers
- Helps developers avoid introducing subtle use-after-free or race condition bugs in future refactors
P3 because it documents non-obvious CNode lifetime, deletion, and concurrency invariants in net.h, providing real value to developers working on peer connection handling.
Membership: The PR consists solely of documentation comments in src/net.h and carries the Docs label.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Reviewability: Stale: Author silent 87 days
- Ready for review
- Clean state with passing CI and no open technical blockers
The author has been inactive for 87 days, exceeding the 60-day stale author threshold.
Author status: silent since 2026-06-22 (87 days)
Open concerns:
- sedited questioned whether the PR is interesting enough to reviewers given its age and suggested it could be closed (no author response).
Resolved concerns:
- laanwj asked what was meant by '(without a help from the standard library)', which the author removed.
- ajtowns suggested phrasing method requirements for interface implementers rather than callers, and requested documenting concurrency safety for m_nodes_disconnected; vasild adopted these suggestions and credited ajtowns as co-author.
Agreement: Strong
- Supported as a clear documentation improvement (w0xlt)
- Helped refine phrasing for interface order and teardown sequences (ajtowns)
- Questions whether there is enough reviewer interest to keep the pull request open (sedited)
Positive; w0xlt ACKed and ajtowns contributed text as co-author, while sedited questioned reviewer interest.
Multiple contributors engaged constructively and supported the doc additions. ajtowns' suggestions were incorporated into the commit, and sedited's question regarding reviewer interest names no technical harm to merging.
- w0xlt ACKed on 2025-04-17: 'Clear improvement in documentation.'
- ajtowns suggested textual refinements on 2025-10-29, which vasild adopted.
- sedited asked on 2026-09-04 whether the PR should be closed due to lack of reviewer interest.
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns | interface | Documentation was originally framed around net.cpp caller requirements rather than interface implementer expectations, and lacked explanation of m_nodes_disconnected thread safety. | resolved | no | yes | 2025-10-29: 'I think this would be better phrased as requirements for implementers of the interface...' Settled: 2025-11-13: vasild incorporated all suggestions and co-authored the commit with ajtowns. |
| sedited | usefulness | open | no | no | 2026-09-04: 'Maybe this is not interesting enough to reviewers and should be closed? These kind of documentation changes might also be less popular...' |
Support:
- w0xlt: Clear improvement in documentation.
- ajtowns: Contributed wording on node deletion sequence and concurrency invariants as co-author.
Participants: w0xlt (support), laanwj (question), ajtowns (objection), sedited (objection)
State derived from the lists: substantive support, no open objection (w0xlt, ajtowns) (model's own read: Positive)
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: w0xlt
Files
0 lines under test/bench/ci.
- src/net.h +61/-6
Uncertainties
- Whether P2P should be considered a member at P4 or left out as member=false; excluded because it is pure documentation without the P2P label.
Card
Documents the expected method call sequence of NetEventsInterface and the CNode reference-counting and deletion lifecycle in src/net.h. It addresses the lack of clear documentation surrounding peer lifetimes and concurrency across the net and message processing threads. The PR has an ACK from w0xlt and co-authored contributions from ajtowns, but has been idle for 87 days following a query on whether it should be closed due to reviewer inactivity. There are no technical blockers or dependencies.