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

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

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:

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

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

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

nit:

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

Done!

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

    bool fMoreWork = poll_result->second;

Done!

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

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

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.

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

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.