refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg #27257

dergoegge · 2023-03-14T17:43:01Z

We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex.

DrahtBot · 2023-03-14T17:43:05Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, theStack, brunoerg, jnewbery

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:

#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)

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.

vasild

Approach ACK 0e769e8

src/net.h

src/net.cpp

src/net.h

dergoegge · 2023-03-17T10:24:37Z

@vasild Thanks for the review! Took all of your suggestions.

vasild

ACK d02b99b

vasild

ACK e29ecec

src/net.cpp

brunoerg

crACK e29ecec

nice cleanup!

dergoegge · 2023-03-19T13:43:50Z

Rebased

theStack

Code-review ACK 6514352

src/net_processing.cpp

theStack

re-ACK 60c3f49

brunoerg

re-ACK 60c3f49

vasild

ACK 60c3f49

Now that all access to the process queue members is handled by methods of `CNode` we can make these members private.

-BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren cs_vProcessMsg m_msg_process_queue_mutex ren vProcessMsg m_msg_process_queue ren nProcessQueueSize m_msg_process_queue_size -END VERIFY SCRIPT-

Both `CConnman` and `ConnmanTestMsg` no longer access private members of `CNode`, we can therefore remove the friend relationship.

dergoegge · 2023-03-22T12:23:44Z

src/net.cpp

+    m_msg_process_queue_size -= msgs.front().m_raw_message_size;
+    fPauseRecv = m_msg_process_queue_size > recv_flood_size;
+
+    return std::make_pair(std::move(msgs.front()), !m_msg_process_queue.empty());


I was making a copy here of the CNetMessage, fixed now!

Easy to review with git range-diff 60c3f4918190900e5f79341abcc0878214656257...3566aa7d495bb783bbd135726238d9f2a9e9f80e.

vasild

ACK 3566aa7

theStack

re-ACK 3566aa7

brunoerg

re-ACK 3566aa7

jnewbery · 2023-03-23T16:22:49Z

utACK 3566aa7

I didn't verify myself that the new code doesn't result in a copy of CNetMessage, but I trust that you've tested that. A follow up PR could potentially delete the default copy constructor for CNetMessage to ensure that we never copy.

All of my review comments are nits/style comments and shouldn't stop this from being merged.

src/net.h

jnewbery · 2023-03-23T14:55:55Z

src/test/util/net.cpp

-            node.nProcessQueueSize += nSizeAdded;
-            node.fPauseRecv = node.nProcessQueueSize > nReceiveFloodSize;
-        }
+        node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);


Style nit: feel free to use a single line for single-line if blocks:

if (complete) node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);

That is a matter of personal taste (since both are allowed), but I just want to mention here why I, personally, avoid that:

It is not gdb friendly - you can't set a breakpoint on the if body (yes, one can set a breakpoint on the function call, but it becomes tedious if that function is called a lot from other places)

Creates bigger diff during changes in the future, comapre:

- if (A) B; + if (A) { + B; + C; + }

vs

if (A) { B; + C; }

The latter is easier to read, especially if A, B and C are complex expressions.

I agree that it's personal taste and the project style guide allows either.

It is not gdb friendly - you can't set a breakpoint on the if body (yes, one can set a breakpoint on the function call, but it becomes tedious if that function is called a lot from other places)

Are you familiar with conditional breakpoints? You can set the breakpoint to only stop the program if a boolean condition evaluates to true. Here, I think you could add the breakpoint:

break "net.cpp":68 if complete

and it would only stop if the if block is going to be called.

Alas that does not work if the condition includes function calls.

Alas that does not work if the condition includes function calls.

Why not? From the GDB docs:

Break conditions can have side effects, and may even call functions in your program.

… and Co…

cd0c8ee [net] Pass nRecvFloodSize to CNode (dergoegge) 860402e [net] Remove trivial GetConnectionType() getter (dergoegge) b5a85b3 [net] Delete CNetMessage copy constructor/assignment op (dergoegge) Pull request description: Follow-up PR for #27257 * Deletes the copy constructor/assignment operator of `CNetMessage` * Removes trivial getter for the connection type * Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation ACKs for top commit: jnewbery: utACK cd0c8ee theStack: ACK cd0c8ee Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41

cd0c8ee [net] Pass nRecvFloodSize to CNode (dergoegge) 860402e [net] Remove trivial GetConnectionType() getter (dergoegge) b5a85b3 [net] Delete CNetMessage copy constructor/assignment op (dergoegge) Pull request description: Follow-up PR for bitcoin#27257 * Deletes the copy constructor/assignment operator of `CNetMessage` * Removes trivial getter for the connection type * Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation ACKs for top commit: jnewbery: utACK cd0c8ee theStack: ACK cd0c8ee Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41

dergoegge mentioned this pull request Mar 14, 2023

net, test: Virtualise CConnman and add initial PeerManager unit tests #25515

Closed

vasild reviewed Mar 16, 2023

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

dergoegge force-pushed the 2023-03-cnode-friends branch from 0e769e8 to d02b99b Compare March 17, 2023 10:23

vasild approved these changes Mar 17, 2023

View reviewed changes

dergoegge force-pushed the 2023-03-cnode-friends branch from d02b99b to e29ecec Compare March 17, 2023 10:32

vasild approved these changes Mar 17, 2023

View reviewed changes

brunoerg reviewed Mar 17, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

brunoerg approved these changes Mar 17, 2023

View reviewed changes

DrahtBot added the Needs rebase label Mar 19, 2023

dergoegge added 2 commits March 19, 2023 14:34

[net] Add connection type getter to CNode

ad44aa5

[net] Deduplicate marking received message for processing

cc5cdf8

dergoegge force-pushed the 2023-03-cnode-friends branch from e29ecec to 6514352 Compare March 19, 2023 13:42

DrahtBot removed the Needs rebase label Mar 19, 2023

theStack approved these changes Mar 21, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from brunoerg and vasild March 21, 2023 01:03

dergoegge force-pushed the 2023-03-cnode-friends branch from 6514352 to 60c3f49 Compare March 21, 2023 14:38

theStack approved these changes Mar 21, 2023

View reviewed changes

brunoerg approved these changes Mar 21, 2023

View reviewed changes

DrahtBot mentioned this pull request Mar 21, 2023

BIP324: Enable v2 P2P encrypted transport #24545

Closed

vasild approved these changes Mar 22, 2023

View reviewed changes

dergoegge added 2 commits March 22, 2023 13:18

[net] Encapsulate CNode message polling

897e342

[net] Make CNode msg process queue members private

23d9352

Now that all access to the process queue members is handled by methods of `CNode` we can make these members private.

dergoegge added 4 commits March 22, 2023 13:18

[net] Make cs_vProcessMsg a non-recursive mutex

6693c49

[net] Add CNode helper for send byte accounting

3eac5e7

[net] Remove CNode friends

3566aa7

Both `CConnman` and `ConnmanTestMsg` no longer access private members of `CNode`, we can therefore remove the friend relationship.

dergoegge force-pushed the 2023-03-cnode-friends branch from 60c3f49 to 3566aa7 Compare March 22, 2023 12:21

dergoegge commented Mar 22, 2023

View reviewed changes

glozow added the Refactoring label Mar 23, 2023

vasild approved these changes Mar 23, 2023

View reviewed changes

DrahtBot requested review from brunoerg and theStack March 23, 2023 13:25

theStack approved these changes Mar 23, 2023

View reviewed changes

brunoerg approved these changes Mar 23, 2023

View reviewed changes

jnewbery reviewed Mar 23, 2023

View reviewed changes

fanquake merged commit 2305643 into bitcoin:master Mar 23, 2023

dergoegge mentioned this pull request Mar 24, 2023

net: #27257 follow-ups #27324

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 24, 2023

Merge bitcoin#27257: refactor, net: End friendship of CNode, CConnman…

707a3ec

… and Co…

This was referenced Mar 24, 2023

p2p: Allow whitelisting manual connections #27114

Merged

refactor: Continue moving application data from CNode to Peer #26621

Closed

dergoegge mentioned this pull request Apr 20, 2023

meta: Isolated fuzzing of net processing #27502

Open

11 tasks

This was referenced Apr 26, 2023

rpc: getnetmsgstats revision IV satsie/bitcoin#5

Closed

rpc: add 'getnetmsgstats', new rpc to view network message statistics #27534

Closed

willcl-ark mentioned this pull request Nov 22, 2023

rpc: add 'getnetmsgstats' RPC #28926

Closed

bitcoin locked and limited conversation to collaborators Mar 23, 2024

refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg #27257

refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg #27257

Uh oh!

Conversation

dergoegge commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Mar 17, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Mar 19, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 23, 2023

Uh oh!

Uh oh!

Uh oh!

jnewbery Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

vasild Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

vasild Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dergoegge commented Mar 14, 2023 •

edited

Loading

DrahtBot commented Mar 14, 2023 •

edited

Loading