Net split meta issue #33958

1. 
   theuni commented at 0:17 am on November 27, 2025: member

   Note: This is just a thought dump for now.. an attempt at a high-level roadmap based on an initial POC and the feedback I’ve gathered thus far. I will edit this meta issue frequently as a real roadmap takes shape.

   The goal of this project to cleanly separate the net/net_processing layers, so that neither depends on the implementation details of the other.

   In order to achieve that, we’ll need to de-tangle the two subsystems (namely CConnman and PeerManager) and connect them instead via abstract interfaces.

   An (ugly) initial POC can be seen here: https://github.com/theuni/bitcoin/tree/multiprocess_p2p. Note that it will likely not be rebased because it was not intended to be used as-is. Instead, chunks will be extracted and PR’d separately.

   The above POC takes the split even further, moving all p2p logic into a separate binary, and communicating over IPC. This is not a goal of this project, but it does highlight how useful the split is.

   Major Projects:

   Reduce PeerManager’s usage of CNode

   The main hurdle for creating a clean separation between CConnman and PeerManager is the fact that CNode is used by both.

   CNode is an implementation detail of CConnman, and its leakage causes (among other things) lifetime and locking issues. The Peer structure was created years ago, with the intention that it would hold the state that PeerManager cares about. Unfortunately, this work was never complete.

   So the first chunk of work is moving the necessary state from CNode to Peer. In many cases, the logic belongs purely at one layer or another. In others (for ex, node id, connection time, etc), both layers will keep copies.

   In order to do that cleanly, some helper functions will move out of CConnman/CNode and into PeerManager/Peer, and others will be split into generic helpers used by both layers.

   Move the message handling loop to PeerManager

   When PeerManager’s dependency on CConnman/CNode is reduced to a reasonably small surface, the message handling loop can be move out of CConnman. Rather than iterating all CNodes, it will now iterate all Peers. Ugliness like ForNode/ForEachNode can be removed.

   Dropped usage of CNode in PeerManager

   With the event loop moved and nascent interfaces beginning to take shape, PeerManager should be able to operate without CNode. At this point, all of CConnman’s public-facing functions will take a NodeId rather than CNode pointer.

   Virtual interfaces and dropped includes

   Lastly, the layers will be severed, includes dropped, and they will only communicate via virtual interfaces. PeerMan’s NetEventsInterface will be filled in, and CConnman will implement a new virtual interface.

   Outstanding questions

   (This list will expand as the project moves along)

   Unit tests and fuzzers

   It’s not immediately obvious what should happen to tests and fuzzers which rely on:

   • CNode’s internals
   • Assumptions about CConnman/PeerManager behavior
   • Assumptions about layer violations between the two

   MANY tests rely on implementation details. Those tests are in conflict with the aim of this project, which is to largely turn those subsystems into black boxes. However, this actually makes the tests easier and more functional, because it becomes possible to test a specific behavior without having to resort to hacking the implementation.

   As an example, because PeerManager’s NodeConnected interface function will now require all of the properties of the connected node, it becomes easier to specify (fake) an exact node configuration without actually creating a node with those properties.

   Rather than modifying the existing unit tests and fuzzers to work as-is, I think it makes sense to take advantage of the new functionality. Likely as the project progresses and paradigms change, some tests will be modified and others will be dropped/rewritten entirely.

2. maflcko added the label Refactoring on Nov 27, 2025
3. maflcko added the label Tests on Nov 27, 2025
4. maflcko added the label P2P on Nov 27, 2025
5. maflcko added the label Tracking Issue on Nov 27, 2025
6. maflcko added the label Brainstorming on Nov 27, 2025
7. SmartDever02 referenced this in commit 709bf81fe0 on Dec 1, 2025
8. SmartDever02 referenced this in commit e8f550f9ed on Dec 1, 2025
9. 
   ajtowns commented at 10:18 am on December 2, 2025: contributor

   Reduce PeerManager’s usage of CNode Move the message handling loop to PeerManager

   Big fan of these aspects. I like the documentation aspect of NetManagerEvents. Would like to see the PeerManager functions move towards accepting a Peer& (vs a CNode* and looking up GetPeerRef, or an explicit CNode, Peer pair) in general too.

   Dropped usage of CNode in PeerManager

   With the event loop moved and nascent interfaces beginning to take shape, PeerManager should be able to operate without CNode. At this point, all of CConnman’s public-facing functions will take a NodeId rather than CNode pointer.

   I think this part is going in the wrong direction though.

   In particular, I think good goals to have here are (not in priority order):

   1. simplify the codebase so it’s easier to understand and update
   2. improve robustness of the codebase, so that if there are bugs, they have less impact
   3. make the code higher performance, so that we can have more peers, and/or run on cheaper hardware

   Getting a better division between Connman and PeerManager is great for the first point. I don’t think it’s a big improvement in the second point – that would come if we could separate PeerManager’s parsing of random data from the internet and generally complex logic from mempool/block validation.

   For the last point, I think there are two main principles we should aim for: (a) minimising the movement and reparsing of data, and (b) minimising the locking and (potentially) maximising the cache performance when we send/receive data. Currently, when we send data and don’t need to block, we:

   • encode the message as two byte streams (the msg_type as a string and the data as a byte vector)
   • take the node’s cs_vSend mutex, move the message into a dequeue and attempt to send the message if the socket seemed idle
   • when sending the message, we do the transport encoding, potentially converting the msg_type to a number, and queue data on the socket
   • after releasing cs_vSend, if we actually sent data, briefly take the global lock m_total_bytes_sent_mutex to track total bytes sent and bytes sent in cycle

   With the POC code, we’re adding extra steps between the first two points:

   • pass the message and the node id to the new p2pinterface
   • if there’s process separation, do a capnp encode-decode cycle and moving the data to another process over IPC
   • taking the global m_nodes_mutex lock and doing a search for the node id through m_nodes, and getting a shared copy of the CNode
   • doing the same logic as above

   So, like I said, that seems headed in the wrong direction to me. In particular introducing IPC in between the two seems like a misfeature – it adds complexity and lowers performance (with both extra locks and extra data movement) for not really any benefit.

   Rather than having net_processing communicate with net via either a full CNode* or just an integer NodeId, I think a better approach would be to give it an opaque handle with a bunch of inlines, something like:

    0class NodeHandle
    1{
    2private:
    3    std::shared_ptr<CNode> node;
    4public:
    5    friend CConnman;
    6    set_disconnect() { node->fDiscconnect = true; }
    7};
    8
    9class CConnman
   10{
   11    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
   12    inline void PushMessage(NodeHandle& nodehandle, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)
   13    {
   14        PushMessage(nodehandle.node.get(), std::move(msg));
   15    }
   16};
   

   You’ve also used NodeId internally within net_processing; so eg ThreadMessageHandler() takes node_id = peer->m_id, then calls ProcessMessages(node_id) and SendMessages(node_id) with both immediately recovering the peer with peer = GetPeerRef(node_id). Presumably that’s just because those are exposed for testing but Peer isn’t; probably could fix it by just also exposing GetPeerRef for testing as well, I think, and calling if (auto p = GetPeerRef(id); p) ProcessMessages(*p); instead of ProcessMessages(id)?

   Lastly, the layers will be severed, includes dropped, and they will only communicate via virtual interfaces.

   As above, I don’t really think severing the layers is a good idea (at least for process separation reasons, maybe for testing harnesses though?), but I think you could make NodeHandle a virtual interface fairly reasonably if it were, while still keeping things quite fast when it’s implemented as a lightweight handle to a CNode*.

   While thinking about this, I’ve been looking at two other aspects, which I’d like to mention for possible discussion:

   NetMsgType::ADDR etc are currently constant strings (with SSO they’ll avoid allocations at least though), so as a result, net_processing has to sanitize them when logging, and when tracking message bytes, we use a map<string,int> to keep track of them, and likewise have an unordered_map<string,uint8_t> for the transport v2 message ids, which seems overly complex when we only actually care about the 30ish messages we know about in almost all situations. Changing this doesn’t look like too bad a cleanup to me: https://github.com/ajtowns/bitcoin/commits/202511-enum-netmsgtype/

   We have a lot of locks in the net/netproc subsystem these days (eg, when I run a debug binary listening on mainnet, I get ~100% CPU usage sustained, mostly due to DEBUG_LOCKORDER; dropping DEBUG_LOCKORDER gets me down ~30% CPU), and it would be nice to work on reducing them, rather than adding more – ie, having data be accessed by a single thread, rather than shared across threads. I’ve been looking whether we can change the way PushMessage etc works, so that the socket operations all happen in the socket handler thread, rather than also opportunistically from the sending thread. I think the two parts to making that work while still being low latency are (a) having a fast message queue in CNode that PeerMan can push to and Connman can pull from, and (b) a way of cheaply notifying Connman to wake up from its poll() and start sending newly queued data. Feels a fair bit like reinventing the wheel vs libevent/libuv/whatever, but I think as long as we give net_processing some sort of “handle” object, it can be made fairly simple/safe as well as being fast.

   I’d also like to get some cleanup of the Transport stuff – even just separating it into its own module as a start maybe. In particular, if we introduce new one-byte BIP324 message-types, and have those negotiated over p2p rather than just decided at connection time, then I don’t think we need the conversion between short id and msg_type to happen at the net_processing side of the queue, rather than the net side of the queue where it currently occurs, which probably means it’s better to change CNetMessage and CSerializedNetMsg to contain a ~13 byte SerializedNetMsgType where v1 transport has the first byte as always 0, and netproc becomes responsible the short id encoding, rather than V2Transport. Probably should do that before any of the NetMsgType stuff above.

   (EDIT: concretely, moving the bip324 shortid logic to net_processing also requires moving the bytes{sent,rev}permsg logic to net_processing, which requires rpc/net’s sendmsgtopeer to go through net_processing, which would involve additional lock juggling if net_processing were handling the msgproc thread rather than net. EDIT^2: Hey, success after 3 years: https://github.com/bitcoin/bips/pull/1378#discussion_r2585766526 . The locking isn’t too bad as is, but would still improve slightly with the msgproc loop having access to PeerManagerImpl, and that could better ensure the std::promise stuff didn’t get lost too)

10. 
    theuni commented at 5:51 pm on December 8, 2025: member

    In particular, I think good goals to have here are (not in priority order):

    1. simplify the codebase so it's easier to understand and update
    
    2. improve robustness of the codebase, so that if there are bugs, they have less impact
    
    3. make the code higher performance, so that we can have more peers, and/or run on cheaper hardware
    

    Goal #1 is a logical separation of concerns. Today, the many of interactions between CConnman and PeerManager are subtle and (I’d guess) virtually unknown to all but 2 or 3 people. In particular, issues around lifetime and disconnection.

    Getting a better division between Connman and PeerManager is great for the first point. I don’t think it’s a big improvement in the second point – that would come if we could separate PeerManager’s parsing of random data from the internet and generally complex logic from mempool/block validation.

    For the last point, I think there are two main principles we should aim for: (a) minimising the movement and reparsing of data, and (b) minimising the locking and (potentially) maximising the cache performance when we send/receive data. Currently, when we send data and don’t need to block, we:

    * encode the message as two byte streams (the msg_type as a string and the data as a byte vector)
    
    * take the node's `cs_vSend` mutex, move the message into a dequeue and attempt to send the message if the socket seemed idle
    
    * when sending the message, we do the transport encoding, potentially converting the msg_type to a number, and queue data on the socket
    
    * after releasing `cs_vSend`, if we actually sent data, briefly take the global lock `m_total_bytes_sent_mutex` to track total bytes sent and bytes sent in cycle
    

    I’m all for these goals, but I consider almost all of those things to be premature optimizations compared to the real-world bottlenecks that we currently see. As two examples, I’d challenge you to demonstrate a meaningful statistical slowdown caused by cs_vSend as compared to:

    • the time we take to encrypt/decrypt bip324 messages. ( I have a branch which gives us a ~3x speedup here, PR coming soon)
    • The archaic ProcessMessages/SendMessages loop as compared to multi-threaded event-based message handling

    I realize those seem like disjointed/unrelated arguments, but I’m trying to make a larger point: I’d rather not spend time re-factoring to avoid some map/lookup inefficiencies when we have existing massive architectural logjams.

    With the POC code, we’re adding extra steps between the first two points:

    * pass the message and the node id to the new p2pinterface
    
    * if there's process separation, do a capnp encode-decode cycle and moving the data to another process over IPC
    
    * taking the global `m_nodes_mutex` lock and doing a search for the node id through m_nodes, and getting a shared copy of the CNode
    
    * doing the same logic as above
    

    So, like I said, that seems headed in the wrong direction to me. In particular introducing IPC in between the two seems like a misfeature – it adds complexity and lowers performance (with both extra locks and extra data movement) for not really any benefit.

    Again, I’d ask that you not worry too much about the map lookups and data copies. In principle I completely agree.. if we were rewriting this all from scratch I’d prefer to aim for a zero-copy and non-pipeline-stalling implementation. But the reality is that we have heaps of tech debt that overshadow these inefficiencies.

    As for the process separation.. maybe I shouldn’t have even brought that up. It’s something that becomes possible with this architecture, but it’s a very reasonable argument that it’s the wrong split. Please think of it as a toy rather than an end-goal.

    Rather than having net_processing communicate with net via either a full CNode* or just an integer NodeId, I think a better approach would be to give it an opaque handle with a bunch of inlines, something like:

    class NodeHandle { private: std::shared_ptr node; public: friend CConnman; set_disconnect() { node->fDiscconnect = true; } };

    class CConnman { void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); inline void PushMessage(NodeHandle& nodehandle, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex) { PushMessage(nodehandle.node.get(), std::move(msg)); } };

    This reintroduces the exact lifetime issues I’m trying to remove :(

    Again, compared to everything else happening in these threads, I consider avoiding an uncontested lock and map lookup to be a premature optimization. Unless it can be demonstrated that these operations even show up in a flamegraph, I don’t think reintroducing the complexity of shared ownership is worth the complexity.

    Remember that the message handling loop is now happening in PeerManager, so the locking of m_nodes_mutex is now much less interesting. And even if it does ever show up in benchmarks, because it’s only ever modified when nodes are connected/disconnected, it could be made a read/write lock.. that would ensure that it’s essentially never contended.

    HOWEVER, I did anticipate this argument and came up with a similar solution, but with a tweak to avoid the layer violations. See here for the actual impl: https://github.com/theuni/bitcoin/commit/6a9710417d29e7eab1aa5b515bfb1b1ea0ce5b9c

    Tl;dr: It’s essentially the same thing you’re suggesting above, but it looks like this (simplified, missing type safety) instead:

     0struct NodeHandle
     1{
     2    std::weak_ptr<void> m_node;
     3    NodeId m_id;
     4};
     5
     6class CConnman
     7{
     8private:
     9    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
    10public:
    11    inline void PushMessage(const NodeHandle& nodehandle, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)
    12    {
    13        if (auto node = std::static_pointer_cast<CNode>(nodehandle.m_node.lock()); node) {
    14            PushMessage(node.get(), std::move(msg));
    15        }
    16    }
    17};
    

    It exploits std::weak_ptr<void> as an opaque handle (while keeping CNode’s deleter).

    The above has the same benefits you’re after:

    • Avoids locking
    • No map lookups

    But without the layer violations:

    • PeerManager doesn’t influence CNode lifetimes (except for the duration of the weak->shared)
    • PeerManager sees a 100% opaque type.

    But again, I consider this to be a last resort in the (imo incredibly unlikely) event that the read-lock + map lookup causes any meaningful slowdown.

    You’ve also used NodeId internally within net_processing; so eg ThreadMessageHandler() takes node_id = peer->m_id, then calls ProcessMessages(node_id) and SendMessages(node_id) with both immediately recovering the peer with peer = GetPeerRef(node_id). Presumably that’s just because those are exposed for testing but Peer isn’t; probably could fix it by just also exposing GetPeerRef for testing as well, I think, and calling if (auto p = GetPeerRef(id); p) ProcessMessages(*p); instead of ProcessMessages(id)?

    I stopped short of optimizing these things in my branch. But sure, there’s lots of room for optimizing things like that.

    I’d also like to get some cleanup of the Transport stuff – even just separating it into its own module as a start maybe. In particular, if we introduce new one-byte BIP324 message-types, and have those negotiated over p2p rather than just decided at connection time, then I don’t think we need the conversion between short id and msg_type to happen at the net_processing side of the queue, rather than the net side of the queue where it currently occurs, which probably means it’s better to change CNetMessage and CSerializedNetMsg to contain a ~13 byte SerializedNetMsgType where v1 transport has the first byte as always 0, and netproc becomes responsible the short id encoding, rather than V2Transport. Probably should do that before any of the NetMsgType stuff above.

    Sure, this seems great. Seems to me it’s 3 distinct layers anyway :)

11. 
    ajtowns commented at 9:29 pm on December 8, 2025: contributor

    For the last point, I think there are two main principles we should aim for: (a) minimising the movement and reparsing of data, and (b) minimising the locking and (potentially) maximising the cache performance when we send/receive data. Currently, when we send data and don’t need to block, we:

    I’m all for these goals, but I consider almost all of those things to be premature optimizations compared to the real-world bottlenecks that we currently see.

    The single biggest bottleneck I see today is due to DEBUG_LOCKORDER which takes a dev build from using ~10%-40% of a VPS core to ~100%.

    Otherwise, we waste a fair bit of time in NodeSnapshot (1.98% - mostly shuffle()), CNodeState lookups (1.97%), GetPeerRef lookups (1.01%), GenerateWaitSockets (3.61%), and, at least for what it does, AccountForSentBytes (0.11%),

    We spend a lot of time doing ProcessTransaction (20%), TxDownloadManager (12.27%),CompareInvMempoolOrder heap ops (2.7%), RollingBloomFilter stuff (5.8%), and serialization and v2 en/decryption (3.06%), but at least they’re all things we’re actively trying to do.

    As two examples, I’d challenge you to demonstrate a meaningful statistical slowdown caused by cs_vSend as compared to:

    cs_vSend is a per-peer lock, so I don’t think it’s very problematic. The main reason to look at removing it is that it’s about protecting “net” internals and the only purpose of having the lock is to allow those internals to be touched by the msghand thread (and perhaps the rpc threads for sendmsgtopeer), rather than just the net thread, because queuing data for 50ms while waiting for the net thread to wakeup, when it could just be sent immediately is pretty lame.

    * the time we take to encrypt/decrypt bip324 messages. ( I have a branch which gives us a ~3x speedup here, PR coming soon)
    

    Presuming my percentages aren’t just due to being a dev build, wow!

    * The archaic ProcessMessages/SendMessages loop as compared to multi-threaded event-based message handling
    

    I don’t think ProcessMessages or SendMessages is particularly bad at present; though the NodeSnapshot and GenerateWaitSockets stuff isn’t great, and maybe that’s just a question of where in the stack you’re assigning the blame.

    Switching to a “multi-threaded” model is a big ask, that I think’s better treated as a separate project (making tx and block processing happen async in another thread, while msg handling continues for other peers? making it possible to process multiple txs/blocks simultaneously?). But outside of changing the way we interact with validation / cs_main, I don’t think there’s a big win for the multithreaded part here, when we’re only dealing with 100s of peers and maybe a few MB/s of data.

    Making ProcessMessages more event oriented seems pretty plausible. :+1:

    Making SendMessages more event based otoh seems somewhat more difficult; I’d like to be able to think more about that after ThreadMessageHandler is moved. We’ve got lots of if (current_time > peer.m_next_send_feefilter) tests which might translate well to something event based, but it seems like a lot of code movement, and doesn’t seem that bad as-is, so I’m not sure.

    I realize those seem like disjointed/unrelated arguments, but I’m trying to make a larger point: I’d rather not spend time re-factoring to avoid some map/lookup inefficiencies when we have existing massive architectural logjams.

    I look at it more the other way: it’s simpler and easier to understand when functions are directly given the data they’re supposed to operate on. ProcessMessage is meant to take a message from the wire (ie, a type and a payload) from a logical peer (ie, Peer) and do whatever’s necessary to deal with that message. Its signature should be ProcessMessage(Peer&, string_view, DataStream&&) with Peer& giving it immediate access to everything it needs, without having to look data up, and deal with errors if that data is mysteriously unavailable.

    If we’re trying to simplify our net layers, we shouldn’t be adding abstractions that we constantly switch between.

    If we can get ThreadMessageHandler moved into net_processing, I think we’d be pretty close to being able make Peer objects private to the msghand thread – where we currently have other threads accessing Peer directly (eg UpdatedBlockTip), I think we could change to adding to an event queue that msghand multiplexes to each (relevant) peer. (Or have the events first get multiplexed to per-msghand event queues if there are multiple msghand threads), which I think would clear up a huge bunch of locks and simplify a huge bunch of thread safety considerations, eg.

    Again, I’d ask that you not worry too much about the map lookups and data copies. In principle I completely agree.. if we were rewriting this all from scratch I’d prefer to aim for a zero-copy and non-pipeline-stalling implementation. But the reality is that we have heaps of tech debt that overshadow these inefficiencies. As for the process separation.. maybe I shouldn’t have even brought that up. It’s something that becomes possible with this architecture, but it’s a very reasonable argument that it’s the wrong split. Please think of it as a toy rather than an end-goal.

    I think passing around NodeIds instead of the actual data structures we want is adding more tech debt, and putting IPC in between message generation and sending the message on the socket would also be adding more tech debt. (Outside of the IPC POC, I think the only data copy issues are in NodeSnapshot and WaitMany, which I expect this project will improve anyway, or at least not make any worse)

    Rather than having net_processing communicate with net via either a full CNode* or just an integer NodeId, I think a better approach would be to give it an opaque handle with a bunch of inlines, something like: class NodeHandle { private: std::shared_ptr node;

    This reintroduces the exact lifetime issues I’m trying to remove :(

    I’m pretty sure if you’re reworking CConnman, CNode, Peer and PeerManager you can make that arrangement work okay.

    What I was loosely thinking of is that when either PeerMan or CConnman decides it doesn’t want that CNode anymore (because it’s been misbehaving, or because the socket closed, eg) they set an fDisconnect-like flag, and when they next notice the node (whether that’s due to a timeout or triggers an event), whichever hasn’t already dropped it drops it now, so that the shared_ptr is freed, and CNode’s various queues are released. (The socket is already closed prior to CConnman abandoning the shared_ptr)

    In particular, CConnman is not invoking NetEventsInterface::FinalizeNode(cnode) or m_msgproc->markNodeDisconnected(node->GetId()) in this arrangement, peerman is doing that directly when it next gets around to checking what’s going on with that node, and finding that connman’s already abandoned the node, and concludes it should too. It’s only possible to do things that way after PeerManager has its own thread in order to iterate over its peers of course.

    HOWEVER, I did anticipate this argument and came up with a similar solution, but with a tweak to avoid the layer violations. See here for the actual impl: theuni@6a97104

    I mean, that’s fine, I guess, but having the CNode object available but closed seems better than having to check if a weak_ptr is expired. It also allows the msgproc thread to deal with any messages that were received but unprocessed at the time the socket closed. (It’s especially fine if it’s just an intermediate step while connman is still calling peerman’s FinalizeNode)

    You’ve also used NodeId internally within net_processing; so eg ThreadMessageHandler() takes node_id = peer->m_id, then calls ProcessMessages(node_id) and SendMessages(node_id) with both immediately recovering the peer with peer = GetPeerRef(node_id). Presumably that’s just because those are exposed for testing but Peer isn’t; probably could fix it by just also exposing GetPeerRef for testing as well, I think, and calling if (auto p = GetPeerRef(id); p) ProcessMessages(*p); instead of ProcessMessages(id)?

    I stopped short of optimizing these things in my branch. But sure, there’s lots of room for optimizing things like that.

    Do you understand where I’m coming from when I say I don’t think of that as an optimisation, but rather the natural way to write the code? If FooClass::FuncA() has an object Bar b, and FooClass::FuncB() wants to use that object, it should just pass it directly by calling FuncB(b), rather than calling FuncB(b.id) and having FuncB have to recover it with auto b = GetByById(id); if (!b) return;.

    I’d also like to get some cleanup of the Transport stuff – even just separating it into its own module as a start maybe. In particular, if we introduce new one-byte BIP324 message-types, and have those negotiated over p2p rather than just decided at connection time, then I don’t think we need the conversion between short id and msg_type to happen at the net_processing side of the queue, rather than the net side of the queue where it currently occurs, which probably means it’s better to change CNetMessage and CSerializedNetMsg to contain a ~13 byte SerializedNetMsgType where v1 transport has the first byte as always 0, and netproc becomes responsible the short id encoding, rather than V2Transport. Probably should do that before any of the NetMsgType stuff above.

    Sure, this seems great. Seems to me it’s 3 distinct layers anyway :)

    I think I count five layers, fwiw:

    1. socket handling: util/sock.h, b-net thread, net.h, net.cpp
    2. wire encoding: V1Transport, V2Transport, protocol.h
    3. message protocol: ProcessMessage / SendMessages
    4. connection preference: ThreadOpenConnections, eviction handling
    5. sharing policy: header/block download/announcements, TxRequestTracker, address relay

    Maybe 4 and 5 are both just a “policy layer”? I left them separate since (4) is managed by net.cpp, while (5) is exclusively net_processing.cpp; but maybe the openconnections logic should move to net_processing as well as the evictions logic? I think doing that might allow connman to stop needing NetEventsInterface entirely, and instead just use flags on CNode to communicate markNodeDisconnected and markSendBufferFull(bool).

12. bitcoin deleted a comment on Dec 9, 2025

Contributors
theuni ajtowns

Labels
Brainstorming Refactoring Tests P2P Tracking Issue