From fa82f4ea96749115311cffa0919d49d383c4d28b Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 24 Aug 2021 19:19:58 +0200 Subject: [PATCH 1/3] Remove unused MaybeSetAddrName This logic is a no-op since it was introduced in commit f9f5cfc50637f2cd1540923caf337e2651ec1625. m_addr_name is never initialized to the empty string, because ToStringIPPort never returns an empty string. --- src/net.cpp | 18 ++++-------------- src/net.h | 2 -- src/test/fuzz/net.cpp | 3 --- 3 files changed, 4 insertions(+), 19 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 57b8844d6bf31..51d9c7f590450 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -414,14 +414,10 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo return nullptr; } // It is possible that we already have a connection to the IP/port pszDest resolved to. - // In that case, drop the connection that was just created, and return the existing CNode instead. - // Also store the name we used to connect in that CNode, so that future FindNode() calls to that - // name catch this early. + // In that case, drop the connection that was just created. LOCK(cs_vNodes); CNode* pnode = FindNode(static_cast(addrConnect)); - if (pnode) - { - pnode->MaybeSetAddrName(std::string(pszDest)); + if (pnode) { LogPrintf("Failed to open new connection, already connected\n"); return nullptr; } @@ -539,14 +535,8 @@ std::string CNode::GetAddrName() const { return addrName; } -void CNode::MaybeSetAddrName(const std::string& addrNameIn) { - LOCK(cs_addrName); - if (addrName.empty()) { - addrName = addrNameIn; - } -} - -CService CNode::GetAddrLocal() const { +CService CNode::GetAddrLocal() const +{ LOCK(cs_addrLocal); return addrLocal; } diff --git a/src/net.h b/src/net.h index 28cd635976914..ee8c1119e75f7 100644 --- a/src/net.h +++ b/src/net.h @@ -659,8 +659,6 @@ class CNode } std::string GetAddrName() const; - //! Sets the addrName only if it was not previously set - void MaybeSetAddrName(const std::string& addrNameIn); std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 20d8581312812..7b7bde746c4b0 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -37,9 +37,6 @@ FUZZ_TARGET_INIT(net, initialize_net) [&] { node.CloseSocketDisconnect(); }, - [&] { - node.MaybeSetAddrName(fuzzed_data_provider.ConsumeRandomLengthString(32)); - }, [&] { const std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); if (!SanityCheckASMap(asmap)) { From fa786570a5fdf6723b35883054f9f840a3440f92 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 24 Aug 2021 19:54:13 +0200 Subject: [PATCH 2/3] Remove unused RecursiveMutex cs_addrName --- src/net.cpp | 10 +++++----- src/net.h | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 51d9c7f590450..97b3c42d96c66 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -530,9 +530,9 @@ std::string ConnectionTypeAsString(ConnectionType conn_type) assert(false); } -std::string CNode::GetAddrName() const { - LOCK(cs_addrName); - return addrName; +std::string CNode::GetAddrName() const +{ + return m_addr_name; } CService CNode::GetAddrLocal() const @@ -2956,6 +2956,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const : nTimeConnected(GetTimeSeconds()), addr(addrIn), addrBind(addrBindIn), + m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn}, m_inbound_onion(inbound_onion), nKeyedNetGroup(nKeyedNetGroupIn), id(idIn), @@ -2965,7 +2966,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); hSocket = hSocketIn; - addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; if (conn_type_in != ConnectionType::BLOCK_RELAY) { m_tx_relay = std::make_unique(); } @@ -2975,7 +2975,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0; if (fLogIPs) { - LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", addrName, id); + LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", m_addr_name, id); } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } diff --git a/src/net.h b/src/net.h index ee8c1119e75f7..0a1686da8c5c3 100644 --- a/src/net.h +++ b/src/net.h @@ -430,6 +430,7 @@ class CNode const CAddress addr; // Bind address of our side of the connection const CAddress addrBind; + const std::string m_addr_name; //! Whether this peer is an inbound onion, i.e. connected via our Tor onion service. const bool m_inbound_onion; std::atomic nVersion{0}; @@ -691,10 +692,7 @@ class CNode //! service advertisements. const ServiceFlags nLocalServices; - std::list vRecvMsg; // Used only by SocketHandler thread - - mutable RecursiveMutex cs_addrName; - std::string addrName GUARDED_BY(cs_addrName); + std::list vRecvMsg; // Used only by SocketHandler thread // Our address, as reported by the peer CService addrLocal GUARDED_BY(cs_addrLocal); From fa9eade142964d5482fe8d2109fb67a9556ae93d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 26 Aug 2021 10:39:10 +0200 Subject: [PATCH 3/3] Remove GetAddrName https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c131-avoid-trivial-getters-and-setters --- doc/tracing.md | 2 +- src/net.cpp | 13 ++++--------- src/net.h | 4 +--- src/net_processing.cpp | 2 +- src/qt/peertablemodel.cpp | 2 +- src/qt/peertablesortproxy.cpp | 2 +- src/qt/rpcconsole.cpp | 2 +- src/rpc/net.cpp | 2 +- src/test/fuzz/net.cpp | 1 - 9 files changed, 11 insertions(+), 19 deletions(-) diff --git a/doc/tracing.md b/doc/tracing.md index 1242a0d25002b..87fc9603fe02a 100644 --- a/doc/tracing.md +++ b/doc/tracing.md @@ -147,7 +147,7 @@ For example: ```C++ TRACE6(net, inbound_message, pnode->GetId(), - pnode->GetAddrName().c_str(), + pnode->m_addr_name.c_str(), pnode->ConnectionTypeAsString().c_str(), sanitizedType.c_str(), msg.data.size(), diff --git a/src/net.cpp b/src/net.cpp index 97b3c42d96c66..9b1e17c587da3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -331,7 +331,7 @@ CNode* CConnman::FindNode(const std::string& addrName) { LOCK(cs_vNodes); for (CNode* pnode : vNodes) { - if (pnode->GetAddrName() == addrName) { + if (pnode->m_addr_name == addrName) { return pnode; } } @@ -530,11 +530,6 @@ std::string ConnectionTypeAsString(ConnectionType conn_type) assert(false); } -std::string CNode::GetAddrName() const -{ - return m_addr_name; -} - CService CNode::GetAddrLocal() const { LOCK(cs_addrLocal); @@ -577,7 +572,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(nLastBlockTime); X(nTimeConnected); X(nTimeOffset); - stats.addrName = GetAddrName(); + X(m_addr_name); X(nVersion); { LOCK(cs_SubVer); @@ -2127,7 +2122,7 @@ std::vector CConnman::GetAddedNodeInfo() const if (pnode->addr.IsValid()) { mapConnected[pnode->addr] = pnode->IsInboundConn(); } - std::string addrName = pnode->GetAddrName(); + std::string addrName{pnode->m_addr_name}; if (!addrName.empty()) { mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->IsInboundConn(), static_cast(pnode->addr)); } @@ -3004,7 +2999,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) TRACE6(net, outbound_message, pnode->GetId(), - pnode->GetAddrName().c_str(), + pnode->m_addr_name.c_str(), pnode->ConnectionTypeAsString().c_str(), msg.m_type.c_str(), msg.data.size(), diff --git a/src/net.h b/src/net.h index 0a1686da8c5c3..d568c56205e03 100644 --- a/src/net.h +++ b/src/net.h @@ -248,7 +248,7 @@ class CNodeStats int64_t nLastBlockTime; int64_t nTimeConnected; int64_t nTimeOffset; - std::string addrName; + std::string m_addr_name; int nVersion; std::string cleanSubVer; bool fInbound; @@ -659,8 +659,6 @@ class CNode return nLocalServices; } - std::string GetAddrName() const; - std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } /** A ping-pong round trip has completed successfully. Update latest and minimum ping times. */ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8be82f7ebcfa6..3ad34e83ba3fd 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4086,7 +4086,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt TRACE6(net, inbound_message, pfrom->GetId(), - pfrom->GetAddrName().c_str(), + pfrom->m_addr_name.c_str(), pfrom->ConnectionTypeAsString().c_str(), msg.m_command.c_str(), msg.m_recv.size(), diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp index 98efaf29d727d..433a1ea934e19 100644 --- a/src/qt/peertablemodel.cpp +++ b/src/qt/peertablemodel.cpp @@ -72,7 +72,7 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const case NetNodeId: return (qint64)rec->nodeStats.nodeid; case Address: - return QString::fromStdString(rec->nodeStats.addrName); + return QString::fromStdString(rec->nodeStats.m_addr_name); case Direction: return QString(rec->nodeStats.fInbound ? //: An Inbound Connection from a Peer. diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp index f92eef48f106e..419133bc32f2c 100644 --- a/src/qt/peertablesortproxy.cpp +++ b/src/qt/peertablesortproxy.cpp @@ -25,7 +25,7 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd case PeerTableModel::NetNodeId: return left_stats.nodeid < right_stats.nodeid; case PeerTableModel::Address: - return left_stats.addrName.compare(right_stats.addrName) < 0; + return left_stats.m_addr_name.compare(right_stats.m_addr_name) < 0; case PeerTableModel::Direction: return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound case PeerTableModel::ConnectionType: diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index ba3eb80efa59b..19b8aa0f7dda9 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1129,7 +1129,7 @@ void RPCConsole::updateDetailWidget() } const auto stats = selected_peers.first().data(PeerTableModel::StatsRole).value(); // update the detail ui with latest node information - QString peerAddrDetails(QString::fromStdString(stats->nodeStats.addrName) + " "); + QString peerAddrDetails(QString::fromStdString(stats->nodeStats.m_addr_name) + " "); peerAddrDetails += tr("(peer: %1)").arg(QString::number(stats->nodeStats.nodeid)); if (!stats->nodeStats.addrLocal.empty()) peerAddrDetails += "
" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal)); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 861b8891186f2..0f554ec5e7067 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -197,7 +197,7 @@ static RPCHelpMan getpeerinfo() CNodeStateStats statestats; bool fStateStats = peerman.GetNodeStateStats(stats.nodeid, statestats); obj.pushKV("id", stats.nodeid); - obj.pushKV("addr", stats.addrName); + obj.pushKV("addr", stats.m_addr_name); if (stats.addrBind.IsValid()) { obj.pushKV("addrbind", stats.addrBind.ToString()); } diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 7b7bde746c4b0..9a579c053f2cc 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -79,7 +79,6 @@ FUZZ_TARGET_INIT(net, initialize_net) } (void)node.GetAddrLocal(); - (void)node.GetAddrName(); (void)node.GetId(); (void)node.GetLocalNonce(); (void)node.GetLocalServices();