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/PeerManagerbehavior - 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.