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

vasild commented at 12:58 pm on July 15, 2022: contributor

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.

fanquake added the label P2P on Jul 15, 2022

maflcko commented at 1:10 pm on July 15, 2022: member

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 force-pushed on Jul 15, 2022

vasild commented at 4:03 pm on July 15, 2022: contributor

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 commented at 5:05 pm on July 15, 2022: contributor

Concept ACK

jonatack commented at 5:32 pm on July 15, 2022: contributor

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

DrahtBot commented at 1:44 am on July 16, 2022: contributor

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 commented at 5:35 pm on July 16, 2022: member

nit: “host” seems preferable to “addr”

maflcko commented at 11:09 am on July 18, 2022: member

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

vasild force-pushed on Jul 18, 2022

vasild commented at 1:11 pm on July 18, 2022: contributor

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?

DrahtBot added the label Needs rebase on Jul 19, 2022

vasild force-pushed on Jul 19, 2022

vasild commented at 12:48 pm on July 19, 2022: contributor

b9ca6d0af3...9854d3a820: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jul 19, 2022

DrahtBot added the label Needs rebase on Jul 26, 2022

vasild force-pushed on Jul 27, 2022

vasild commented at 5:58 am on July 27, 2022: contributor

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

DrahtBot removed the label Needs rebase on Jul 27, 2022

vasild force-pushed on Jul 27, 2022

vasild commented at 11:07 am on July 27, 2022: contributor

0a798d56c2...cbfa859028: rebase due to conflicts

in src/netaddress.cpp:916 in cbfa859028 outdated

910@@ -916,25 +911,17 @@ std::vector<unsigned char> CService::GetKey() const
911     return key;
912 }
913 
914-std::string CService::ToStringPort() const
915+std::string CService::ToStringAddrPort() const
916 {
917-    return strprintf("%u", port);
918-}
919+    const auto port_str = strprintf("%u", port);

jonatack commented at 9:58 am on July 30, 2022:

cbfa859 would this avoid a copy, or is copy ellision doing it for us

0    const auto& port_str = strprintf("%u", port);

vasild commented at 3:02 pm on August 12, 2022:

There is no copy elision because there is no copy. The standard mandates that in this case there is no temporary to be copied and that the object should be constructed directly into port_str.

in src/qt/optionsdialog.cpp:413 in cbfa859028 outdated

405@@ -406,24 +406,21 @@ void OptionsDialog::updateProxyValidationState()
406 
407 void OptionsDialog::updateDefaultProxyNets()
408 {
409+    CNetAddr ui_proxy_netaddr;
410+    LookupHost(ui->proxyIp->text().toStdString(), ui_proxy_netaddr, /*fAllowLookup=*/false);
411+    const CService ui_proxy{ui_proxy_netaddr, ui->proxyPort->text().toUShort()};

jonatack commented at 10:08 am on July 30, 2022:

7f420c508968 TIL about toUShort() (https://doc.qt.io/qt-5/qlocale.html#toUShort) used for the first time in this codebase. It seems compatible with our minimum required version of qt 5.11.3.

vasild commented at 3:04 pm on August 12, 2022:

I wasn’t aware of toUShort() either. IIRC the auto-completion showed it to me ;-)

jonatack commented at 10:13 am on July 30, 2022: contributor

utACK cbfa85902839255a382a9f4556372ba5ceecf819 code review and debug build

Edit: might be clearest to prefix the PR title and the commit messages with refactor:

DrahtBot added the label Needs rebase on Aug 5, 2022

vasild force-pushed on Aug 12, 2022

vasild commented at 3:22 pm on August 12, 2022: contributor

cbfa859028...d4117c37f6: rebase due to conflicts

Invalidates ACK from @jonatack

DrahtBot removed the label Needs rebase on Aug 12, 2022

jonatack approved

jonatack commented at 8:16 pm on August 12, 2022: contributor

ACK d4117c37f6e4030af623d56c22bdb7a8c84a38c6 only change is a trivial rebase

DrahtBot added the label Needs rebase on Aug 26, 2022

vasild force-pushed on Aug 30, 2022

vasild commented at 9:44 am on August 30, 2022: contributor

d4117c37f6...e5d1916425: rebase due to conflicts

Invalidates ACK from @jonatack

DrahtBot removed the label Needs rebase on Aug 30, 2022

jonatack commented at 4:25 pm on August 30, 2022: contributor

re-ACK e5d191642503346ba6d9fe24f0c8f4414ffa00bc only change since my last review is a trivial rebase

achow101 commented at 7:45 pm on October 12, 2022: member

cc @LarryRuane @Empact

DrahtBot added the label Needs rebase on Oct 12, 2022

Empact commented at 7:33 pm on October 13, 2022: member

Code Review ACK e5d191642503346ba6d9fe24f0c8f4414ffa00bc

vasild force-pushed on Oct 14, 2022

vasild commented at 10:04 am on October 14, 2022: contributor

e5d1916425...caeec22664: rebase due to conflicts

Invalidates ACKs from @jonatack, @Empact

DrahtBot removed the label Needs rebase on Oct 14, 2022

DrahtBot added the label Needs rebase on Dec 12, 2022

scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]()

"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-

043b9de59a

net: remove CNetAddr::ToString() and use ToStringAddr() instead

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.

944a9de08a

net: remove CService::ToString() use ToStringAddrPort() instead

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()`.

96c791dd20

gui: simplify OptionsDialog::updateDefaultProxyNets()

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.

fd4f0f41e9

net: remove CService::ToStringPort()

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

c9d548c91f

vasild force-pushed on Dec 12, 2022

vasild commented at 11:01 am on December 12, 2022: contributor

caeec22664...c9d548c91f: rebase due to conflicts

Previously invalidated ACKs from @jonatack, @Empact

DrahtBot removed the label Needs rebase on Dec 12, 2022

sipa commented at 10:50 pm on February 13, 2023: member

utACK c9d548c91fb12fba516dee896f1f97692cfa2104

DrahtBot requested review from Empact on Feb 13, 2023

DrahtBot requested review from jonatack on Feb 13, 2023

jonatack approved

jonatack commented at 4:33 pm on February 14, 2023: contributor

re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 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 commented at 5:16 pm on February 17, 2023: contributor

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

achow101 commented at 6:21 pm on February 17, 2023: member

ACK c9d548c91fb12fba516dee896f1f97692cfa2104

achow101 merged this on Feb 17, 2023

achow101 closed this on Feb 17, 2023

sidhujag referenced this in commit 8241d6adaa on Feb 17, 2023

vasild deleted the branch on Feb 20, 2023

bitcoin locked this on Feb 20, 2024

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

Reviews

Conflicts