Skip to content

Remove unused MaybeSetAddrName #22782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 27, 2021
Merged

Remove unused MaybeSetAddrName #22782

merged 3 commits into from
Aug 27, 2021

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 fa2d049.

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
36 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 fa2d049.

@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

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 fa973e4, 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 fa973e4

Couple of suggestions for follow-ups.

src/net.cpp
if (pnode)
{
pnode->MaybeSetAddrName(std::string(pszDest));
if (pnode) {
Copy link
Contributor

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

Code review ACK fa9eade

@naumenkogs
Copy link
Member

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