Skip to content

Commit e9a6d8b

Browse files
hebastohebasto
committed
p2p: Unify Send and Receive protocol versions
There is no change in behavior on the P2P network.
1 parent 147d50d commit e9a6d8b

File tree

7 files changed

+39
-73
lines changed

7 files changed

+39
-73
lines changed

‎src/net.cpp

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -621,32 +621,6 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
621621
return true;
622622
}
623623

624-
void CNode::SetSendVersion(int nVersionIn)
625-
{
626-
// Send version may only be changed in the version message, and
627-
// only one version message is allowed per session. We can therefore
628-
// treat this value as const and even atomic as long as it's only used
629-
// once a version message has been successfully processed. Any attempt to
630-
// set this twice is an error.
631-
if (nSendVersion != 0) {
632-
error("Send version already set for node: %i. Refusing to change from %i to %i", id, nSendVersion, nVersionIn);
633-
} else {
634-
nSendVersion = nVersionIn;
635-
}
636-
}
637-
638-
int CNode::GetSendVersion() const
639-
{
640-
// The send version should always be explicitly set to
641-
// INIT_PROTO_VERSION rather than using this value until SetSendVersion
642-
// has been called.
643-
if (nSendVersion == 0) {
644-
error("Requesting unset send version for node: %i. Using %i", id, INIT_PROTO_VERSION);
645-
return INIT_PROTO_VERSION;
646-
}
647-
return nSendVersion;
648-
}
649-
650624
int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes)
651625
{
652626
// copy data to temporary parsing buffer

‎src/net.h

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -827,7 +827,6 @@ class CNode
827827

828828
std::deque<CInv> vRecvGetData;
829829
uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};
830-
std::atomic<int> nRecvVersion{INIT_PROTO_VERSION};
831830

832831
std::atomic<int64_t> nLastSend{0};
833832
std::atomic<int64_t> nLastRecv{0};
@@ -1014,6 +1013,7 @@ class CNode
10141013
const NodeId id;
10151014
const uint64_t nLocalHostNonce;
10161015
const ConnectionType m_conn_type;
1016+
std::atomic<int> m_greatest_common_version{INIT_PROTO_VERSION};
10171017

10181018
//! Services offered to this peer.
10191019
//!
@@ -1033,7 +1033,6 @@ class CNode
10331033
const ServiceFlags nLocalServices;
10341034

10351035
const int nMyStartingHeight;
1036-
int nSendVersion{0};
10371036
NetPermissionFlags m_permissionFlags{ PF_NONE };
10381037
std::list<CNetMessage> vRecvMsg; // Used only by SocketHandler thread
10391038

@@ -1065,16 +1064,14 @@ class CNode
10651064

10661065
bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete);
10671066

1068-
void SetRecvVersion(int nVersionIn)
1067+
void SetCommonVersion(int greatest_common_version)
10691068
{
1070-
nRecvVersion = nVersionIn;
1069+
m_greatest_common_version = greatest_common_version;
10711070
}
1072-
int GetRecvVersion() const
1071+
int GetCommonVersion() const
10731072
{
1074-
return nRecvVersion;
1073+
return m_greatest_common_version;
10751074
}
1076-
void SetSendVersion(int nVersionIn);
1077-
int GetSendVersion() const;
10781075

10791076
CService GetAddrLocal() const;
10801077
//! May not be called more than once

‎src/net_processing.cpp

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -669,12 +669,12 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
669669
// As per BIP152, we only get 3 of our peers to announce
670670
// blocks using compact encodings.
671671
connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
672-
connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
672+
connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
673673
return true;
674674
});
675675
lNodesAnnouncingHeaderAndIDs.pop_front();
676676
}
677-
connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
677+
connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
678678
lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
679679
return true;
680680
});
@@ -1585,7 +1585,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
15851585
LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId());
15861586
}
15871587
}
1588-
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
1588+
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
15891589
// disconnect node in case we have reached the outbound limit for serving historical blocks
15901590
if (send &&
15911591
connman.OutboundTargetReached(true) &&
@@ -1728,7 +1728,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
17281728

17291729
std::deque<CInv>::iterator it = pfrom.vRecvGetData.begin();
17301730
std::vector<CInv> vNotFound;
1731-
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
1731+
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
17321732

17331733
const std::chrono::seconds now = GetTime<std::chrono::seconds>();
17341734
// Get last mempool request time
@@ -1834,14 +1834,14 @@ void PeerManager::SendBlockTransactions(CNode& pfrom, const CBlock& block, const
18341834
resp.txn[i] = block.vtx[req.indexes[i]];
18351835
}
18361836
LOCK(cs_main);
1837-
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
1837+
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
18381838
int nSendFlags = State(pfrom.GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS;
18391839
m_connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp));
18401840
}
18411841

18421842
void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHeader>& headers, bool via_compact_block)
18431843
{
1844-
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
1844+
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
18451845
size_t nCount = headers.size();
18461846

18471847
if (nCount == 0) {
@@ -2211,7 +2211,7 @@ static void ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const CChainPara
22112211
}
22122212

22132213
for (const auto& filter : filters) {
2214-
CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
2214+
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
22152215
.Make(NetMsgType::CFILTER, filter);
22162216
connman.PushMessage(&peer, std::move(msg));
22172217
}
@@ -2263,7 +2263,7 @@ static void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainPar
22632263
return;
22642264
}
22652265

2266-
CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
2266+
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
22672267
.Make(NetMsgType::CFHEADERS,
22682268
filter_type_ser,
22692269
stop_index->GetBlockHash(),
@@ -2315,7 +2315,7 @@ static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainPar
23152315
}
23162316
}
23172317

2318-
CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
2318+
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
23192319
.Make(NetMsgType::CFCHECKPT,
23202320
filter_type_ser,
23212321
stop_index->GetBlockHash(),
@@ -2406,10 +2406,10 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
24062406
PushNodeVersion(pfrom, m_connman, GetAdjustedTime());
24072407

24082408
if (nVersion >= WTXID_RELAY_VERSION) {
2409-
m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::WTXIDRELAY));
2409+
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::WTXIDRELAY));
24102410
}
24112411

2412-
m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
2412+
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::VERACK));
24132413

24142414
pfrom.nServices = nServices;
24152415
pfrom.SetAddrLocal(addrMe);
@@ -2431,7 +2431,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
24312431
}
24322432

24332433
// Change version
2434-
pfrom.SetSendVersion(nSendVersion);
2434+
pfrom.SetCommonVersion(nSendVersion);
24352435
pfrom.nVersion = nVersion;
24362436

24372437
if((nServices & NODE_WITNESS))
@@ -2520,11 +2520,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
25202520
}
25212521

25222522
// At this point, the outgoing message serialization version can't change.
2523-
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
2523+
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
25242524

25252525
if (msg_type == NetMsgType::VERACK)
25262526
{
2527-
pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION));
2527+
pfrom.SetCommonVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION));
25282528

25292529
if (!pfrom.IsInboundConn()) {
25302530
// Mark this node as currently connected, so we update its timestamp later.
@@ -3872,7 +3872,7 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgP
38723872
}
38733873
CNetMessage& msg(msgs.front());
38743874

3875-
msg.SetVersion(pfrom->GetRecvVersion());
3875+
msg.SetVersion(pfrom->GetCommonVersion());
38763876
// Check network magic
38773877
if (!msg.m_valid_netmagic) {
38783878
LogPrint(BCLog::NET, "PROCESSMESSAGE: INVALID MESSAGESTART %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId());
@@ -3920,7 +3920,7 @@ void PeerManager::ConsiderEviction(CNode& pto, int64_t time_in_seconds)
39203920
AssertLockHeld(cs_main);
39213921

39223922
CNodeState &state = *State(pto.GetId());
3923-
const CNetMsgMaker msgMaker(pto.GetSendVersion());
3923+
const CNetMsgMaker msgMaker(pto.GetCommonVersion());
39243924

39253925
if (!state.m_chain_sync.m_protect && pto.IsOutboundOrBlockRelayConn() && state.fSyncStarted) {
39263926
// This is an outbound peer subject to disconnection if they don't
@@ -4082,7 +4082,7 @@ bool PeerManager::SendMessages(CNode* pto)
40824082
return true;
40834083

40844084
// If we get here, the outgoing message serialization version is set and can't change.
4085-
const CNetMsgMaker msgMaker(pto->GetSendVersion());
4085+
const CNetMsgMaker msgMaker(pto->GetCommonVersion());
40864086

40874087
//
40884088
// Message: ping

‎src/test/denialofservice_tests.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
8585
// Mock an outbound peer
8686
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
8787
CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY);
88-
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
88+
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
8989

9090
peerLogic->InitializeNode(&dummyNode1);
9191
dummyNode1.nVersion = 1;
@@ -138,7 +138,7 @@ static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerManager &pee
138138
CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
139139
vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), 0, INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY));
140140
CNode &node = *vNodes.back();
141-
node.SetSendVersion(PROTOCOL_VERSION);
141+
node.SetCommonVersion(PROTOCOL_VERSION);
142142

143143
peerLogic.InitializeNode(&node);
144144
node.nVersion = 1;
@@ -230,7 +230,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
230230
banman->ClearBanned();
231231
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
232232
CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND);
233-
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
233+
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
234234
peerLogic->InitializeNode(&dummyNode1);
235235
dummyNode1.nVersion = 1;
236236
dummyNode1.fSuccessfullyConnected = true;
@@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
244244

245245
CAddress addr2(ip(0xa0b0c002), NODE_NONE);
246246
CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND);
247-
dummyNode2.SetSendVersion(PROTOCOL_VERSION);
247+
dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
248248
peerLogic->InitializeNode(&dummyNode2);
249249
dummyNode2.nVersion = 1;
250250
dummyNode2.fSuccessfullyConnected = true;
@@ -281,7 +281,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
281281

282282
CAddress addr(ip(0xa0b0c001), NODE_NONE);
283283
CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, 4, 4, CAddress(), "", ConnectionType::INBOUND);
284-
dummyNode.SetSendVersion(PROTOCOL_VERSION);
284+
dummyNode.SetCommonVersion(PROTOCOL_VERSION);
285285
peerLogic->InitializeNode(&dummyNode);
286286
dummyNode.nVersion = 1;
287287
dummyNode.fSuccessfullyConnected = true;

‎src/test/fuzz/net.cpp

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
4848
fuzzed_data_provider.ConsumeRandomLengthString(32),
4949
fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND_FULL_RELAY, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY, ConnectionType::ADDR_FETCH})};
5050
while (fuzzed_data_provider.ConsumeBool()) {
51-
switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 12)) {
51+
switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 11)) {
5252
case 0: {
5353
node.CloseSocketDisconnect();
5454
break;
@@ -58,7 +58,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
5858
break;
5959
}
6060
case 2: {
61-
node.SetSendVersion(fuzzed_data_provider.ConsumeIntegral<int>());
61+
node.SetCommonVersion(fuzzed_data_provider.ConsumeIntegral<int>());
6262
break;
6363
}
6464
case 3: {
@@ -71,21 +71,17 @@ void test_one_input(const std::vector<uint8_t>& buffer)
7171
break;
7272
}
7373
case 4: {
74-
node.SetRecvVersion(fuzzed_data_provider.ConsumeIntegral<int>());
75-
break;
76-
}
77-
case 5: {
7874
const CNode* add_ref_node = node.AddRef();
7975
assert(add_ref_node == &node);
8076
break;
8177
}
82-
case 6: {
78+
case 5: {
8379
if (node.GetRefCount() > 0) {
8480
node.Release();
8581
}
8682
break;
8783
}
88-
case 7: {
84+
case 6: {
8985
if (node.m_addr_known == nullptr) {
9086
break;
9187
}
@@ -96,7 +92,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
9692
node.AddAddressKnown(*addr_opt);
9793
break;
9894
}
99-
case 8: {
95+
case 7: {
10096
if (node.m_addr_known == nullptr) {
10197
break;
10298
}
@@ -108,27 +104,27 @@ void test_one_input(const std::vector<uint8_t>& buffer)
108104
node.PushAddress(*addr_opt, fast_random_context);
109105
break;
110106
}
111-
case 9: {
107+
case 8: {
112108
const std::optional<CInv> inv_opt = ConsumeDeserializable<CInv>(fuzzed_data_provider);
113109
if (!inv_opt) {
114110
break;
115111
}
116112
node.AddKnownTx(inv_opt->hash);
117113
break;
118114
}
119-
case 10: {
115+
case 9: {
120116
node.PushTxInventory(ConsumeUInt256(fuzzed_data_provider));
121117
break;
122118
}
123-
case 11: {
119+
case 10: {
124120
const std::optional<CService> service_opt = ConsumeDeserializable<CService>(fuzzed_data_provider);
125121
if (!service_opt) {
126122
break;
127123
}
128124
node.SetAddrLocal(*service_opt);
129125
break;
130126
}
131-
case 12: {
127+
case 11: {
132128
const std::vector<uint8_t> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);
133129
bool complete;
134130
node.ReceiveMsgBytes((const char*)b.data(), b.size(), complete);
@@ -143,10 +139,9 @@ void test_one_input(const std::vector<uint8_t>& buffer)
143139
(void)node.GetLocalNonce();
144140
(void)node.GetLocalServices();
145141
(void)node.GetMyStartingHeight();
146-
(void)node.GetRecvVersion();
147142
const int ref_count = node.GetRefCount();
148143
assert(ref_count >= 0);
149-
(void)node.GetSendVersion();
144+
(void)node.GetCommonVersion();
150145
(void)node.RelayAddrsWithConn();
151146

152147
const NetPermissionFlags net_permission_flags = fuzzed_data_provider.ConsumeBool() ?

‎src/test/fuzz/process_message.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
7171
CNode& p2p_node = *MakeUnique<CNode>(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND_FULL_RELAY).release();
7272
p2p_node.fSuccessfullyConnected = true;
7373
p2p_node.nVersion = PROTOCOL_VERSION;
74-
p2p_node.SetSendVersion(PROTOCOL_VERSION);
74+
p2p_node.SetCommonVersion(PROTOCOL_VERSION);
7575
connman.AddTestNode(p2p_node);
7676
g_setup->m_node.peerman->InitializeNode(&p2p_node);
7777
try {

‎src/test/fuzz/process_messages.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
5151
p2p_node.fSuccessfullyConnected = true;
5252
p2p_node.fPauseSend = false;
5353
p2p_node.nVersion = PROTOCOL_VERSION;
54-
p2p_node.SetSendVersion(PROTOCOL_VERSION);
54+
p2p_node.SetCommonVersion(PROTOCOL_VERSION);
5555
g_setup->m_node.peerman->InitializeNode(&p2p_node);
5656

5757
connman.AddTestNode(p2p_node);

0 commit comments

Comments
 (0)