net: avoid overriding non-virtual ToString() in CService and use better naming #25619

vasild · 2022-07-15T12:58:23Z

Before this PR we had the somewhat confusing combination of methods:

CNetAddr::ToStringIP()
CNetAddr::ToString() (duplicate of the above)
CService::ToStringIPPort()
CService::ToString() (duplicate of the above, overrides a non-virtual method from CNetAddr)
CService::ToStringPort()

Avoid overriding non-virtual methods.

"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".

Change the above to:

CNetAddr::ToStringAddr()
CService::ToStringAddrPort()

The changes touch a lot of files, but are mostly mechanical.

maflcko · 2022-07-15T13:10:22Z

ToStringPort should probably be removed. The only place where it is used in the gui should probably use ToStringIPPort to properly display IPV6, no?

vasild · 2022-07-15T16:03:10Z

5b3c6bc788..7f4aa41772: simplify OptionsDialog::updateDefaultProxyNets() to not use CService::ToStringPort() and drop the latter.

ToStringPort should probably be removed.

Done! :)

The only place where it is used in the gui should probably use ToStringIPPort to properly display IPV6, no?

To be honest, I stared at that code in the GUI for a few minutes before opening this PR, trying to figure out what is going on and "how is it possible that it displays ipv6addr:port without [ ] !?", but it only used that (non-standard) representation to compare with another string internally, never displayed it. It could have used any other separator, e.g. "ipv6addr-port". Anyway, we have CService::operator==() for that, so I changed it.

jonatack · 2022-07-15T17:05:37Z

Concept ACK

jonatack · 2022-07-15T17:32:29Z

Light ACK, skimmed commits, debug build and unit tests. Will review more thoroughly.

DrahtBot · 2022-07-16T01:44:51Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, jonatack, LarryRuane, achow101
Stale ACK	Empact

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27071 (Handle CJDNS from LookupSubNet() by vasild)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26312 (Remove Sock::Get() and Sock::Sock() by vasild)
#26261 (p2p: cleanup LookupIntern, Lookup and LookupHost by brunoerg)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#21878 (Make all networking code mockable by vasild)

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.

luke-jr · 2022-07-16T17:35:14Z

nit: "host" seems preferable to "addr"

maflcko · 2022-07-18T11:09:00Z

Maybe the scripted-diff can be made first to avoid repeatedly changing the same line of code several times?

vasild · 2022-07-18T13:11:27Z

7f4aa41772...b9ca6d0af3: do the scripted-diff first

Cumulative diff of the PR is unchanged before and after this force push:
before: git diff 85b601e043..7f4aa41772
after: git diff d806407173..b9ca6d0af3
(identical)

Maybe the scripted-diff can be made first to avoid repeatedly changing the same line of code several times?

I actually made it this way originally but then decided it is better to have the scripted-diff at the end so that it is optional and if it is contentious, then just drop the last commit (the scripted-diff). Changed now, since there are two more commits on top. For reviewing it shouldn't matter much since, I guess, reviewers are not staring carefully at the entire diff of scripted-diffs. It would matter for history chasing - to reduce the number of hops ("git blame"s) one has to do.

nit: "host" seems preferable to "addr"

Could be, can you elaborate a bit why? I did not change it yet because I am somewhat in favor of "addr" because "host" may be misunderstood to return a host (aka hostname): e.g. ToStringHost() could be misunderstood to return bar.foo.com instead of 34.206.39.153. I am fine either way, what do others think?

vasild · 2022-07-19T12:48:16Z

b9ca6d0af3...9854d3a820: rebase due to conflicts

vasild · 2022-07-27T05:58:13Z

9854d3a820...0a798d56c2: rebase due to conflicts

vasild · 2022-07-27T11:07:11Z

0a798d56c2...cbfa859028: rebase due to conflicts

Empact · 2022-10-13T19:33:21Z

Code Review ACK e5d1916

vasild · 2022-10-14T10:04:10Z

e5d1916425...caeec22664: rebase due to conflicts

Invalidates ACKs from @jonatack, @Empact

"IP" stands for "Internet Protocol". "IP address" is sometimes shortened to just "IP" or "address". However, Tor or I2P addresses are not "IP addresses", nor "IPs". Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or I2P addresses: `CService::ToStringIPPort()` -> `CService::ToStringAddrPort()` `CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()` -BEGIN VERIFY SCRIPT- sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src) sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src) -END VERIFY SCRIPT-

Both methods do the same thing, so simplify to having just one. Further, `CService` inherits `CNetAddr` and `CService::ToString()` overrides `CNetAddr::ToString()` but the latter is not virtual which may be confusing. Avoid such a confusion by not having non-virtual methods with the same names in inheritance.

Both methods do the same thing, so simplify to having just one. `ToString()` is too generic in this case and it is unclear what it does, given that there are similar methods: `ToStringAddr()` (inherited from `CNetAddr`), `ToStringPort()` and `ToStringAddrPort()`.

Do not create strings and compare them to check if one `addr:port` equals another. Use `CService::operator==()` instead. `strDefaultProxyGUI` was assigned the same value 3 times. Instead save it in `const CService ui_proxy` at the beginning of the function.

It is used only internally in `CService::ToStringAddrPort()`.

vasild · 2022-12-12T11:01:40Z

caeec22664...c9d548c91f: rebase due to conflicts

Previously invalidated ACKs from @jonatack, @Empact

sipa · 2023-02-13T22:50:39Z

utACK c9d548c

jonatack

re-ACK c9d548c only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests

LarryRuane

ACK c9d548c
rebased to master, built, ran tests, ran bitcoind mainnet, code reviewed except I didn't completely understand the qt changes.

achow101 · 2023-02-17T18:21:36Z

ACK c9d548c

…CServic…

fanquake added the P2P label Jul 15, 2022

vasild force-pushed the CNetAddr_ToString branch from 19e1f8a to 5b3c6bc Compare July 15, 2022 13:16

vasild force-pushed the CNetAddr_ToString branch from 7f4aa41 to b9ca6d0 Compare July 18, 2022 12:58

DrahtBot added the Needs rebase label Jul 19, 2022

vasild force-pushed the CNetAddr_ToString branch from b9ca6d0 to 9854d3a Compare July 19, 2022 12:48

DrahtBot removed the Needs rebase label Jul 19, 2022

DrahtBot added the Needs rebase label Jul 26, 2022

vasild force-pushed the CNetAddr_ToString branch from 9854d3a to 0a798d5 Compare July 27, 2022 05:58

DrahtBot removed the Needs rebase label Jul 27, 2022

vasild force-pushed the CNetAddr_ToString branch from 0a798d5 to cbfa859 Compare July 27, 2022 11:07

DrahtBot mentioned this pull request Jul 30, 2022

addrman: Use system time instead of adjusted network time #24662

Merged

DrahtBot added the Needs rebase label Oct 12, 2022

vasild force-pushed the CNetAddr_ToString branch from e5d1916 to caeec22 Compare October 14, 2022 10:03

DrahtBot mentioned this pull request Oct 14, 2022

Remove Sock::Get() and Sock::Sock() #26312

Merged

DrahtBot removed the Needs rebase label Oct 14, 2022

DrahtBot mentioned this pull request Oct 29, 2022

p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #25923

Closed

DrahtBot mentioned this pull request Nov 28, 2022

net, refactor: Kill proxy globals #26590

Closed

DrahtBot mentioned this pull request Dec 6, 2022

refactor: Continue moving application data from CNode to Peer #26621

Closed

DrahtBot added the Needs rebase label Dec 12, 2022

vasild added 5 commits December 12, 2022 11:48

net: remove CService::ToStringPort()

c9d548c

It is used only internally in `CService::ToStringAddrPort()`.

vasild force-pushed the CNetAddr_ToString branch from caeec22 to c9d548c Compare December 12, 2022 11:00

DrahtBot removed the Needs rebase label Dec 12, 2022

DrahtBot mentioned this pull request Dec 17, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot mentioned this pull request Feb 10, 2023

Handle CJDNS from LookupSubNet() #27071

Merged

DrahtBot requested review from Empact and jonatack February 13, 2023 22:50

jonatack approved these changes Feb 14, 2023

View reviewed changes

LarryRuane reviewed Feb 17, 2023

View reviewed changes

achow101 merged commit 35fbc97 into bitcoin:master Feb 17, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 17, 2023

Merge bitcoin#25619: net: avoid overriding non-virtual ToString() in …

8241d6a

…CServic…

vasild deleted the CNetAddr_ToString branch February 20, 2023 09:48

bitcoin locked and limited conversation to collaborators Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: avoid overriding non-virtual ToString() in CService and use better naming #25619

net: avoid overriding non-virtual ToString() in CService and use better naming #25619

vasild commented Jul 15, 2022 •

edited

maflcko commented Jul 15, 2022

vasild commented Jul 15, 2022

jonatack commented Jul 15, 2022

jonatack commented Jul 15, 2022

DrahtBot commented Jul 16, 2022 •

edited

luke-jr commented Jul 16, 2022

maflcko commented Jul 18, 2022

vasild commented Jul 18, 2022

vasild commented Jul 19, 2022

vasild commented Jul 27, 2022

vasild commented Jul 27, 2022

Empact commented Oct 13, 2022

vasild commented Oct 14, 2022

vasild commented Dec 12, 2022

sipa commented Feb 13, 2023

jonatack left a comment

LarryRuane left a comment

achow101 commented Feb 17, 2023

net: avoid overriding non-virtual ToString() in CService and use better naming #25619

net: avoid overriding non-virtual ToString() in CService and use better naming #25619

Conversation

vasild commented Jul 15, 2022 • edited

maflcko commented Jul 15, 2022

vasild commented Jul 15, 2022

jonatack commented Jul 15, 2022

jonatack commented Jul 15, 2022

DrahtBot commented Jul 16, 2022 • edited

Reviews

Conflicts

luke-jr commented Jul 16, 2022

maflcko commented Jul 18, 2022

vasild commented Jul 18, 2022

vasild commented Jul 19, 2022

vasild commented Jul 27, 2022

vasild commented Jul 27, 2022

Empact commented Oct 13, 2022

vasild commented Oct 14, 2022

vasild commented Dec 12, 2022

sipa commented Feb 13, 2023

jonatack left a comment

Choose a reason for hiding this comment

LarryRuane left a comment

Choose a reason for hiding this comment

achow101 commented Feb 17, 2023

vasild commented Jul 15, 2022 •

edited

DrahtBot commented Jul 16, 2022 •

edited