refactor: Split per-peer parts of net module into new node/connection module

ajtowns commented at 10:23 am on October 19, 2023: contributor

Move per-peer code from net into a new node/connection module. This is intended to capture logic that is lower-level than the p2p protocol (done in net_processing), but higher-level than raw bits-over-the-wire (handled by Sock).

DrahtBot commented at 10:23 am on October 19, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	naumenkogs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28857 (test, refactor: Magic bytes array followup by TheCharlatan)
#28690 (build: Introduce internal kernel library by TheCharlatan)
#28455 (refactor: share and use GenerateRandomKey helper by theStack)
#28451 (refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by maflcko)
#27600 (net: Add new permission forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)
#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
#26697 (logging: use bitset for categories by LarryRuane)
#25832 (tracing: network connection tracepoints by 0xB10C)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Oct 19, 2023

ajtowns commented at 10:31 am on October 19, 2023: contributor

The idea here is to restructure CNode / CConnman / PeerManager a bit. Currently, net.h combines all the p2p logic in between the very low-level socket stuff (sock.h) and the high-level protocol (net_processing.h). This splits that into two parts: node/node.h that now contains CNode, TransportV1, etc that focusses on dealing with a single peer; and leaves CConnman in net.h, with the idea that that focusses on coordinating amongst the many peers we have (so, opening new connections, choosing what to evict, working out which node to give cpu time to next, etc).

This PR just moves code around; the idea is that future PRs would improve the separation more (see https://github.com/ajtowns/bitcoin/commits/202308-netnode and #28252 (comment)) and, eventually, breaking the circular dependency between PeerManager and CConnman (ie, have PeerManager only deal with a single node at a time, eg see the PushMessage changes in the repo just mentioned).

ajtowns force-pushed on Oct 19, 2023

maflcko commented at 11:37 am on October 19, 2023: member

node/node seems a bit confusing, because the two node refer to different things. node refers to this node and the other node refers to a peer, so node/peer may be better?

ajtowns commented at 11:52 am on October 19, 2023: contributor

node/node seems a bit confusing, because the two node refer to different things. node refers to this node and the other node refers to a peer, so node/peer may be better?

I did think about that, but it’s confusing in its own way: we have class Peer in net_processing that contains the per-peer protocol-level data for a particular connection. I guess we could decide to move towards calling them Peer (instead of CNode) and PeerLogic (instead of Peer), and PeerLogicManager (instead of PeerManager)?

dergoegge commented at 12:22 pm on October 19, 2023: member

While we are bike-shedding the naming, I actually think node/connection would make sense. Connection would also seem like the appropriate name for CNode given that it is managed by something called connection manager.

ajtowns force-pushed on Oct 19, 2023

ajtowns commented at 1:14 pm on October 19, 2023: contributor

While we are bike-shedding the naming, I actually think node/connection would make sense. Connection would also seem like the appropriate name for CNode given that it is managed by something called connection manager.

I like it! Done.

ajtowns renamed this:
~~refactor: Split per-peer parts of net module into new node/node module~~
refactor: Split per-peer parts of net module into new node/connection module
on Oct 19, 2023

in src/test/validation_tests.cpp:7 in 1116e57de9 outdated

3@@ -4,7 +4,6 @@
4 
5 #include <chainparams.h>
6 #include <consensus/amount.h>
7-#include <net.h>

kevkevinpal commented at 0:19 am on October 20, 2023:

was this header not needed? I don’t see #include <node/connection.h> being added

ajtowns commented at 3:05 am on October 20, 2023:

Seems like it wasn’t; iwyu doesn’t recommend adding it back, either. Shouldn’t be needed – validation stuff should be “kernel” level, so shouldn’t rely on any p2p/net things.

kevkevinpal commented at 3:29 am on October 20, 2023:

sounds good to me!

in src/node/connection.h:259 in 1116e57de9 outdated

254+
255+    /** Whether upon disconnections, a reconnect with V1 is warranted. */
256+    virtual bool ShouldReconnectV1() const noexcept = 0;
257+};
258+
259+class V1Transport final : public Transport

dergoegge commented at 9:19 am on October 20, 2023:

What do you think of moving the transport impls to net.cpp (or their own module)?. They aren’t really part of this interface, more of a implementation detail.

ajtowns commented at 11:28 am on October 20, 2023:

The class info is exposed for unit tests, and you at least need class Transport in order to know what a CNode looks like. Separating out the V1/V2 implementation details alone doesn’t seem like much of a win. (And wouldn’t really be a win at all if we just had a concrete Transport class instead of the replaceable virtual setup, cf #28331 (review))

ajtowns commented at 11:29 am on October 20, 2023:

(The Transport stuff is all specific to a single connection, so it would go in node/connection.cpp rather than net.cpp per the logic of this PR)

DrahtBot added the label Needs rebase on Oct 24, 2023

qt/guiutil: add missing include 463109541c

move-only: split CNode into new node/connection.h module

Move per-peer code from net into a new node/connection module. This
is intended to capture logic that is lower-level than the p2p protocol
(done in net_processing), but higher-level than raw bits-over-the-wire
(handled by Sock).

30c495920d

remove unnecessary net.h includes fb03ca14c8

ajtowns force-pushed on Oct 25, 2023

DrahtBot removed the label Needs rebase on Oct 25, 2023

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

naumenkogs commented at 10:38 am on October 27, 2023: member

Concept ACK. Good thing this separation doesn’t require additional wrappers and fields.

naumenkogs commented at 7:42 am on October 31, 2023: member

ACK fb03ca14c8076bc5fd3c9535c280996032db9cf6

DrahtBot added the label Needs rebase on Nov 14, 2023

DrahtBot commented at 4:21 pm on November 14, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

maflcko commented at 4:16 pm on November 28, 2023: member

Are you still working on this?

ajtowns commented at 5:26 pm on November 28, 2023: contributor

Closing as up-for-grabs (will grab it myself in a while if no one else does first)

ajtowns closed this on Nov 28, 2023

ajtowns added the label Up for grabs on Nov 28, 2023

refactor: Split per-peer parts of net module into new node/connection module #28686

Code Coverage

Reviews

Conflicts