Skip to content

Commit

Permalink
Merge #17785: p2p: Unify Send and Receive protocol versions
Browse files Browse the repository at this point in the history 
ddefb5c p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d20267 refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)

Pull request description:

  On master (6fef85b) `CNode` has two members to keep protocol version:
  - `nRecvVersion` for received messages
  - `nSendVersion` for messages to send

  After exchanging with `VERSION` and `VERACK` messages via protocol version `INIT_PROTO_VERSION`, both nodes set `nRecvVersion` _and_ `nSendVersion` to _the same_ value which is the greatest common protocol version.

  This PR:
  - replaces two `CNode` members, `nRecvVersion` `nSendVersion`, with `m_greatest_common_version`
  - removes duplicated getter and setter

  There is no change in behavior on the P2P network.

ACKs for top commit:
  jnewbery:
    ACK ddefb5c
  naumenkogs:
    ACK ddefb5c
  fjahr:
    Code review ACK ddefb5c
  amitiuttarwar:
    code review but untested ACK ddefb5c
  benthecarman:
    utACK `ddefb5c`

Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
  • Loading branch information
@laanwj
laanwj committed Sep 21, 2020
2 parents 8c5f681 + ddefb5c commit 7737603
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 92 deletions.
28 changes: 1 addition & 27 deletions src/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -622,32 +622,6 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
return true;
}

void CNode::SetSendVersion(int nVersionIn)
{
// Send version may only be changed in the version message, and
// only one version message is allowed per session. We can therefore
// treat this value as const and even atomic as long as it's only used
// once a version message has been successfully processed. Any attempt to
// set this twice is an error.
if (nSendVersion != 0) {
error("Send version already set for node: %i. Refusing to change from %i to %i", id, nSendVersion, nVersionIn);
} else {
nSendVersion = nVersionIn;
}
}

int CNode::GetSendVersion() const
{
// The send version should always be explicitly set to
// INIT_PROTO_VERSION rather than using this value until SetSendVersion
// has been called.
if (nSendVersion == 0) {
error("Requesting unset send version for node: %i. Using %i", id, INIT_PROTO_VERSION);
return INIT_PROTO_VERSION;
}
return nSendVersion;
}

int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes)
{
// copy data to temporary parsing buffer
Expand Down Expand Up @@ -1194,7 +1168,7 @@ void CConnman::InactivityCheck(CNode *pnode)
LogPrintf("socket sending timeout: %is\n", nTime - pnode->nLastSend);
pnode->fDisconnect = true;
}
else if (nTime - pnode->nLastRecv > (pnode->nVersion > BIP0031_VERSION ? TIMEOUT_INTERVAL : 90*60))
else if (nTime - pnode->nLastRecv > (pnode->GetCommonVersion() > BIP0031_VERSION ? TIMEOUT_INTERVAL : 90*60))
{
LogPrintf("socket receive timeout: %is\n", nTime - pnode->nLastRecv);
pnode->fDisconnect = true;
Expand Down
13 changes: 5 additions & 8 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,6 @@ class CNode

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

std::atomic<int64_t> nLastSend{0};
std::atomic<int64_t> nLastRecv{0};
Expand Down Expand Up @@ -1018,6 +1017,7 @@ class CNode
const NodeId id;
const uint64_t nLocalHostNonce;
const ConnectionType m_conn_type;
std::atomic<int> m_greatest_common_version{INIT_PROTO_VERSION};

//! Services offered to this peer.
//!
Expand All @@ -1037,7 +1037,6 @@ class CNode
const ServiceFlags nLocalServices;

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

Expand Down Expand Up @@ -1069,16 +1068,14 @@ class CNode

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

void SetRecvVersion(int nVersionIn)
void SetCommonVersion(int greatest_common_version)
{
nRecvVersion = nVersionIn;
m_greatest_common_version = greatest_common_version;
}
int GetRecvVersion() const
int GetCommonVersion() const
{
return nRecvVersion;
return m_greatest_common_version;
}
void SetSendVersion(int nVersionIn);
int GetSendVersion() const;

CService GetAddrLocal() const;
//! May not be called more than once
Expand Down
66 changes: 31 additions & 35 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,12 +669,12 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
// As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings.
connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
return true;
});
lNodesAnnouncingHeaderAndIDs.pop_front();
}
connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
return true;
});
Expand Down Expand Up @@ -1359,7 +1359,7 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
LockAssertion lock(::cs_main);

// TODO: Avoid the repeated-serialization here
if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
if (pnode->GetCommonVersion() < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
return;
ProcessBlockAvailability(pnode->GetId());
CNodeState &state = *State(pnode->GetId());
Expand Down Expand Up @@ -1586,7 +1586,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId());
}
}
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
// disconnect node in case we have reached the outbound limit for serving historical blocks
if (send &&
connman.OutboundTargetReached(true) &&
Expand Down Expand Up @@ -1729,7 +1729,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm

std::deque<CInv>::iterator it = pfrom.vRecvGetData.begin();
std::vector<CInv> vNotFound;
const CNetMsgMaker msgMaker(pfrom.GetSendVersion());
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());

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

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

if (nCount == 0) {
Expand Down Expand Up @@ -2212,7 +2212,7 @@ static void ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const CChainPara
}

for (const auto& filter : filters) {
CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
.Make(NetMsgType::CFILTER, filter);
connman.PushMessage(&peer, std::move(msg));
}
Expand Down Expand Up @@ -2264,7 +2264,7 @@ static void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainPar
return;
}

CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
.Make(NetMsgType::CFHEADERS,
filter_type_ser,
stop_index->GetBlockHash(),
Expand Down Expand Up @@ -2316,7 +2316,7 @@ static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainPar
}
}

CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion())
.Make(NetMsgType::CFCHECKPT,
filter_type_ser,
stop_index->GetBlockHash(),
Expand Down Expand Up @@ -2351,13 +2351,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
uint64_t nServiceInt;
ServiceFlags nServices;
int nVersion;
int nSendVersion;
std::string cleanSubVer;
int nStartingHeight = -1;
bool fRelay = true;

vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
nSendVersion = std::min(nVersion, PROTOCOL_VERSION);
nServices = ServiceFlags(nServiceInt);
if (!pfrom.IsInboundConn())
{
Expand Down Expand Up @@ -2406,11 +2404,16 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
if (pfrom.IsInboundConn())
PushNodeVersion(pfrom, m_connman, GetAdjustedTime());

if (nVersion >= WTXID_RELAY_VERSION) {
m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::WTXIDRELAY));
// Change version
const int greatest_common_version = std::min(nVersion, PROTOCOL_VERSION);
pfrom.SetCommonVersion(greatest_common_version);
pfrom.nVersion = nVersion;

if (greatest_common_version >= WTXID_RELAY_VERSION) {
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::WTXIDRELAY));
}

m_connman.PushMessage(&pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::VERACK));

pfrom.nServices = nServices;
pfrom.SetAddrLocal(addrMe);
Expand All @@ -2431,10 +2434,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
pfrom.m_tx_relay->fRelayTxes = fRelay; // set to true after we get the first filter* message
}

// Change version
pfrom.SetSendVersion(nSendVersion);
pfrom.nVersion = nVersion;

if((nServices & NODE_WITNESS))
{
LOCK(cs_main);
Expand Down Expand Up @@ -2480,7 +2479,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
}

// Get recent addresses
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR));
pfrom.fGetAddr = true;

// Moves address from New to Tried table in Addrman, resolves
Expand All @@ -2502,9 +2501,9 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
AddTimeData(pfrom.addr, nTimeOffset);

// If the peer is old enough to have the old alert system, send it the final alert.
if (pfrom.nVersion <= 70012) {
if (greatest_common_version <= 70012) {
CDataStream finalAlert(ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50"), SER_NETWORK, PROTOCOL_VERSION);
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make("alert", finalAlert));
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", finalAlert));
}

// Feeler connections exist only to verify if address is online.
Expand All @@ -2521,12 +2520,10 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
}

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

if (msg_type == NetMsgType::VERACK)
{
pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION));

if (!pfrom.IsInboundConn()) {
// Mark this node as currently connected, so we update its timestamp later.
LOCK(cs_main);
Expand All @@ -2537,14 +2534,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
pfrom.m_tx_relay == nullptr ? "block-relay" : "full-relay");
}

if (pfrom.nVersion >= SENDHEADERS_VERSION) {
if (pfrom.GetCommonVersion() >= SENDHEADERS_VERSION) {
// Tell our peer we prefer to receive headers rather than inv's
// We send this to non-NODE NETWORK peers as well, because even
// non-NODE NETWORK peers can announce blocks (such as pruning
// nodes)
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDHEADERS));
}
if (pfrom.nVersion >= SHORT_IDS_BLOCKS_VERSION) {
if (pfrom.GetCommonVersion() >= SHORT_IDS_BLOCKS_VERSION) {
// Tell our peer we are willing to provide version 1 or 2 cmpctblocks
// However, we do not request new block announcements using
// cmpctblock messages.
Expand All @@ -2570,7 +2567,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
pfrom.fDisconnect = true;
return;
}
if (pfrom.nVersion >= WTXID_RELAY_VERSION) {
if (pfrom.GetCommonVersion() >= WTXID_RELAY_VERSION) {
LOCK(cs_main);
if (!State(pfrom.GetId())->m_wtxid_relay) {
State(pfrom.GetId())->m_wtxid_relay = true;
Expand Down Expand Up @@ -3583,8 +3580,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
}

if (msg_type == NetMsgType::PING) {
if (pfrom.nVersion > BIP0031_VERSION)
{
if (pfrom.GetCommonVersion() > BIP0031_VERSION) {
uint64_t nonce = 0;
vRecv >> nonce;
// Echo the message back with the nonce. This allows for two useful features:
Expand Down Expand Up @@ -3871,7 +3867,7 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgP
}
CNetMessage& msg(msgs.front());

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

CNodeState &state = *State(pto.GetId());
const CNetMsgMaker msgMaker(pto.GetSendVersion());
const CNetMsgMaker msgMaker(pto.GetCommonVersion());

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

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

//
// Message: ping
Expand All @@ -4102,7 +4098,7 @@ bool PeerManager::SendMessages(CNode* pto)
}
pto->fPingQueued = false;
pto->m_ping_start = GetTime<std::chrono::microseconds>();
if (pto->nVersion > BIP0031_VERSION) {
if (pto->GetCommonVersion() > BIP0031_VERSION) {
pto->nPingNonceSent = nonce;
m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::PING, nonce));
} else {
Expand Down Expand Up @@ -4641,7 +4637,7 @@ bool PeerManager::SendMessages(CNode* pto)
//
// Message: feefilter
//
if (pto->m_tx_relay != nullptr && pto->nVersion >= FEEFILTER_VERSION && gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
if (pto->m_tx_relay != nullptr && pto->GetCommonVersion() >= FEEFILTER_VERSION && gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
!pto->HasPermission(PF_FORCERELAY) // peers with the forcerelay permission should not filter txs to us
) {
CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
Expand Down
10 changes: 5 additions & 5 deletions src/test/denialofservice_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
// Mock an outbound peer
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY);
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);

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

peerLogic.InitializeNode(&node);
node.nVersion = 1;
Expand Down Expand Up @@ -229,7 +229,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
banman->ClearBanned();
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND);
dummyNode1.SetSendVersion(PROTOCOL_VERSION);
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode1);
dummyNode1.nVersion = 1;
dummyNode1.fSuccessfullyConnected = true;
Expand All @@ -243,7 +243,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)

CAddress addr2(ip(0xa0b0c002), NODE_NONE);
CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND);
dummyNode2.SetSendVersion(PROTOCOL_VERSION);
dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode2);
dummyNode2.nVersion = 1;
dummyNode2.fSuccessfullyConnected = true;
Expand Down Expand Up @@ -280,7 +280,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)

CAddress addr(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, 4, 4, CAddress(), "", ConnectionType::INBOUND);
dummyNode.SetSendVersion(PROTOCOL_VERSION);
dummyNode.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode);
dummyNode.nVersion = 1;
dummyNode.fSuccessfullyConnected = true;
Expand Down
Loading

0 comments on commit 7737603

Please sign in to comment.