refactor: Pass CNode& to ProcessMessages and SendMessages
What changed, and why it matters
This commit is a straightforward code cleanup: it changes two internal networking functions so they receive a node object directly instead of via a pointer. The author states the pointer was never null, so this just makes the code clearer and safer in a general sense, but it does not fix any known bug or vulnerability.
No security action required; treat as normal code-quality refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch refactors PeerManagerImpl::ProcessMessages and PeerManagerImpl::SendMessages (plus the NetEventsInterface virtual declarations and all call sites) to take CNode& references rather than CNode pointers. Inside the implementations a temporary CNode alias is kept with a comment noting it is removed later. No logic, null checks, locking, or message handling behavior is changed. The commit message explicitly calls this a refactor and says ‘The node is never nullptr.’
Changed components
src/net.cppsrc/net.hsrc/net_processing.cpptest code calling ProcessMessages/SendMessagesInspect captured patch +42 / −40
diff --git a/src/net.cpp b/src/net.cpp
index 6b79e913..646fe9e9 100644
--- a/src/net.cpp
+++ b/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;
diff --git a/src/net.h b/src/net.h
index 1e4e9124..1f7bbe4a 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1043,21 +1043,21 @@ public:
virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0;
/**
- * Process protocol messages received from a given node
- *
- * @param[in] pnode The node which we have received messages from.
- * @param[in] interrupt Interrupt condition for processing threads
- * @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;
+ * Process protocol messages received from a given node
+ *
+ * @param[in] node The node which we have received messages from.
+ * @param[in] interrupt Interrupt condition for processing threads
+ * @return True if there is more work to be done
+ */
+ virtual bool ProcessMessages(CNode& node, std::atomic<bool>& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
/**
- * Send queued protocol messages to a given node.
- *
- * @param[in] pnode The node which we are sending messages to.
- * @return True if there is more work to be done
- */
- virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
+ * Send queued protocol messages to a given node.
+ *
+ * @param[in] node The node which we are sending messages to.
+ * @return True if there is more work to be done
+ */
+ virtual bool SendMessages(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
protected:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 34425948..3f8c2217 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -529,9 +529,9 @@ public:
void InitializeNode(const CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_tx_download_mutex);
void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex, !m_tx_download_mutex);
bool HasAllDesirableServiceFlags(ServiceFlags services) const override;
- bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
+ bool ProcessMessages(CNode& node, std::atomic<bool>& interrupt) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex, !m_tx_download_mutex);
- bool SendMessages(CNode* pto) override
+ bool SendMessages(CNode& node) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, g_msgproc_mutex, !m_tx_download_mutex);
/** Implement PeerManager */
@@ -5181,11 +5181,12 @@ bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer)
return true;
}
-bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgProc)
+bool PeerManagerImpl::ProcessMessages(CNode& node, std::atomic<bool>& interruptMsgProc)
{
AssertLockNotHeld(m_tx_download_mutex);
AssertLockHeld(g_msgproc_mutex);
+ CNode* pfrom{&node}; // alias removed in a later commit.
PeerRef peer = GetPeerRef(pfrom->GetId());
if (peer == nullptr) return false;
@@ -5683,11 +5684,12 @@ bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer)
return true;
}
-bool PeerManagerImpl::SendMessages(CNode* pto)
+bool PeerManagerImpl::SendMessages(CNode& node)
{
AssertLockNotHeld(m_tx_download_mutex);
AssertLockHeld(g_msgproc_mutex);
+ CNode* pto{&node}; // alias removed in a later commit
PeerRef peer = GetPeerRef(pto->GetId());
if (!peer) return false;
const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 0dc03230..776a1878 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/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);
diff --git a/src/test/fuzz/p2p_handshake.cpp b/src/test/fuzz/p2p_handshake.cpp
index aec7387e..bf9b7445 100644
--- a/src/test/fuzz/p2p_handshake.cpp
+++ b/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);
}
}
diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp
index d5871374..4aa56686 100644
--- a/src/test/fuzz/p2p_headers_presync.cpp
+++ b/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)
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index ef8cb686..c03dfbd6 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -121,7 +121,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();
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index f36f528b..8997dbac 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -120,7 +120,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();
diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h
index 90c018a4..d19c4cbf 100644
--- a/src/test/fuzz/util/net.h
+++ b/src/test/fuzz/util/net.h
@@ -150,9 +150,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;
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index ff2083de..72a06a68 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -847,7 +847,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
m_node.peerman->InitializeNode(peer, NODE_NETWORK);
- m_node.peerman->SendMessages(&peer);
+ m_node.peerman->SendMessages(peer);
connman.FlushSendBuffer(peer); // Drop sent version message
auto msg_version_receive =
@@ -857,7 +857,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
bool more_work{connman.ProcessMessagesOnce(peer)};
Assert(!more_work);
- m_node.peerman->SendMessages(&peer);
+ m_node.peerman->SendMessages(peer);
connman.FlushSendBuffer(peer); // Drop sent verack message
Assert(connman.ReceiveMsgFrom(peer, NetMsg::Make(NetMsgType::VERACK)));
@@ -890,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);
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index 0f59e0d6..0979e2ae 100644
--- a/src/test/util/net.cpp
+++ b/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);
}
}
diff --git a/src/test/util/net.h b/src/test/util/net.h
index ee02d404..732aefb3 100644
--- a/src/test/util/net.h
+++ b/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;
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.