Merge bitcoin/bitcoin#34181: refactor: [p2p] Make ProcessMessage private again, Use references when non-null

fa43897c1d doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac5415466 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a0 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada838014 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3cee test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)

Pull request description:

  There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.

  Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.

  Fix both issues in a series of commits, to:

  * refactor the single unit test to call higher-level functions
  * Make `ProcessMessage` private again
  * Use references instead of implicit non-null pointers, mostly in a scripted-diff

ACKs for top commit:
  optout21:
    reACK fa43897c1d
  ajtowns:
    ACK fa43897c1d
  Crypt-iQ:
    crACK fa43897c1d
  achow101:
    ACK fa43897c1d

Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e

src/net.cpp

@@ -3139,12 +3139,12 @@ void CConnman::ThreadMessageHandler()
                     continue;
 
                 // Receive messages
-                bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
+                bool fMoreNodeWork{m_msgproc->ProcessMessages(*pnode, flagInterruptMsgProc)};
                 fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
                 if (flagInterruptMsgProc)
                     return;
                 // Send messages
-                m_msgproc->SendMessages(pnode);
+                m_msgproc->SendMessages(*pnode);
 
                 if (flagInterruptMsgProc)
                     return;

src/net.h

@@ -1043,21 +1043,21 @@ public:
     virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0;
 
     /**
-    * Process protocol messages received from a given node
+     * Process protocol messages received from a given node
-    *
+     *
-    * @param[in]   pnode           The node which we have received messages from.
+     * @param[in]   node            The node which we have received messages from.
-    * @param[in]   interrupt       Interrupt condition for processing threads
+     * @param[in]   interrupt       Interrupt condition for processing threads
-    * @return                      True if there is more work to be done
+     * @return                      True if there is more work to be done
-    */
+     */
-    virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
+    virtual bool ProcessMessages(CNode& node, std::atomic<bool>& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
 
     /**
-    * Send queued protocol messages to a given node.
+     * Send queued protocol messages to a given node.
-    *
+     *
-    * @param[in]   pnode           The node which we are sending messages to.
+     * @param[in]   node            The node which we are sending messages to.
-    * @return                      True if there is more work to be done
+     * @return                      True if there is more work to be done
-    */
+     */
-    virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
+    virtual bool SendMessages(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
 
 
 protected:

src/net_processing.h

@@ -147,10 +147,6 @@ public:
      */
     virtual void CheckForStaleTipAndEvictPeers() = 0;
 
-    /** Process a single message from a peer. Public for fuzz testing */
-    virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
-                                std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
-
     /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
     virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
 

src/test/denialofservice_tests.cpp

@@ -81,7 +81,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     }
 
     // Test starts here
-    BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
+    BOOST_CHECK(peerman.SendMessages(dummyNode1)); // should result in getheaders
 
     {
         LOCK(dummyNode1.cs_vSend);
@@ -93,7 +93,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     int64_t nStartTime = GetTime();
     // Wait 21 minutes
     SetMockTime(nStartTime+21*60);
-    BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
+    BOOST_CHECK(peerman.SendMessages(dummyNode1)); // should result in getheaders
     {
         LOCK(dummyNode1.cs_vSend);
         const auto& [to_send, _more, _msg_type] = dummyNode1.m_transport->GetBytesToSend(false);
@@ -101,7 +101,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     }
     // Wait 3 more minutes
     SetMockTime(nStartTime+24*60);
-    BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
+    BOOST_CHECK(peerman.SendMessages(dummyNode1)); // should result in disconnect
     BOOST_CHECK(dummyNode1.fDisconnect == true);
 
     peerman.FinalizeNode(dummyNode1);
@@ -336,7 +336,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     nodes[0]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[0]);
     peerLogic->UnitTestMisbehaving(nodes[0]->GetId()); // Should be discouraged
-    BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
+    BOOST_CHECK(peerLogic->SendMessages(*nodes[0]));
 
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(nodes[0]->fDisconnect);
@@ -356,7 +356,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     peerLogic->InitializeNode(*nodes[1], NODE_NETWORK);
     nodes[1]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[1]);
-    BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
+    BOOST_CHECK(peerLogic->SendMessages(*nodes[1]));
     // [0] is still discouraged/disconnected.
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(nodes[0]->fDisconnect);
@@ -364,7 +364,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
     BOOST_CHECK(!nodes[1]->fDisconnect);
     peerLogic->UnitTestMisbehaving(nodes[1]->GetId());
-    BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
+    BOOST_CHECK(peerLogic->SendMessages(*nodes[1]));
     // Expect both [0] and [1] to be discouraged/disconnected now.
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(nodes[0]->fDisconnect);
@@ -388,7 +388,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     nodes[2]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[2]);
     peerLogic->UnitTestMisbehaving(nodes[2]->GetId());
-    BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
+    BOOST_CHECK(peerLogic->SendMessages(*nodes[2]));
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(banman->IsDiscouraged(addr[1]));
     BOOST_CHECK(banman->IsDiscouraged(addr[2]));
@@ -431,7 +431,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     dummyNode.fSuccessfullyConnected = true;
 
     peerLogic->UnitTestMisbehaving(dummyNode.GetId());
-    BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
+    BOOST_CHECK(peerLogic->SendMessages(dummyNode));
     BOOST_CHECK(banman->IsDiscouraged(addr));
 
     peerLogic->FinalizeNode(dummyNode);

src/test/fuzz/p2p_handshake.cpp

@@ -100,7 +100,7 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
                 more_work = connman.ProcessMessagesOnce(connection);
             } catch (const std::ios_base::failure&) {
             }
-            peerman->SendMessages(&connection);
+            peerman->SendMessages(connection);
         }
     }
 

src/test/fuzz/p2p_headers_presync.cpp

@@ -97,7 +97,7 @@ void HeadersSyncSetup::SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSe
         connman.ProcessMessagesOnce(connection);
     } catch (const std::ios_base::failure&) {
     }
-    m_node.peerman->SendMessages(&connection);
+    m_node.peerman->SendMessages(connection);
 }
 
 CBlockHeader ConsumeHeader(FuzzedDataProvider& fuzzed_data_provider, const uint256& prev_hash, uint32_t prev_nbits)

src/test/fuzz/process_message.cpp

@@ -123,7 +123,7 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
             more_work = connman.ProcessMessagesOnce(p2p_node);
         } catch (const std::ios_base::failure&) {
         }
-        node.peerman->SendMessages(&p2p_node);
+        node.peerman->SendMessages(p2p_node);
     }
     node.validation_signals->SyncWithValidationInterfaceQueue();
     node.connman->StopNodes();

src/test/fuzz/process_messages.cpp

@@ -122,7 +122,7 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
                 more_work = connman.ProcessMessagesOnce(random_node);
             } catch (const std::ios_base::failure&) {
             }
-            node.peerman->SendMessages(&random_node);
+            node.peerman->SendMessages(random_node);
         }
     }
     node.validation_signals->SyncWithValidationInterfaceQueue();

src/test/fuzz/util/net.h

@@ -151,9 +151,9 @@ public:
 
     virtual bool HasAllDesirableServiceFlags(ServiceFlags) const override { return m_fdp.ConsumeBool(); }
 
-    virtual bool ProcessMessages(CNode*, std::atomic<bool>&) override { return m_fdp.ConsumeBool(); }
+    virtual bool ProcessMessages(CNode&, std::atomic<bool>&) override { return m_fdp.ConsumeBool(); }
 
-    virtual bool SendMessages(CNode*) override { return m_fdp.ConsumeBool(); }
+    virtual bool SendMessages(CNode&) override { return m_fdp.ConsumeBool(); }
 
 private:
     FuzzedDataProvider& m_fdp;

src/test/net_tests.cpp

@@ -6,7 +6,6 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <compat/compat.h>
-#include <cstdint>
 #include <net.h>
 #include <net_processing.h>
 #include <netaddress.h>
@@ -16,6 +15,7 @@
 #include <serialize.h>
 #include <span.h>
 #include <streams.h>
+#include <test/util/net.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
@@ -26,6 +26,7 @@
 #include <boost/test/unit_test.hpp>
 
 #include <algorithm>
+#include <cstdint>
 #include <ios>
 #include <memory>
 #include <optional>
@@ -802,6 +803,7 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle)
 BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
 {
     LOCK(NetEventsInterface::g_msgproc_mutex);
+    auto& connman{static_cast<ConnmanTestMsg&>(*m_node.connman)};
 
     // Tests the following scenario:
     // * -bind=3.4.5.6:20001 is specified
@@ -845,22 +847,24 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
 
     m_node.peerman->InitializeNode(peer, NODE_NETWORK);
 
-    std::atomic<bool> interrupt_dummy{false};
+    m_node.peerman->SendMessages(peer);
-    std::chrono::microseconds time_received_dummy{0};
+    connman.FlushSendBuffer(peer); // Drop sent version message
 
-    const auto msg_version =
+    auto msg_version_receive =
         NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
-    DataStream msg_version_stream{msg_version.data};
+    Assert(connman.ReceiveMsgFrom(peer, std::move(msg_version_receive)));
+    peer.fPauseSend = false;
+    bool more_work{connman.ProcessMessagesOnce(peer)};
+    Assert(!more_work);
 
-    m_node.peerman->ProcessMessage(
+    m_node.peerman->SendMessages(peer);
-        peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy);
+    connman.FlushSendBuffer(peer); // Drop sent verack message
-
-    const auto msg_verack = NetMsg::Make(NetMsgType::VERACK);
-    DataStream msg_verack_stream{msg_verack.data};
 
+    Assert(connman.ReceiveMsgFrom(peer, NetMsg::Make(NetMsgType::VERACK)));
+    peer.fPauseSend = false;
     // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()).
-    m_node.peerman->ProcessMessage(
+    more_work = connman.ProcessMessagesOnce(peer);
-        peer, NetMsgType::VERACK, msg_verack_stream, time_received_dummy, interrupt_dummy);
+    Assert(!more_work);
 
     // Ensure that peer_us_addr:bind_port is sent to the peer.
     const CService expected{peer_us_addr, bind_port};
@@ -886,7 +890,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
         }
     };
 
-    m_node.peerman->SendMessages(&peer);
+    m_node.peerman->SendMessages(peer);
 
     BOOST_CHECK(sent);
 

src/test/util/net.cpp

@@ -31,7 +31,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
     auto& connman{*this};
 
     peerman.InitializeNode(node, local_services);
-    peerman.SendMessages(&node);
+    peerman.SendMessages(node);
     FlushSendBuffer(node); // Drop the version message added by SendMessages.
 
     CSerializedNetMsg msg_version{
@@ -52,7 +52,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
     (void)connman.ReceiveMsgFrom(node, std::move(msg_version));
     node.fPauseSend = false;
     connman.ProcessMessagesOnce(node);
-    peerman.SendMessages(&node);
+    peerman.SendMessages(node);
     FlushSendBuffer(node); // Drop the verack message added by SendMessages.
     if (node.fDisconnect) return;
     assert(node.nVersion == version);
@@ -66,7 +66,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
         (void)connman.ReceiveMsgFrom(node, std::move(msg_verack));
         node.fPauseSend = false;
         connman.ProcessMessagesOnce(node);
-        peerman.SendMessages(&node);
+        peerman.SendMessages(node);
         assert(node.fSuccessfullyConnected == true);
     }
 }

src/test/util/net.h

@@ -101,7 +101,7 @@ struct ConnmanTestMsg : public CConnman {
 
     bool ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex)
     {
-        return m_msgproc->ProcessMessages(&node, flagInterruptMsgProc);
+        return m_msgproc->ProcessMessages(node, flagInterruptMsgProc);
     }
 
     void NodeReceiveMsgBytes(CNode& node, std::span<const uint8_t> msg_bytes, bool& complete) const;