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

dergoegge commented at 5:43 pm on March 14, 2023: member

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 commented at 5:43 pm on March 14, 2023: contributor

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.

in src/net.h:428 in 0e769e8ba5 outdated

419+    void MarkReceivedMsgsForProcessing(unsigned int recv_flood_size)
420+        EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex);
421+
422+    /** Poll the next message from the processing queue of this connection. */
423+    std::optional<std::pair<CNetMessage, bool>> PollMessage(size_t recv_flood_size)
424+        EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex);

vasild commented at 1:41 pm on March 16, 2023:

Would be nice to extend the comment to explain what is the bool in the return value.

in src/net.cpp:2802 in 0e769e8ba5 outdated

2793@@ -2805,6 +2794,37 @@ CNode::CNode(NodeId idIn,
2794     }
2795 }
2796 
2797+void CNode::MarkReceivedMsgsForProcessing(unsigned int recv_flood_size)
2798+{
2799+    size_t nSizeAdded = 0;

vasild commented at 2:15 pm on March 16, 2023:

Add AssertLockNotHeld(m_msg_process_queue_mutex) as per doc/developer-notes.md:

Combine annotations in function declarations with run-time asserts in function definitions:

in src/net.cpp:2813 in 0e769e8ba5 outdated

2806+        LOCK(m_msg_process_queue_mutex);
2807+        m_msg_process_queue.splice(m_msg_process_queue.end(), vRecvMsg);
2808+        m_msg_process_queue_size += nSizeAdded;
2809+        fPauseRecv = m_msg_process_queue_size > recv_flood_size;
2810+    }
2811+}

vasild commented at 2:16 pm on March 16, 2023:

The nested scope is unnecessary now, you can remove { and }.

in src/net.h:427 in 0e769e8ba5 outdated

422+    /** Poll the next message from the processing queue of this connection. */
423+    std::optional<std::pair<CNetMessage, bool>> PollMessage(size_t recv_flood_size)
424+        EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex);
425+
426+    /** Account for the total size of a send message in the per msg type connection stats. */
427+    void AccountForSendBytes(const std::string& msg_type, size_t received_bytes)

vasild commented at 2:21 pm on March 16, 2023:

s/Send/Sent/ s/received_bytes/sent_bytes/

0    void AccountForSentBytes(const std::string& msg_type, size_t sent_bytes)

vasild commented at 2:26 pm on March 16, 2023: contributor

Approach ACK 0e769e8ba5c81ff7b5b0a585465091ac3ccead2c

dergoegge force-pushed on Mar 17, 2023

dergoegge commented at 10:24 am on March 17, 2023: member

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

vasild approved

vasild commented at 10:32 am on March 17, 2023: contributor

ACK d02b99b10ab6a748fd3c8b0d0faca013ada2d6e3

dergoegge force-pushed on Mar 17, 2023

vasild approved

vasild commented at 10:33 am on March 17, 2023: contributor

ACK e29ecece9fed29663cc21caff4b00ceb29ecb959

in src/net.cpp:2816 in e29ecece9f outdated

2811+    fPauseRecv = m_msg_process_queue_size > recv_flood_size;
2812+}
2813+
2814+std::optional<std::pair<CNetMessage, bool>> CNode::PollMessage(size_t recv_flood_size)
2815+{
2816+    std::list<CNetMessage> msgs;

brunoerg commented at 8:15 pm on March 17, 2023:

nit:

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 1ee783518..e8a25f9e7 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -2813,11 +2813,10 @@ void CNode::MarkReceivedMsgsForProcessing(unsigned int recv_flood_size)
 5 
 6 std::optional<std::pair<CNetMessage, bool>> CNode::PollMessage(size_t recv_flood_size)
 7 {
 8-    std::list<CNetMessage> msgs;
 9-
10     LOCK(m_msg_process_queue_mutex);
11     if (m_msg_process_queue.empty()) return std::nullopt;
12 
13+    std::list<CNetMessage> msgs;
14     // Just take one message
15     msgs.splice(msgs.begin(), m_msg_process_queue, m_msg_process_queue.begin());
16     m_msg_process_queue_size -= msgs.front().m_raw_message_size;

dergoegge commented at 1:43 pm on March 19, 2023:

Done!

brunoerg approved

brunoerg commented at 8:17 pm on March 17, 2023: contributor

crACK e29ecece9fed29663cc21caff4b00ceb29ecb959

nice cleanup!

DrahtBot added the label Needs rebase on Mar 19, 2023

[net] Add connection type getter to CNode ad44aa5c64

[net] Deduplicate marking received message for processing cc5cdf8776

dergoegge force-pushed on Mar 19, 2023

dergoegge commented at 1:43 pm on March 19, 2023: member

Rebased

DrahtBot removed the label Needs rebase on Mar 19, 2023

in src/net_processing.cpp:4893 in d816bcced0 outdated

4899+        // No message to process
4900+        return false;
4901+    }
4902+
4903+    CNetMessage& msg{poll_result->first};
4904+    fMoreWork = poll_result->second;

theStack commented at 1:01 am on March 21, 2023:

in commit d816bcced0c1afb22f97df4650a35301a25cd9de: nit: since fMoreWork is unused above, could reduce it’s scope and declare/init it right here:

0    bool fMoreWork = poll_result->second;

dergoegge commented at 2:39 pm on March 21, 2023:

Done!

theStack approved

theStack commented at 1:03 am on March 21, 2023: contributor

Code-review ACK 6514352829364ab1ccff3a58faf6ca6ef4aafbbb

DrahtBot requested review from brunoerg on Mar 21, 2023

DrahtBot requested review from vasild on Mar 21, 2023

dergoegge force-pushed on Mar 21, 2023

theStack approved

theStack commented at 3:17 pm on March 21, 2023: contributor

re-ACK 60c3f4918190900e5f79341abcc0878214656257

brunoerg approved

brunoerg commented at 5:36 pm on March 21, 2023: contributor

re-ACK 60c3f4918190900e5f79341abcc0878214656257

vasild approved

vasild commented at 4:25 am on March 22, 2023: contributor

ACK 60c3f4918190900e5f79341abcc0878214656257

[net] Encapsulate CNode message polling 897e342d6e

[net] Make CNode msg process queue members private

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

23d9352654

[net] Make cs_vProcessMsg a non-recursive mutex 6693c499f7

scripted-diff: [net] Rename CNode process queue members

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

60441a3432

[net] Add CNode helper for send byte accounting 3eac5e7cd1

[net] Remove CNode friends

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

3566aa7d49

dergoegge force-pushed on Mar 22, 2023

in src/net.cpp:2826 in 3566aa7d49

2821+    // Just take one message
2822+    msgs.splice(msgs.begin(), m_msg_process_queue, m_msg_process_queue.begin());
2823+    m_msg_process_queue_size -= msgs.front().m_raw_message_size;
2824+    fPauseRecv = m_msg_process_queue_size > recv_flood_size;
2825+
2826+    return std::make_pair(std::move(msgs.front()), !m_msg_process_queue.empty());

dergoegge commented at 12:23 pm on March 22, 2023:

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

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

glozow added the label Refactoring on Mar 23, 2023

vasild approved

vasild commented at 1:25 pm on March 23, 2023: contributor

ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

DrahtBot requested review from brunoerg on Mar 23, 2023

DrahtBot requested review from theStack on Mar 23, 2023

theStack approved

theStack commented at 1:34 pm on March 23, 2023: contributor

re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

brunoerg approved

brunoerg commented at 1:36 pm on March 23, 2023: contributor

re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

in src/net.h:416 in 3566aa7d49

409@@ -417,6 +410,30 @@ class CNode
410     std::atomic_bool fPauseRecv{false};
411     std::atomic_bool fPauseSend{false};
412 
413+    const ConnectionType& GetConnectionType() const
414+    {
415+        return m_conn_type;
416+    }

jnewbery commented at 2:35 pm on March 23, 2023:

C++ Core Guidelines suggest avoiding trivial getters: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters

Here, m_conn_type is const, so it can just be made public and read from outside the class.

in src/net.h:419 in 3566aa7d49

414+    {
415+        return m_conn_type;
416+    }
417+
418+    /** Move all messages from the received queue to the processing queue. */
419+    void MarkReceivedMsgsForProcessing(unsigned int recv_flood_size)

jnewbery commented at 2:41 pm on March 23, 2023:

(for follow up) it’d be nice to remove the recv_flood_size parameter here:

Add a const size_t m_receive_flood_size member to CNode and set it during construction.
Remove this parameter and use m_receive_flood_size for the comparison in this function.
Remove the GetReceiveFloodSize() function from CConnman.

in src/test/util/net.cpp:69 in 3566aa7d49

76-            LOCK(node.cs_vProcessMsg);
77-            node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg);
78-            node.nProcessQueueSize += nSizeAdded;
79-            node.fPauseRecv = node.nProcessQueueSize > nReceiveFloodSize;
80-        }
81+        node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);

jnewbery commented at 2:55 pm on March 23, 2023:

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

0    if (complete) node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);

vasild commented at 9:18 am on March 24, 2023:

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:

0- if (A) B;
1+ if (A) {
2+     B;
3+     C;
4+ }

vs

0 if (A) {
1     B;
2+    C;
3 }

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

jnewbery commented at 9:57 am on March 24, 2023:

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.

vasild commented at 12:54 pm on March 24, 2023:

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

jnewbery commented at 1:59 pm on March 24, 2023:

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.

jnewbery commented at 4:22 pm on March 23, 2023: contributor

utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

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.

fanquake merged this on Mar 23, 2023

fanquake closed this on Mar 23, 2023

sidhujag referenced this in commit 707a3ecbc8 on Mar 24, 2023

fanquake referenced this in commit d254f942a5 on Mar 28, 2023

sidhujag referenced this in commit 1c07f8ff47 on Mar 28, 2023

bitcoin locked this on Mar 23, 2024

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

Reviews

Conflicts