Skip to content

Commit bf12abe

Browse files
ajtownsajtowns
committed
net: drop cs_sendProcessing
SendMessages() is now protected g_msgproc_mutex; so this additional per-node mutex is redundant.
1 parent 1e78f56 commit bf12abe

File tree

8 files changed

+18
-60
lines changed

8 files changed

+18
-60
lines changed

‎src/net.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2005,10 +2005,7 @@ void CConnman::ThreadMessageHandler()
20052005
if (flagInterruptMsgProc)
20062006
return;
20072007
// Send messages
2008-
{
2009-
LOCK(pnode->cs_sendProcessing);
2010-
m_msgproc->SendMessages(pnode);
2011-
}
2008+
m_msgproc->SendMessages(pnode);
20122009

20132010
if (flagInterruptMsgProc)
20142011
return;

‎src/net.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,6 @@ class CNode
377377
std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
378378
size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0};
379379

380-
RecursiveMutex cs_sendProcessing;
381-
382380
uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};
383381

384382
std::atomic<std::chrono::seconds> m_last_send{0s};
@@ -653,7 +651,7 @@ class NetEventsInterface
653651
* @param[in] pnode The node which we are sending messages to.
654652
* @return True if there is more work to be done
655653
*/
656-
virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_sendProcessing) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
654+
virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
657655

658656

659657
protected:

‎src/net_processing.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,7 @@ class PeerManagerImpl final : public PeerManager
516516
void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);
517517
bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
518518
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
519-
bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing)
519+
bool SendMessages(CNode* pto) override
520520
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex);
521521

522522
/** Implement PeerManager */

‎src/test/denialofservice_tests.cpp

Lines changed: 10 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
8282
}
8383

8484
// Test starts here
85-
{
86-
LOCK(dummyNode1.cs_sendProcessing);
87-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
88-
}
85+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
86+
8987
{
9088
LOCK(dummyNode1.cs_vSend);
9189
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
@@ -95,20 +93,14 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
9593
int64_t nStartTime = GetTime();
9694
// Wait 21 minutes
9795
SetMockTime(nStartTime+21*60);
98-
{
99-
LOCK(dummyNode1.cs_sendProcessing);
100-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
101-
}
96+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
10297
{
10398
LOCK(dummyNode1.cs_vSend);
10499
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
105100
}
106101
// Wait 3 more minutes
107102
SetMockTime(nStartTime+24*60);
108-
{
109-
LOCK(dummyNode1.cs_sendProcessing);
110-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
111-
}
103+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
112104
BOOST_CHECK(dummyNode1.fDisconnect == true);
113105

114106
peerman.FinalizeNode(dummyNode1);
@@ -312,10 +304,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
312304
nodes[0]->fSuccessfullyConnected = true;
313305
connman->AddTestNode(*nodes[0]);
314306
peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
315-
{
316-
LOCK(nodes[0]->cs_sendProcessing);
317-
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
318-
}
307+
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
308+
319309
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
320310
BOOST_CHECK(nodes[0]->fDisconnect);
321311
BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
@@ -334,21 +324,15 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
334324
nodes[1]->fSuccessfullyConnected = true;
335325
connman->AddTestNode(*nodes[1]);
336326
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
337-
{
338-
LOCK(nodes[1]->cs_sendProcessing);
339-
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
340-
}
327+
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
341328
// [0] is still discouraged/disconnected.
342329
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
343330
BOOST_CHECK(nodes[0]->fDisconnect);
344331
// [1] is not discouraged/disconnected yet.
345332
BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
346333
BOOST_CHECK(!nodes[1]->fDisconnect);
347334
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), 1); // [1] reaches discouragement threshold
348-
{
349-
LOCK(nodes[1]->cs_sendProcessing);
350-
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
351-
}
335+
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
352336
// Expect both [0] and [1] to be discouraged/disconnected now.
353337
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
354338
BOOST_CHECK(nodes[0]->fDisconnect);
@@ -371,10 +355,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
371355
nodes[2]->fSuccessfullyConnected = true;
372356
connman->AddTestNode(*nodes[2]);
373357
peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD);
374-
{
375-
LOCK(nodes[2]->cs_sendProcessing);
376-
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
377-
}
358+
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
378359
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
379360
BOOST_CHECK(banman->IsDiscouraged(addr[1]));
380361
BOOST_CHECK(banman->IsDiscouraged(addr[2]));
@@ -417,10 +398,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
417398
dummyNode.fSuccessfullyConnected = true;
418399

419400
peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
420-
{
421-
LOCK(dummyNode.cs_sendProcessing);
422-
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
423-
}
401+
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
424402
BOOST_CHECK(banman->IsDiscouraged(addr));
425403

426404
peerLogic->FinalizeNode(dummyNode);

‎src/test/fuzz/process_message.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,7 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
9494
GetTime<std::chrono::microseconds>(), std::atomic<bool>{false});
9595
} catch (const std::ios_base::failure&) {
9696
}
97-
{
98-
LOCK(p2p_node.cs_sendProcessing);
99-
g_setup->m_node.peerman->SendMessages(&p2p_node);
100-
}
97+
g_setup->m_node.peerman->SendMessages(&p2p_node);
10198
SyncWithValidationInterfaceQueue();
10299
g_setup->m_node.connman->StopNodes();
103100
}

‎src/test/fuzz/process_messages.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
7272
connman.ProcessMessagesOnce(random_node);
7373
} catch (const std::ios_base::failure&) {
7474
}
75-
{
76-
LOCK(random_node.cs_sendProcessing);
77-
g_setup->m_node.peerman->SendMessages(&random_node);
78-
}
75+
g_setup->m_node.peerman->SendMessages(&random_node);
7976
}
8077
SyncWithValidationInterfaceQueue();
8178
g_setup->m_node.connman->StopNodes();

‎src/test/net_tests.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -891,10 +891,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
891891
}
892892
};
893893

894-
{
895-
LOCK(peer.cs_sendProcessing);
896-
m_node.peerman->SendMessages(&peer);
897-
}
894+
m_node.peerman->SendMessages(&peer);
898895

899896
BOOST_CHECK(sent);
900897

‎src/test/util/net.cpp

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
4444
(void)connman.ReceiveMsgFrom(node, msg_version);
4545
node.fPauseSend = false;
4646
connman.ProcessMessagesOnce(node);
47-
{
48-
LOCK(node.cs_sendProcessing);
49-
peerman.SendMessages(&node);
50-
}
47+
peerman.SendMessages(&node);
5148
if (node.fDisconnect) return;
5249
assert(node.nVersion == version);
5350
assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
@@ -60,10 +57,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
6057
(void)connman.ReceiveMsgFrom(node, msg_verack);
6158
node.fPauseSend = false;
6259
connman.ProcessMessagesOnce(node);
63-
{
64-
LOCK(node.cs_sendProcessing);
65-
peerman.SendMessages(&node);
66-
}
60+
peerman.SendMessages(&node);
6761
assert(node.fSuccessfullyConnected == true);
6862
}
6963
}

0 commit comments

Comments
 (0)