#32278 doc: better document NetEventsInterface and the deletion of "CNode"s
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #32278 doc: better document NetEventsInterface and the deletion of "CNode"s | Waiting on author | 0 (+1) | Strong | 61 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
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.
Reviewability
Paused · Waiting on author
Open objection from sedited with no author reply for 13 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: response to sedited's question on whether to close the PR 2026-10-19
Agreement
Strong
substantive support, no open objection (w0xlt)
Thread notes: sedited pinged the author to follow up on the previous suggestion to consider closing the PR due to lack of interest.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| laanwj (member) | question, approach | what is meant by "(without a help from the standard library)" | answered · fix pushed · author explained what was meant and removed the phrase | answered | yes | 2025-10-23: 'What do you mean with "(without a help from the standard library)"?' Status from: 2025-11-13: vasild: 'I meant without using shared_ptr, but surely we do use the standard library for other things. Removed this sentence alto' |
| ajtowns (member) | suggestion, interface | phrase documentation as requirements for interface implementers rather than users | fixed · fix pushed · author adopted the suggested phrasing | answered | yes | 2025-10-29: 'I think this would be better phrased as requirements for implementers of the interface, rather than the user of the interface.' Status from: 2025-11-13: vasild: 'Changed.' |
| ajtowns (member) | suggestion, approach | rephrase the CNode destruction documentation into an explicit step-by-step list | fixed · fix pushed · author adopted the suggested wording and credited ajtowns as co-author | answered | yes | 2025-10-29: 'I'd phrase this as: Destroying objects from this list works as follows (see DisconnectNodes(), DeleteNode()):' Status from: 2025-11-13: vasild: 'I took that wording, thanks! Since I copied it right away, I added you as co-author of the commit.' |
| ajtowns (member) | suggestion, maintenance | document how concurrent access to m_nodes_disconnected is prevented | fixed · fix pushed · author added an explanation of concurrent access | answered | yes | 2025-10-29: 'Perhaps document how we prevent concurrent access to this while you're at it?' Status from: 2025-11-13: vasild: 'Good idea! Added some brief explanation amongst those lines, thanks!' |
| sedited (member) | suggestion, usefulness | consider closing the PR due to lack of reviewer interest and documentation changes being less useful | open | answered | no | 2026-09-04: 'Maybe this is not interesting enough to reviewers and should be closed?' |
Support:
- w0xlt (contributor) (ACK, 2025-04-17): Clear improvement in documentation.
Participants:
- w0xlt (contributor, support, 1 statements 2025-04-17 to 2025-04-17): ACKed the PR as a clear improvement in documentation
- laanwj (member, question, 1 statements 2025-10-23 to 2025-10-23): asked for clarification on '(without a help from the standard library)'
- ajtowns (member, neutral, 3 statements 2025-10-29 to 2025-10-29): suggested improvements to documentation phrasing and concurrent access explanation
- sedited (member, neutral, 3 statements 2026-06-19 to 2026-10-19): suggested closing due to lack of reviewer interest and pinged the author for a response
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
The short text the ranking pass compares across PRs.
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.
Data
thread read through 2026-10-19 (14 statements), head 5647c18813
labels: Docs
Categories
Documentation
P3 · cleanup (ranking pass: Internal developer documentation with no user-facing misdirection; sits below the privacy/config docs despite its high standalone score.)
Clarifies complex internal lifecycle rules and call sequencing for developers. The PR documents 'the expected call sequence for NetEventsInterface methods' and 'how CNode reference counting and deletion operate across m_nodes and m_nodes_disconnected' to reduce the likelihood of memory and concurrency errors in networking code.
Membership: Matches the 'Docs' label and consists entirely of explanatory code comments added to src/net.h.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0