#35561 net: move some CNodeState fields to Peer

ReviewabilityReviewsAgreementSize
#35561 Crypt-iQ net: move some CNodeState fields to PeerNeeds rebase0 (+2)Strong40 + 2 tests
  • Reduce global lock contention and lock inversion risks during P2P message handling and block relay
  • Allow compact block negotiation and peer download tracking to proceed without locking the main mutex
  • Ready for review
  • Clean rebase with no changes to the underlying logic
  • Strong support across multiple reviewers with no open objections
  • Support for reducing main lock scope where locks are unnecessary (davidgumberg)
  • Verified the changes look safe and the atomic download counter is correct (dergoegge)
  • Concept approval citing existing codebase precedent for the atomic pattern (mzumsande)
  • 40 lines added or modified outside tests
  • 2 lines added or modified in tests
  • 42 lines removed in total
  • 4 files, 3 commits
P2P#51 of 71
P3 · cleanup
  • P3 because this is an incremental internal locking cleanup rather than a fix for an active bug
  • Reduces main lock scope and avoids lock inversion hazards across message processing codepaths
  • Performance benefit is theoretical without measured bottlenecks in practice
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):

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

dossier JSON · extract JSON · model ledger, generated 2026-09-24T02:49, confidence high, input hash 811b0fbec5e9e512
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