Skip to content

Remove unused MaybeSetAddrName#22782

Merged
maflcko merged 3 commits intobitcoin:masterfrom
maflcko:2108-noMaybeSetAddrName
Aug 27, 2021
Merged

Remove unused MaybeSetAddrName#22782
maflcko merged 3 commits intobitcoin:masterfrom
maflcko:2108-noMaybeSetAddrName

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 23, 2021 •
edited by fanquake
Loading

.

hebasto
hebasto approved these changes Aug 23, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa2d049bfe4f1365011e440adaeb096ddd544143.

Indeed, on master (dbcb574) the CNode::addrName member has been assigned with a non-empty value in the CNode::CNode constructor:

bitcoin/src/net.cpp

Line 2978 in dbcb574

addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn;

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

@hebasto hebasto mentioned this pull request Aug 23, 2021
Open
35 tasks
promag
promag reviewed Aug 23, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa2d049bfe4f1365011e440adaeb096ddd544143.

@fanquake fanquake requested a review from jnewbery August 24, 2021 02:57
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 24, 2021 •
edited
Loading

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
Copy link
Contributor

jnewbery commented Aug 24, 2021

Concept ACK.

This was introduced in f9f5cfc, 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
fa82f4e 
This logic is a no-op since it was introduced in commit
f9f5cfc.

m_addr_name is never initialized to the empty string, because
ToStringIPPort never returns an empty string.
@maflcko maflcko force-pushed the 2108-noMaybeSetAddrName branch from fa2d049 to fa973e4 Compare August 24, 2021 17:40
hebasto
hebasto approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot DrahtBot mentioned this pull request Aug 24, 2021
Merged
jnewbery
jnewbery reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa973e4d2f50e952fed0aeba7a1a6132f9968a7c

Couple of suggestions for follow-ups.

src/net.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified further to:

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -415,9 +415,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
             }
             // 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.
-            LOCK(cs_vNodes);
-            CNode* pnode = FindNode(static_cast<CService>(addrConnect));
-            if (pnode) {
+            if (FindNode(static_cast<CService>(addrConnect)));
                 LogPrintf("Failed to open new connection, already connected\n");
                 return nullptr;
             }

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

@DrahtBot DrahtBot mentioned this pull request Aug 25, 2021
Closed
@maflcko maflcko force-pushed the 2108-noMaybeSetAddrName branch from fa973e4 to fabbe64 Compare August 26, 2021 08:28
@maflcko
Copy link
Member Author

maflcko commented Aug 26, 2021

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

@maflcko maflcko force-pushed the 2108-noMaybeSetAddrName branch from fabbe64 to fa9eade Compare August 26, 2021 08:45
@DrahtBot DrahtBot mentioned this pull request Aug 26, 2021
Closed
12 tasks
@jnewbery
Copy link
Contributor

jnewbery commented Aug 26, 2021

Code review ACK fa9eade

@naumenkogs
Copy link
Contributor

naumenkogs commented Aug 26, 2021

utACK fa9eade

@maflcko maflcko merged commit 33707a2 into bitcoin:master Aug 27, 2021
@maflcko maflcko deleted the 2108-noMaybeSetAddrName branch August 27, 2021 09:54
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2021
Merge bitcoin#22782: Remove unused MaybeSetAddrName
d29e14a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@jnewbery jnewbery jnewbery left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @jnewbery @naumenkogs @promag @hebasto