Skip to content

Commit d4bcd25

Browse files
theuniFuzzbawls
theuni
authored and
Fuzzbawls
committed
net: push only raw data into CConnman
This fixes one of the last major layer violations in the networking stack. The network side is no longer in charge of message serialization, so it is now decoupled from Bitcoin structures. Only the header is serialized and attached to the payload.
1 parent b79e416 commit d4bcd25

File tree

8 files changed

+110
-129
lines changed

8 files changed

+110
-129
lines changed

‎src/main.cpp‎

Lines changed: 60 additions & 53 deletions
Large diffs are not rendered by default.

‎src/masternode-budget.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "masternode-sync.h"
1414
#include "masternode.h"
1515
#include "masternodeman.h"
16+
#include "netmessagemaker.h"
1617
#include "util.h"
1718

1819

@@ -1340,6 +1341,7 @@ void CBudgetManager::Sync(CNode* pfrom, const uint256& nProp, bool fPartial)
13401341
13411342
*/
13421343

1344+
CNetMsgMaker msgMaker(pfrom->GetSendVersion());
13431345
int nInvCount = 0;
13441346

13451347
std::map<uint256, CBudgetProposalBroadcast>::iterator it1 = mapSeenMasternodeBudgetProposals.begin();
@@ -1364,7 +1366,7 @@ void CBudgetManager::Sync(CNode* pfrom, const uint256& nProp, bool fPartial)
13641366
++it1;
13651367
}
13661368

1367-
g_connman->PushMessage(pfrom, NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_BUDGET_PROP, nInvCount);
1369+
g_connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_BUDGET_PROP, nInvCount));
13681370

13691371
LogPrint(BCLog::MNBUDGET, "CBudgetManager::Sync - sent %d items\n", nInvCount);
13701372

@@ -1392,7 +1394,7 @@ void CBudgetManager::Sync(CNode* pfrom, const uint256& nProp, bool fPartial)
13921394
++it3;
13931395
}
13941396

1395-
g_connman->PushMessage(pfrom, NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_BUDGET_FIN, nInvCount);
1397+
g_connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_BUDGET_FIN, nInvCount));
13961398
LogPrint(BCLog::MNBUDGET, "CBudgetManager::Sync - sent %d items\n", nInvCount);
13971399
}
13981400

@@ -1410,7 +1412,7 @@ bool CBudgetManager::UpdateProposal(CBudgetVote& vote, CNode* pfrom, std::string
14101412
mapOrphanMasternodeBudgetVotes[vote.nProposalHash] = vote;
14111413

14121414
if (!askedForSourceProposalOrBudget.count(vote.nProposalHash)) {
1413-
g_connman->PushMessage(pfrom, NetMsgType::BUDGETVOTESYNC, vote.nProposalHash);
1415+
g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::BUDGETVOTESYNC, vote.nProposalHash));
14141416
askedForSourceProposalOrBudget[vote.nProposalHash] = GetTime();
14151417
}
14161418
}
@@ -1437,7 +1439,7 @@ bool CBudgetManager::UpdateFinalizedBudget(CFinalizedBudgetVote& vote, CNode* pf
14371439
mapOrphanFinalizedBudgetVotes[vote.nBudgetHash] = vote;
14381440

14391441
if (!askedForSourceProposalOrBudget.count(vote.nBudgetHash)) {
1440-
g_connman->PushMessage(pfrom, NetMsgType::BUDGETVOTESYNC, vote.nBudgetHash);
1442+
g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::BUDGETVOTESYNC, vote.nBudgetHash));
14411443
askedForSourceProposalOrBudget[vote.nBudgetHash] = GetTime();
14421444
}
14431445
}

‎src/masternode-payments.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "masternode-budget.h"
1111
#include "masternode-sync.h"
1212
#include "masternodeman.h"
13+
#include "netmessagemaker.h"
1314
#include "spork.h"
1415
#include "sync.h"
1516
#include "util.h"
@@ -784,7 +785,7 @@ void CMasternodePayments::Sync(CNode* node, int nCountNeeded)
784785
}
785786
++it;
786787
}
787-
g_connman->PushMessage(node, NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_MNW, nInvCount);
788+
g_connman->PushMessage(node, CNetMsgMaker(node->GetSendVersion()).Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_MNW, nInvCount));
788789
}
789790

790791
std::string CMasternodePayments::ToString() const

‎src/masternode-sync.cpp‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "masternode-budget.h"
1212
#include "masternode.h"
1313
#include "masternodeman.h"
14+
#include "netmessagemaker.h"
1415
#include "spork.h"
1516
#include "util.h"
1617
#include "addrman.h"
@@ -291,14 +292,15 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool isRegTestNet)
291292
{
292293
if (isRegTestNet) {
293294
if (RequestedMasternodeAttempt <= 2) {
294-
g_connman->PushMessageWithVersion(pnode, INIT_PROTO_VERSION, NetMsgType::GETSPORKS); //get current network sporks
295+
g_connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::GETSPORKS)); //get current network sporks
295296
} else if (RequestedMasternodeAttempt < 4) {
296297
mnodeman.DsegUpdate(pnode);
297298
} else if (RequestedMasternodeAttempt < 6) {
298299
int nMnCount = mnodeman.CountEnabled();
299-
g_connman->PushMessage(pnode, NetMsgType::GETMNWINNERS, nMnCount); //sync payees
300+
CNetMsgMaker msgMaker(pnode->GetSendVersion());
301+
g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETMNWINNERS, nMnCount)); //sync payees
300302
uint256 n;
301-
g_connman->PushMessage(pnode, NetMsgType::BUDGETVOTESYNC, n); //sync masternode votes
303+
g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::BUDGETVOTESYNC, n)); //sync masternode votes
302304
} else {
303305
RequestedMasternodeAssets = MASTERNODE_SYNC_FINISHED;
304306
}
@@ -311,12 +313,15 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool isRegTestNet)
311313
if (pnode->HasFulfilledRequest("getspork")) return true;
312314
pnode->FulfilledRequest("getspork");
313315

314-
g_connman->PushMessageWithVersion(pnode, INIT_PROTO_VERSION, NetMsgType::GETSPORKS); //get current network sporks
316+
g_connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::GETSPORKS)); //get current network sporks
315317
if (RequestedMasternodeAttempt >= 2) GetNextAsset();
316318
RequestedMasternodeAttempt++;
317319
return false;
318320
}
319321

322+
// At this point, we know that the handshake has finished, so set the message version
323+
CNetMsgMaker msgMaker(pnode->GetSendVersion());
324+
320325
if (pnode->nVersion >= masternodePayments.GetMinMasternodePaymentsProto()) {
321326
if (RequestedMasternodeAssets == MASTERNODE_SYNC_LIST) {
322327
LogPrint(BCLog::MASTERNODE, "CMasternodeSync::Process() - lastMasternodeList %lld (GetTime() - MASTERNODE_SYNC_TIMEOUT) %lld\n", lastMasternodeList, GetTime() - MASTERNODE_SYNC_TIMEOUT);
@@ -377,7 +382,7 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool isRegTestNet)
377382
if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3) return false;
378383

379384
int nMnCount = mnodeman.CountEnabled();
380-
g_connman->PushMessage(pnode, NetMsgType::GETMNWINNERS, nMnCount); //sync payees
385+
g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETMNWINNERS, nMnCount)); //sync payees
381386
RequestedMasternodeAttempt++;
382387
return false;
383388
}
@@ -410,7 +415,7 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool isRegTestNet)
410415
if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3) return false;
411416

412417
uint256 n;
413-
g_connman->PushMessage(pnode, NetMsgType::BUDGETVOTESYNC, n); //sync masternode votes
418+
g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::BUDGETVOTESYNC, n)); //sync masternode votes
414419
RequestedMasternodeAttempt++;
415420
return false;
416421
}

‎src/masternodeman.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "masternode.h"
1313
#include "messagesigner.h"
1414
#include "netbase.h"
15+
#include "netmessagemaker.h"
1516
#include "spork.h"
1617
#include "swifttx.h"
1718
#include "util.h"
@@ -232,7 +233,7 @@ void CMasternodeMan::AskForMN(CNode* pnode, CTxIn& vin)
232233
// ask for the mnb info once from the node that sent mnp
233234

234235
LogPrint(BCLog::MASTERNODE, "CMasternodeMan::AskForMN - Asking node for missing entry, vin: %s\n", vin.prevout.hash.ToString());
235-
g_connman->PushMessage(pnode, NetMsgType::GETMNLIST, vin);
236+
g_connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETMNLIST, vin));
236237
int64_t askAgain = GetTime() + MASTERNODE_MIN_MNP_SECONDS;
237238
mWeAskedForMasternodeListEntry[vin.prevout] = askAgain;
238239
}
@@ -437,7 +438,7 @@ void CMasternodeMan::DsegUpdate(CNode* pnode)
437438
}
438439
}
439440

440-
g_connman->PushMessage(pnode, NetMsgType::GETMNLIST, CTxIn());
441+
g_connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETMNLIST, CTxIn()));
441442
int64_t askAgain = GetTime() + MASTERNODES_DSEG_SECONDS;
442443
mWeAskedForMasternodeList[pnode->addr] = askAgain;
443444
}
@@ -810,7 +811,7 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData
810811
}
811812

812813
if (vin == CTxIn()) {
813-
g_connman->PushMessage(pfrom, NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_LIST, nInvCount);
814+
g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_LIST, nInvCount));
814815
LogPrint(BCLog::MASTERNODE, "dseg - Sent %d Masternode entries to peer %i\n", nInvCount, pfrom->GetId());
815816
}
816817
}

‎src/net.cpp‎

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "hash.h"
2121
#include "main.h"
2222
#include "miner.h"
23+
#include "netmessagemaker.h"
2324
#include "primitives/transaction.h"
2425
#include "netbase.h"
2526
#include "scheduler.h"
@@ -470,7 +471,7 @@ bool CNode::DisconnectOldProtocol(int nVersionRequired, std::string strLastComma
470471
fDisconnect = false;
471472
if (nVersion < nVersionRequired) {
472473
LogPrintf("%s : peer=%d using obsolete version %i; disconnecting\n", __func__, id, nVersion);
473-
g_connman->PushMessageWithVersion(this, INIT_PROTO_VERSION, NetMsgType::REJECT, strLastCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", ActiveProtocol()));
474+
g_connman->PushMessage(this, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strLastCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", ActiveProtocol())));
474475
fDisconnect = true;
475476
}
476477

@@ -793,13 +794,13 @@ int CNetMessage::readData(const char* pch, unsigned int nBytes)
793794
// requires LOCK(cs_vSend)
794795
size_t SocketSendData(CNode* pnode)
795796
{
796-
std::deque<CSerializeData>::iterator it = pnode->vSendMsg.begin();
797+
auto it = pnode->vSendMsg.begin();
797798
size_t nSentSize = 0;
798799

799800
while (it != pnode->vSendMsg.end()) {
800-
const CSerializeData& data = *it;
801+
const auto& data = *it;
801802
assert(data.size() > pnode->nSendOffset);
802-
int nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
803+
int nBytes = send(pnode->hSocket, reinterpret_cast<const char*>(data.data()) + pnode->nSendOffset, data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
803804
if (nBytes > 0) {
804805
pnode->nLastSend = GetTime();
805806
pnode->nSendBytes += nBytes;
@@ -2364,7 +2365,7 @@ void CConnman::RelayTransactionLockReq(const CTransaction& tx, bool relayToAll)
23642365
if (!relayToAll && !pnode->fRelayTxes)
23652366
continue;
23662367

2367-
g_connman->PushMessage(pnode, NetMsgType::IX, tx);
2368+
g_connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::IX, tx));
23682369
}
23692370
}
23702371

@@ -2520,30 +2521,19 @@ void CNode::AskFor(const CInv& inv)
25202521
mapAskFor.insert(std::make_pair(nRequestTime, inv));
25212522
}
25222523

2523-
CDataStream CConnman::BeginMessage(CNode* pnode, int nVersion, int flags, const std::string& sCommand)
2524+
void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
25242525
{
2525-
return {SER_NETWORK, (nVersion ? nVersion : pnode->GetSendVersion()) | flags, CMessageHeader(Params().MessageStart(), sCommand.c_str(), 0) };
2526-
}
2527-
2528-
void CConnman::EndMessage(CDataStream& strm)
2529-
{
2530-
// Set the size
2531-
assert(strm.size () >= CMessageHeader::HEADER_SIZE);
2532-
unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2533-
WriteLE32((uint8_t*)&strm[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize);
2534-
// Set the checksum
2535-
uint256 hash = Hash(strm.begin() + CMessageHeader::HEADER_SIZE, strm.end());
2536-
memcpy((char*)&strm[CMessageHeader::CHECKSUM_OFFSET], hash.begin(), CMessageHeader::CHECKSUM_SIZE);
2537-
2538-
}
2526+
size_t nMessageSize = msg.data.size();
2527+
size_t nTotalSize = nMessageSize + CMessageHeader::HEADER_SIZE;
2528+
LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n", SanitizeString(msg.command.c_str()), nMessageSize, pnode->id);
25392529

2540-
void CConnman::PushMessage(CNode* pnode, CDataStream& strm, const std::string& sCommand)
2541-
{
2542-
if(strm.empty())
2543-
return;
2530+
std::vector<unsigned char> serializedHeader;
2531+
serializedHeader.reserve(CMessageHeader::HEADER_SIZE);
2532+
uint256 hash = Hash(msg.data.data(), msg.data.data() + nMessageSize);
2533+
CMessageHeader hdr(Params().MessageStart(), msg.command.c_str(), nMessageSize);
2534+
memcpy(hdr.pchChecksum, hash.begin(), CMessageHeader::CHECKSUM_SIZE);
25442535

2545-
unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2546-
LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n", SanitizeString(sCommand.c_str()), nSize, pnode->id);
2536+
CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, serializedHeader, 0, hdr};
25472537

25482538
size_t nBytesSent = 0;
25492539
{
@@ -2552,11 +2542,14 @@ void CConnman::PushMessage(CNode* pnode, CDataStream& strm, const std::string& s
25522542
return;
25532543
}
25542544
bool optimisticSend(pnode->vSendMsg.empty());
2555-
pnode->vSendMsg.emplace_back(strm.begin(), strm.end());
25562545

25572546
//log total amount of bytes per command
2558-
pnode->mapSendBytesPerMsgCmd[sCommand] += strm.size();
2559-
pnode->nSendSize += strm.size();
2547+
pnode->mapSendBytesPerMsgCmd[msg.command] += nTotalSize;
2548+
pnode->nSendSize += nTotalSize;
2549+
2550+
pnode->vSendMsg.push_back(std::move(serializedHeader));
2551+
if (nMessageSize)
2552+
pnode->vSendMsg.push_back(std::move(msg.data));
25602553

25612554
// If write queue empty, attempt "optimistic write"
25622555
if (optimisticSend == true)

‎src/net.h‎

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -154,32 +154,7 @@ class CConnman
154154

155155
bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
156156

157-
template <typename... Args>
158-
void PushMessageWithVersionAndFlag(CNode* pnode, int nVersion, int flag, const std::string& sCommand, Args&&... args)
159-
{
160-
auto msg(BeginMessage(pnode, nVersion, flag, sCommand));
161-
::SerializeMany(msg, std::forward<Args>(args)...);
162-
EndMessage(msg);
163-
PushMessage(pnode, msg, sCommand);
164-
}
165-
166-
template <typename... Args>
167-
void PushMessageWithFlag(CNode* pnode, int flag, const std::string& sCommand, Args&&... args)
168-
{
169-
PushMessageWithVersionAndFlag(pnode, 0, flag, sCommand, std::forward<Args>(args)...);
170-
}
171-
172-
template <typename... Args>
173-
void PushMessageWithVersion(CNode* pnode, int nVersion, const std::string& sCommand, Args&&... args)
174-
{
175-
PushMessageWithVersionAndFlag(pnode, nVersion, 0, sCommand, std::forward<Args>(args)...);
176-
}
177-
178-
template <typename... Args>
179-
void PushMessage(CNode* pnode, const std::string& sCommand, Args&&... args)
180-
{
181-
PushMessageWithVersionAndFlag(pnode, 0, 0, sCommand, std::forward<Args>(args)...);
182-
}
157+
void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
183158

184159
template<typename Callable>
185160
bool ForEachNodeContinueIf(Callable&& func)
@@ -373,10 +348,6 @@ class CConnman
373348
void DumpData();
374349
void DumpBanlist();
375350

376-
CDataStream BeginMessage(CNode* node, int nVersion, int flags, const std::string& sCommand);
377-
void PushMessage(CNode* pnode, CDataStream& strm, const std::string& sCommand);
378-
void EndMessage(CDataStream& strm);
379-
380351
// Network stats
381352
void RecordBytesRecv(uint64_t bytes);
382353
void RecordBytesSent(uint64_t bytes);
@@ -602,7 +573,7 @@ class CNode
602573
size_t nSendSize; // total size of all vSendMsg entries
603574
size_t nSendOffset; // offset inside the first vSendMsg already sent
604575
uint64_t nSendBytes;
605-
std::deque<CSerializeData> vSendMsg;
576+
std::deque<std::vector<unsigned char>> vSendMsg;
606577
RecursiveMutex cs_vSend;
607578

608579
RecursiveMutex cs_vProcessMsg;
@@ -757,7 +728,7 @@ class CNode
757728
{
758729
// The send version should always be explicitly set to
759730
// INIT_PROTO_VERSION rather than using this value until the handshake
760-
// is complete. See PushMessageWithVersion().
731+
// is complete.
761732
assert(nSendVersion != 0);
762733
return nSendVersion;
763734
}

‎src/spork.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "main.h"
77
#include "messagesigner.h"
88
#include "net.h"
9+
#include "netmessagemaker.h"
910
#include "spork.h"
1011
#include "sporkdb.h"
1112
#include <iostream>
@@ -153,7 +154,7 @@ void CSporkManager::ProcessSpork(CNode* pfrom, std::string& strCommand, CDataStr
153154
std::map<SporkId, CSporkMessage>::iterator it = mapSporksActive.begin();
154155

155156
while (it != mapSporksActive.end()) {
156-
g_connman->PushMessage(pfrom, NetMsgType::SPORK, it->second);
157+
g_connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::SPORK, it->second));
157158
it++;
158159
}
159160
}

0 commit comments

Comments
 (0)