#30988 Split CConnman
https://github.com/bitcoin/bitcoin/pull/30988 · · +1430/-751 in 20 files, 17 commits · labels: Needs rebase · draft
Goal
- Separate low-level socket handling from Bitcoin network protocol logic
- Make networking code more modular and easier for maintainers to maintain
Splits `CConnman` into a low-level socket management class (`SockMan`) and a higher-level Bitcoin P2P protocol layer. Socket operations (bind, listen, accept, poll, send, recv) and socket lifecycle are handled by `SockMan`, while P2P connection policies, banning, and protocol handling remain in `CConnman`. As part of the refactoring, `CConnman::m_nodes` is changed from a vector to an unordered map indexed by connection ID.
Problem: `CConnman` mixes low-level socket polling and OS network calls with high-level Bitcoin P2P state and transport logic, complicating maintenance and preventing reuse in other networking components.
Category: P2P (#56 of 65)
P3 · cleanup
- P3 because it is an internal refactoring without direct user-facing benefit
- Improves peer lookup efficiency to constant time while modularizing network code
- Does not unblock external consumers like HTTP or Stratum v2 as originally envisioned
Decouples low-level socket operations from P2P protocol mechanics in CConnman and makes peer lookup O(1) instead of O(N). Its priority remains P3 because it is an internal refactoring without direct user impact, and the initial motivation of unblocking HTTP and Stratum v2 socket sharing was abandoned.
Membership: Refactors src/net.cpp, src/net.h, and CConnman socket handling and connection data structures.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 1
Reviewability: Stale: Needs rebase
- Review is blocked by merge conflicts with master
- Author has been inactive for over six months while in the middle of reworking the API
The branch has merge conflicts with master and the author has had no activity for over 190 days.
Author status: silent since 2026-03-05 after stating they were midway through implementing ryanofsky's API suggestions
Open concerns:
- theuni Concept NACKed the abstraction, arguing it mimics a generic I/O framework while remaining tightly coupled to Core's P2P send/recv model, making it unsuitable for HTTP or SV2.
- ryanofsky identified API fragility in relying on integer connection IDs and maps instead of passing connection objects and suggested adding thread wakeup mechanisms to avoid polling delays.
Resolved concerns:
- Various lint errors, compiler warnings, and fuzz test harness breakages were fixed during rebases.
- Extracted CAddress cleanups into merged PR #31854.
Agreement: Disputed
- Contested architectural split with multiple Concept ACKs alongside an unresolved blocking objection
- Blocking objection: abstraction is at the wrong level and unsuitable for other protocols (theuni)
- Concept approval for separating low-level socket details from protocol logic (ryanofsky)
- Concept approval for the modularity improvements (Sjors, pinheadmz, tdb3, jonatack, rkrux)
Disputed: theuni Concept NACKs the abstraction; ryanofsky and several others Concept ACK the architectural split.
theuni lodged a blocking Concept NACK arguing that SockMan is the wrong abstraction level and ill-suited for non-P2P use cases like HTTP and SV2. The author defended the split for P2P modularity, and multiple contributors gave Concept ACKs, but the fundamental architectural disagreement remains unresolved.
- theuni: 'I believe this is the wrong abstraction level. SockMan is essentially mimicking the behavior of a generic io multiplexing framework... but without the feature-set of those libs... I'm afraid I'm a concept NACK.'
- vasild: 'IMO this split makes sense even without Sv2 and HTTP because it makes the code more modular and reusable.'
- ryanofsky: 'Concept ACK... Overall I think this is a nice change that simplifies net.cpp by removing low level socket calls and details of establishing connections...'
- Concept ACKs also logged from Sjors, pinheadmz, tdb3, jonatack, and rkrux.
Review verdicts (DrahtBot): 1 (+1) -1
Files
109 lines under test/bench/ci.
- src/net.cpp +299/-552
- src/common/sockman.cpp +532/-0
- src/common/sockman.h +467/-0
- src/net.h +66/-134
- src/test/util/net.h +19/-12
- src/test/fuzz/connman.cpp +17/-11
- src/net_processing.cpp +9/-4
- src/test/net_peer_connection_tests.cpp +5/-6
- test/functional/feature_port.py +5/-5
- src/test/net_tests.cpp +0/-9
- src/test/denialofservice_tests.cpp +0/-6
- src/rpc/net.cpp +4/-0
- src/CMakeLists.txt +2/-1
- src/test/fuzz/net.cpp +0/-3
- src/test/fuzz/util/net.h +0/-3
- src/node/txreconciliation.cpp +1/-1
- src/test/fuzz/p2p_handshake.cpp +1/-1
- src/test/fuzz/p2p_headers_presync.cpp +1/-1
- src/test/fuzz/process_message.cpp +1/-1
- src/test/fuzz/process_messages.cpp +1/-1
Card
PR #30988 splits CConnman into a low-level socket multiplexing class (SockMan) and a P2P protocol layer, while indexing active connections by ID in an unordered map. The split aims to improve modularity and was originally intended to be reused by an HTTP server and Stratum v2, though both consumers have since declined to use it. There is an active architectural dispute: theuni issued a Concept NACK asserting the abstraction is tailored to P2P rather than a clean I/O framework, while ryanofsky and others support the cleanup. The PR is currently in draft with merge conflicts, and the author has been silent for over six months.