#30988 Split CConnman

ReviewabilityReviewsAgreementSize
#30988 vasild Split CConnmanAuthor silent 241d1 (+1) -1Mild1,380 + 50 tests
  • Separate low-level socket I/O from P2P protocol rules to simplify network code
  • Allow socket event polling to be reused by services such as Stratum v2 or an embedded HTTP server
  • Speed up peer lookups from linear to constant time
  • Needs rebase
  • Author inactive for over 200 days, so review responses may be delayed
  • Architectural objection: wrong abstraction level and ill-suited for HTTP or SV2 (theuni)
  • Unaddressed safety objection: returning raw node pointers risks dangling references (hodlinator)
  • Code review approval finding simplified networking and portable transport logic (ryanofsky)
  • Tested peer connections across multiple platforms and signet (pinheadmz, Sjors)
  • Concept approval without stated reasons (jonatack, tdb3)
  • 1,380 lines added or modified outside tests
  • 50 lines added or modified in tests
  • 751 lines removed in total
  • 20 files, 17 commits
P2P#50 of 71
P3 · cleanup
  • P3 after comparing with the other PRs here (assessed alone as P2): Demoted to P3: like #35040, #34824 and #35561 it is architectural restructuring whose payoff (reuse, O(1) lookup) is stated but not yet concrete; those siblings sit at P3.
  • P2 because it addresses longstanding structural coupling between socket I/O and P2P connection logic
  • High maintainability leverage across node networking that can unblock modular reuse in other services
Ranking notes
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

Extracts low-level socket operations (listening, connecting, event loops, send/recv, and I2P SAM sessions) out of CConnman into a new SockMan class located in common/. CConnman inherits from SockMan and handles Bitcoin P2P logic through event-driven virtual callbacks. In addition, CConnman converts its internal node storage from a vector to an unordered_map keyed by NodeId for faster lookups.

Problem: CConnman couples low-level socket I/O and OS polling with higher-level Bitcoin P2P protocol rules, address management, and peer eviction. This tight coupling complicates the networking code and prevents the socket event loop from being reused by other services such as Stratum v2 or an embedded HTTP server.

Evidence of importance:

  • Author states in the description that the PR addresses issue #30694 ("Split socket handling out of CConnman") to make socket handling modular and reusable for projects like Stratum V2 or a libevent-less HTTP server.
  • Commit 63751341d1 notes that switching node storage to an unordered map turns node lookups in DisconnectNode, ForNode, and AttemptToEvictConnection from O(N) into O(1).

Scope: Large structural refactor (+1430/-751) moving networking code to src/common/sockman.cpp and src/common/sockman.h, and moving i2p.cpp into bitcoin_common. Changing CConnman::m_nodes from a vector to an unordered map alters node iteration order, requiring an adjustment to peer eviction logic in PeerManagerImpl::EvictExtraOutboundPeers.

Reviewability

Stale · Author silent 241d

No author activity for 241 days; a review may wait a long time for a response.

Agreement

Mild

objection that needs a change was answered, no reply from the objector (hodlinator: returning a raw CNode pointer after unlocking m_nodes_mutex risks leaving callers with a dangling pointer if another thr; theuni (NACK): SockMan mimics a generic I/O framework without its features, remains tightly coupled to Core's P2P send/recv model, and )

Thread notes: Initial record of discussion. The PR has multiple Concept ACKs and full/code review ACKs, but remains blocked by an open Concept NACK from theuni challenging the abstraction level, alongside an open safety issue from hodlinator on raw CNode lifetime and API fragility comments from ryanofsky.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
hodlinator (contributor)objection, correctnessbind error message string is untranslatedfixed · fix pushed · v1, not yet re-checkedansweredyes2025-01-29: 'Problem: " (<CLIENT_NAME> already running?)"-string is untranslated.'
Status from: 2025-02-07: vasild: 'Done.'
hodlinator (contributor)objection, safetyreturning a raw CNode pointer after unlocking m_nodes_mutex risks leaving callers with a dangling pointer if another thread deletes the nodeanswered · reply on record; standing not assessed yet · v1, not yet re-checkedchangedyes2025-01-30: 'An issue here is that we return a CNode*, unlock m_nodes_mutex, then another thread comes along and deletes the node - leaving the caller with a dangling pointer.'
theuni (member)objection, approachSockMan mimics a generic I/O framework without its features, remains tightly coupled to Core's P2P send/recv model, and is ill-suited for HTTP or SV2answered · reply on record; standing not assessed yet · v1, not yet re-checkedchangedyes2025-03-05: 'I believe this is the wrong abstraction level. SockMan is essentially mimicking the behavior of a generic io multiplexing framework (libevent, libuv, etc.), but without the feature-set of those libs.'
ryanofsky (contributor)objection, interfacemaintaining separate unordered maps to link connection IDs, sockets, and metadata is fragile and prone to getting out of syncanswered · reply on record; standing not assessed yet · v1, not yet re-checkedansweredyes2025-11-17: 'It would seem less fragile to just replace the m_permissions field with a more generic field like std::any m_metadata or std::any m_user_data instead of requiring the application to maintain a separate unordered map and needing to deal with conditions when the map and vector could get out of sync'

Support:

  • tdb3 (contributor) (Concept ACK, 2024-09-29): Concept ACK [not substantive]
  • pinheadmz (member) (Concept ACK, 2024-09-30): This looks great and the API in the header looks easy, thanks.
  • pinheadmz (member) (ACK, 2024-10-18): Built and ran tests on macos/arm as well as debian/x86. Synced to signet with this build on a debian server, then synced from that node locally using the macos build.
  • Sjors (member) (Concept ACK, 2025-01-15): supports splitting socket handling and tested subclassing in Sv2Connman
  • jonatack (member) (Concept ACK, 2025-02-17): Concept ACK [not substantive]
  • ryanofsky (contributor) (Concept ACK, 2025-11-04): Overall I think this is a nice change that simplifies net.cpp by removing low level socket calls and details of establishing connections from implementation of the transport protocol. It also seems like it should make net.cpp code more portable
  • ryanofsky (contributor) (ACK, 2025-11-17): Code review ACK 6091e947504a13df9ee3310e0fce4585c79d19db ... has a lot of nice code improvements in addition to conceptual benefits
  • rkrux (contributor) (Concept ACK, 2026-05-05): for the split of CConnman into smaller and modular parts that should be reused by other components such as libevent-less RPC/HTTP server.

Participants:

  • Sjors (member, support, 43 statements 2024-09-27 to 2025-08-22): Concept ACK; tested integration against Sv2Connman branch and provided extensive review
  • maflcko (member, neutral, 4 statements 2024-09-27 to 2025-02-10): raised nits on comments and discussed std::span conversions
  • tdb3 (contributor, support, 1 statements 2024-09-29 to 2024-09-29): Concept ACK without further comment
  • pinheadmz (member, support, 36 statements 2024-09-30 to 2025-08-07): Concept ACK and full ACK with functional testing, integrating SockMan into an HTTP branch
  • Jacksonearl2468 (none, neutral, 1 statements 2024-10-05 to 2024-10-05): approved without comment and has no project history
  • hodlinator (contributor, objection, 41 statements 2025-01-29 to 2025-02-10): Concept ACK but requested changes; raised safety concern regarding returning raw CNode pointers without locking m_nodes_mutex
  • jonatack (member, support, 1 statements 2025-02-17 to 2025-02-17): Concept ACK
  • theuni (member, objection, 1 statements 2025-03-05 to 2025-03-05): Concept NACK, arguing SockMan is the wrong abstraction level, tightly coupled to Core's P2P model, and ill-suited for HTTP or SV2
  • fanquake (member, neutral, 1 statements 2025-03-13 to 2025-03-13): clarified that SV2 should not be considered a motivating use case for Core P2P changes
  • ryanofsky (contributor, objection, 10 statements 2025-11-04 to 2025-11-18): Concept ACK and review ACK, but raised concerns about API fragility with separate ID/socket maps and thread wake latency
  • rkrux (contributor, support, 1 statements 2026-05-05 to 2026-05-05): Concept ACK for modularity and potential reuse across components

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

The short text the ranking pass compares across PRs.

This PR extracts low-level socket handling—including listening, outbound connections, polling, send/receive loops, and I2P session management—from CConnman into a new SockMan base class. CConnman implements event callbacks for P2P-specific protocol handling and changes its node storage from a vector to an unordered map keyed by NodeId. The change resolves issue #30694 to decouple networking I/O from P2P logic and enable potential reuse in Stratum v2 or RPC servers. Evidence of importance is limited to developer modularity and architectural goals cited by the author, with no reported bug or measurable user-facing performance gain.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-18T03:33, confidence high, input hash f589afe85ee659f6
thread read through 2026-05-05 (248 statements), head 6091e94750
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Needs rebase · draft

Categories

P2P

P3 · cleanup (assessed alone as P2; Demoted to P3: like #35040, #34824 and #35561 it is architectural restructuring whose payoff (reuse, O(1) lookup) is stated but not yet concrete; those siblings sit at P3.)

Splitting socket handling out of CConnman is a major structural maintainability improvement for P2P networking tied to concrete future work like Stratum v2. While reviewers dispute whether the current abstraction cleanly decouples SV2 or HTTP, resolving the tight coupling between socket polling and P2P connection logic addresses a longstanding structural burden (#30694).

Membership: Extensively refactors CConnman in src/net.cpp and src/net.h, separating low-level socket I/O from Bitcoin P2P connection management and modifying peer eviction in src/net_processing.cpp.

Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 2