#35561 net: move some CNodeState fields to Peer
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35561 net: move some CNodeState fields to Peer | Needs rebase | 0 (+2) | Strong | 40 + 2 tests | |||
|
|
|
|
| |||
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
Moves `m_requested_hb_cmpctblocks`, `m_provides_cmpctblocks`, and `fPreferredDownload` (renamed `m_preferred_download`) from `CNodeState` to `Peer` as atomic variables. It also converts the counter `m_num_preferred_download_peers` to an atomic integer, eliminating `cs_main` locks in several P2P message processing and block relay codepaths.
Problem: Compact block negotiation and preferred download flags in `CNodeState` are protected by `cs_main`, creating unnecessary global lock contention and risks of lock inversion across P2P message handling.
Evidence of importance:
- Commit b3defd44cb notes that protecting these fields with `cs_main` was unnecessary and converting them to atomics avoids 'potential lock inversion in the future or thread safety annotations'
- Commit 640f486447 notes that converting `fPreferredDownload` and `m_num_preferred_download_peers` to atomics enables dropping `cs_main` usage in message processing
Scope: Commit 9686707821 is a scripted-diff renaming `fPreferredDownload` to `m_preferred_download`.
Risk of a mistake: medium (p2p). Mistakes in the concurrency model or atomic operations when finalizing peers or updating preferred download counts could trigger assertion failures or cause incorrect peer disconnection behavior during initial sync.
Changed since the previous assessment: Rebased without changes to the underlying approach or core logic.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- reviewer: re-review after rebase 2026-09-23
Agreement
Strong
substantive support, no open objection (davidgumberg, dergoegge, mzumsande)
Thread notes: Crypt-iQ rebased the PR after a new field was added upstream to CNodeState. Awaiting reviewer re-confirmation.
Objections, suggestions, and questions: none recorded.
Support:
- davidgumberg (member) (Concept ACK, 2026-06-19) [read]: Less `cs_main` is good, locks are scary, and can hurt performance, and are unnecessary for all of these fields.
- dergoegge (member) (ACK, 2026-06-30) [read: m_num_preferred_download_peers]: The changes themselves look fine/safe to me but I think it might be useful to provide more motivation for this change. The `m_num_preferred_download_peers` change also seems correct to me
- mzumsande (member) (Concept ACK, 2026-08-14) [read: m_wtxid_relay, m_wtxid_relay_peers]: to be fair, we do have the same pattern with `m_wtxid_relay` / `m_wtxid_relay_peers` which are also two atomics, so no strong opinion.
- w0xlt (contributor) (ACK, 2026-09-08): ACK 3861ab49ec77674356ddf3fc1bd090b4b25c2fde [not substantive]
Participants:
- davidgumberg (member, support, 1 statements 2026-06-19 to 2026-06-19): Concept ACK; noted reducing cs_main usage is good and locks are unnecessary for these fields
- dergoegge (member, support, 2 statements 2026-06-30 to 2026-06-30): ACK; verified changes look safe and m_num_preferred_download_peers atomic handling seems correct
- mzumsande (member, support, 3 statements 2026-08-14 to 2026-08-17): Concept ACK; noted existing precedent for the atomic counter pattern
- w0xlt (contributor, support, 1 statements 2026-09-08 to 2026-09-08): ACKed commit 3861ab4
Files
4 lines under test/bench/ci.
- src/net_processing.cpp +39/-39
- src/node/txdownloadman_impl.cpp +1/-1
- test/functional/test_framework/test_framework.py +1/-1
- test/functional/wallet_listsinceblock.py +1/-1
Card
The short text the ranking pass compares across PRs.
PR 35561 moves compact block relay flags and preferred download status from CNodeState to Peer as atomic variables, and converts the m_num_preferred_download_peers counter into an atomic integer. This eliminates several acquisitions of cs_main in net_processing message handling and compact block dispatch. The change is an internal refactoring aimed at reducing cs_main contention and preventing potential lock inversion in P2P processing. It has no external dependencies.
Data
thread read through 2026-09-23 (14 statements), head 9686707821
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: P2P, Needs rebase
Categories
P2P
P3 · cleanup
Reduces global `cs_main` lock scope by moving compact block negotiation and preferred download flags to atomic variables on `Peer`. Reviewers noted this removes unnecessary locks and potential lock inversion hazards, but it represents an incremental internal locking cleanup without an active bug or measured performance bottleneck.
Membership: Carries the P2P label and changes peer state management and lock scoping in src/net_processing.cpp.
Factors: security/stability 1, bug 0, performance 1, user value 0, leverage 0