Remove unused MaybeSetAddrName

MarcoFalke commented at 5:26 pm on August 23, 2021: member

.

MarcoFalke added the label Refactoring on Aug 23, 2021

MarcoFalke added the label P2P on Aug 23, 2021

hebasto approved

hebasto commented at 8:16 pm on August 23, 2021: member

ACK fa2d049bfe4f1365011e440adaeb096ddd544143.

Indeed, on master (dbcb5742c48fd26f77e500291d7083e12eec741b) the CNode::addrName member has been assigned with a non-empty value in the CNode::CNode constructor: https://github.com/bitcoin/bitcoin/blob/dbcb5742c48fd26f77e500291d7083e12eec741b/src/net.cpp#L2978

Maybe split out a rename commit with s/addrName/m_addr_name/?

promag commented at 10:46 pm on August 23, 2021: member

Code review ACK fa2d049bfe4f1365011e440adaeb096ddd544143.

fanquake requested review from jnewbery on Aug 24, 2021

DrahtBot commented at 9:09 am on August 24, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21878 (Make all networking code mockable by vasild)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
#19757 (net/net_processing: Convert net std::list buffers to std::forward_list by JeremyRubin)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery commented at 12:45 pm on August 24, 2021: member

Concept ACK.

This was introduced in https://github.com/bitcoin/bitcoin/commit/f9f5cfc50637f2cd1540923caf337e2651ec1625, and it looks like it would never work as intended, since the CNode ctor has always set the addrName to a non-empty string.

The comment above explains the intention here: Also store the name we used to connect in that CNode, so that future FindNode() calls to that name catch this early.. That comment should be removed if we get rid of this code.

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.

fa82f4ea96

MarcoFalke force-pushed on Aug 24, 2021

hebasto approved

hebasto commented at 6:33 pm on August 24, 2021: member

ACK fa973e4d2f50e952fed0aeba7a1a6132f9968a7c, rebased, split in two commits and fixed comment since my previous review.

in src/net.cpp:420 in fa973e4d2f outdated

421             LOCK(cs_vNodes);
422             CNode* pnode = FindNode(static_cast<CService>(addrConnect));
423-            if (pnode)
424-            {
425-                pnode->MaybeSetAddrName(std::string(pszDest));
426+            if (pnode) {

jnewbery commented at 11:22 am on August 25, 2021:

Could be simplified further to:

 0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -415,9 +415,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 3             }
 4             // It is possible that we already have a connection to the IP/port pszDest resolved to.
 5             // In that case, drop the connection that was just created.
 6-            LOCK(cs_vNodes);
 7-            CNode* pnode = FindNode(static_cast<CService>(addrConnect));
 8-            if (pnode) {
 9+            if (FindNode(static_cast<CService>(addrConnect)));
10                 LogPrintf("Failed to open new connection, already connected\n");
11                 return nullptr;
12             }

but probably best left for a future PR. Same for the call to FindNode() above.

in src/net.cpp:533 in fa973e4d2f outdated

538-void CNode::MaybeSetAddrName(const std::string& addrNameIn) {
539-    LOCK(cs_addrName);
540-    if (addrName.empty()) {
541-        addrName = addrNameIn;
542-    }
543+std::string CNode::GetAddrName() const

jnewbery commented at 11:29 am on August 25, 2021:

Consider putting the definition of this simple getter in the header, for inlining, or making the const member public and removing the getter entirely (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c131-avoid-trivial-getters-and-setters)

MarcoFalke commented at 8:29 am on August 26, 2021:

done

jnewbery commented at 8:46 am on August 26, 2021:

fuzz/net.cpp is still using GetAddrName()

MarcoFalke commented at 8:47 am on August 26, 2021:

Thanks, fixed.

jnewbery commented at 11:30 am on August 25, 2021: member

Code review ACK fa973e4d2f50e952fed0aeba7a1a6132f9968a7c

Couple of suggestions for follow-ups.

Remove unused RecursiveMutex cs_addrName fa786570a5

MarcoFalke force-pushed on Aug 26, 2021

MarcoFalke commented at 8:30 am on August 26, 2021: member

I’ve force pushed to address a review comment. Happy to revert to the version that had two ACKs, just let me know.

Remove GetAddrName

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c131-avoid-trivial-getters-and-setters

fa9eade142

MarcoFalke force-pushed on Aug 26, 2021

jnewbery commented at 9:24 am on August 26, 2021: member

Code review ACK fa9eade142964d5482fe8d2109fb67a9556ae93d

naumenkogs commented at 11:31 am on August 26, 2021: member

utACK fa9eade142964d5482fe8d2109fb67a9556ae93d

MarcoFalke merged this on Aug 27, 2021

MarcoFalke closed this on Aug 27, 2021

MarcoFalke deleted the branch on Aug 27, 2021

sidhujag referenced this in commit d29e14a5e4 on Aug 28, 2021

DrahtBot locked this on Aug 27, 2022

Remove unused MaybeSetAddrName #22782

Conflicts