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

ajtowns · 2023-10-19T10:23:42Z

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 · 2023-10-19T10:23:45Z

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.

ajtowns · 2023-10-19T10:31:04Z

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).

maflcko · 2023-10-19T11:37:22Z

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 · 2023-10-19T11:52:01Z

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 · 2023-10-19T12:22:12Z

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 · 2023-10-19T13:14:05Z

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.

src/test/validation_tests.cpp

dergoegge · 2023-10-20T09:19:40Z

src/node/connection.h

+    virtual bool ShouldReconnectV1() const noexcept = 0;
+};
+
+class V1Transport final : public Transport


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.

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 (comment))

(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)

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).

naumenkogs · 2023-10-27T10:38:41Z

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

naumenkogs · 2023-10-31T07:42:46Z

ACK fb03ca1

DrahtBot · 2023-11-14T16:21:40Z

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

maflcko · 2023-11-28T16:16:31Z

Are you still working on this?

ajtowns · 2023-11-28T17:26:33Z

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

DrahtBot added the Refactoring label Oct 19, 2023

ajtowns force-pushed the 202310-nodenode branch from 9cfa7d8 to 468b2fb Compare October 19, 2023 10:32

This was referenced Oct 19, 2023

txorphanage: add size accounting, use wtxids, support multiple announcers #28481

Closed

refactor: share and use GenerateRandomKey helper #28455

Merged

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

Merged

ajtowns force-pushed the 202310-nodenode branch from 468b2fb to 1116e57 Compare October 19, 2023 13:13

ajtowns changed the title ~~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 Oct 19, 2023

kevkevinpal reviewed Oct 20, 2023

View reviewed changes

src/test/validation_tests.cpp Show resolved Hide resolved

dergoegge reviewed Oct 20, 2023

View reviewed changes

This was referenced Oct 20, 2023

build: Introduce internal kernel library #28690

Open

Multiprocess bitcoin #10102

Draft

DrahtBot added the Needs rebase label Oct 24, 2023

ajtowns added 3 commits October 25, 2023 17:10

qt/guiutil: add missing include

4631095

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

30c4959

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).

remove unnecessary net.h includes

fb03ca1

ajtowns force-pushed the 202310-nodenode branch from 1116e57 to fb03ca1 Compare October 25, 2023 07:21

DrahtBot added CI failed and removed Needs rebase CI failed labels Oct 25, 2023

ajtowns mentioned this pull request Oct 30, 2023

refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC #28503

Merged

DrahtBot mentioned this pull request Nov 13, 2023

test, refactor: Magic bytes array followup #28857

Merged

DrahtBot added the Needs rebase label Nov 14, 2023

ajtowns closed this Nov 28, 2023

ajtowns added the Up for grabs label Nov 28, 2023

bitcoin locked and limited conversation to collaborators Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ajtowns commented Oct 19, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Oct 19, 2023 •

edited

Loading

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

maflcko commented Oct 19, 2023

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

dergoegge commented Oct 19, 2023

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

Uh oh!

dergoegge Oct 20, 2023

Uh oh!

ajtowns Oct 20, 2023

Uh oh!

ajtowns Oct 20, 2023

Uh oh!

naumenkogs commented Oct 27, 2023

Uh oh!

naumenkogs commented Oct 31, 2023

Uh oh!

DrahtBot commented Nov 14, 2023

Uh oh!

maflcko commented Nov 28, 2023

Uh oh!

ajtowns commented Nov 28, 2023

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

ajtowns commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

maflcko commented Oct 19, 2023

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

dergoegge commented Oct 19, 2023

Uh oh!

ajtowns commented Oct 19, 2023

Uh oh!

Uh oh!

dergoegge Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Oct 27, 2023

Uh oh!

naumenkogs commented Oct 31, 2023

Uh oh!

DrahtBot commented Nov 14, 2023

Uh oh!

maflcko commented Nov 28, 2023

Uh oh!

ajtowns commented Nov 28, 2023

Uh oh!

Uh oh!

ajtowns commented Oct 19, 2023 •

edited

Loading

DrahtBot commented Oct 19, 2023 •

edited

Loading