To improve readability instead of returning a bool and passing stuff by reference, this PR changes:
LookupHost to return std::vector<CNetAddr>
LookupHost to return std::optional<CNetAddr>
Lookup to return std::vector<CService>
Lookup to return std::optional<CService>.
LookupIntern to return std::vector<CNetAddr>
As discussed in #26078, it would be better to avoid using optional in some cases, but for specific Lookup and LookupHost functions it’s necessary to use optional to verify if they were able to catch some data from their overloaded function.
DrahtBot added the label
P2P
on Oct 5, 2022
stickies-v
commented at 10:23 am on October 6, 2022:
contributor
Wouldn’t a std::optional approach make more sense here? And I think it’d be appropriate to update all Lookup functions at once, instead of just LookupInternal?
brunoerg
commented at 4:15 pm on October 6, 2022:
contributor
Wouldn’t a std::optional approach make more sense here?
Given my experience in #26078, I’m not sure about using std::optional here. At some point, I’d have to check if it has_value, if so, I’d have to check the content of the vector anyway. Not sure about the benefits and code quality here.
And I think it’d be appropriate to update all Lookup functions at once, instead of just LookupInternal?
I agree, I’d planned to do it in a follow-up (tried to keep this PR simple/easy to review) but could do it here, no problem.
stickies-v
commented at 10:32 am on October 7, 2022:
contributor
Not sure about the benefits and code quality here.
I think you’re right. At first sight it looked like in the current implementation there was the need to distinguish between invalid parameters (!ContainsNoNUL(name)) and a name that just wouldn’t resolve to any addresses (empty vector), but upon further inspection that seems to not be true.
but could do it here, no problem.
I think the main benefit of doing them all together is to ensure they all adhere to the same consistent interface. From a review point of view, that’s most efficient too, and it shouldn’t be a huge change anyway.
brunoerg
commented at 12:57 pm on October 7, 2022:
contributor
I think the main benefit of doing them all together is to ensure they all adhere to the same consistent interface. From a review point of view, that’s most efficient too, and it shouldn’t be a huge change anyway.
You’re right, I am gonna turn this PR to draft to work on it.
brunoerg marked this as a draft
on Oct 7, 2022
brunoerg force-pushed
on Oct 10, 2022
brunoerg renamed this:
p2p: return `std::vector<CNetAddr>` in `LookupIntern`
p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`
on Oct 10, 2022
brunoerg force-pushed
on Oct 10, 2022
brunoerg force-pushed
on Oct 10, 2022
DrahtBot
commented at 0:09 am on October 11, 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.
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:
#27679 (ZMQ: Support UNIX domain sockets by pinheadmz)
#27375 (net: support unix domain sockets for -proxy and -onion by pinheadmz)
#27071 (Handle CJDNS from LookupSubNet() by vasild)
#26078 (p2p: return CSubNet in LookupSubNet by brunoerg)
#25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)
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.
brunoerg force-pushed
on Oct 11, 2022
brunoerg marked this as ready for review
on Oct 11, 2022
stickies-v
commented at 10:26 am on October 19, 2022:
Developer notes state: “If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.”
Here, and in a few other instances too
0 if(addresses.empty()) return std::nullopt;
in
src/net.cpp:1481
in
e4e03e9935outdated
1477@@ -1479,7 +1478,8 @@ void CConnman::ThreadDNSAddressSeed()
1478 continue;
1479 }
1480 unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
1481- if (LookupHost(host, vIPs, nMaxIPs, true)) {
1482+ const std::vector<CNetAddr> vIPs{LookupHost(host, nMaxIPs, true)};
stickies-v
commented at 10:26 am on October 19, 2022:
nit: consistent naming (you could do all the renaming in a first, new commit to minimize the renaming diff in subsequent commits)
123 /**
124 * Resolve a host string to its first corresponding network address.
125 *
126- * @see LookupHost(const std::string&, std::vector<CNetAddr>&, uint16_t, bool, DNSLookupFn)
127+ * @returns Whether or not the specified host string successfully resolved to
128+ * any resulting network addresses.
stickies-v
commented at 11:49 am on October 19, 2022:
0 * [@returns](/bitcoin-bitcoin/contributor/returns/) The resulting network addresses to which the specified host
1 * string resolved.
stickies-v
commented at 11:59 am on October 19, 2022:
0 std::vector<CService> services;
in
src/netbase.cpp:237
in
e4e03e9935outdated
231@@ -235,8 +232,9 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault, DNSLookupF
232 CService addr;
233 // "1.2:345" will fail to resolve the ip, but will still set the port.
234 // If the ip fails to resolve, re-init the result.
235- if(!Lookup(name, addr, portDefault, false, dns_lookup_function))
236- addr = CService();
237+ const std::optional<CService> addr_opt{Lookup(name, portDefault, false, dns_lookup_function)};
238+ if (addr_opt.has_value())
239+ addr = addr_opt.value();
stickies-v
commented at 1:07 pm on October 19, 2022:
This can be simplified:
0diff --git a/src/netbase.cpp b/src/netbase.cpp
1index 534dafdb4..2d4341a4c 100644
2--- a/src/netbase.cpp
3+++ b/src/netbase.cpp
4@@ -229,13 +229,9 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault, DNSLookupF
5 if (!ContainsNoNUL(name)) {
6 return {};
7 }
8- CService addr;
9 // "1.2:345" will fail to resolve the ip, but will still set the port.
10 // If the ip fails to resolve, re-init the result.
11- const std::optional<CService> addr_opt{Lookup(name, portDefault, false, dns_lookup_function)};
12- if (addr_opt.has_value())
13- addr = addr_opt.value();
14- return addr;
15+ return Lookup(name, portDefault, false, dns_lookup_function).value_or(CService{});
16 }
1718 /** SOCKS version */
brunoerg
commented at 1:48 pm on October 20, 2022:
Great, gonna implement it! Simpler!
in
src/netbase.cpp:235
in
e4e03e9935outdated
231@@ -235,8 +232,9 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault, DNSLookupF
232 CService addr;
233 // "1.2:345" will fail to resolve the ip, but will still set the port.
234 // If the ip fails to resolve, re-init the result.
235- if(!Lookup(name, addr, portDefault, false, dns_lookup_function))
236- addr = CService();
237+ const std::optional<CService> addr_opt{Lookup(name, portDefault, false, dns_lookup_function)};
stickies-v
commented at 1:09 pm on October 19, 2022:
stickies-v
commented at 1:29 pm on October 19, 2022:
I think this comment was not meant to be committed?
brunoerg
commented at 1:54 pm on October 20, 2022:
Sorry, I removed this PR from draft before pushing more changes.. my fault!
stickies-v
commented at 1:33 pm on October 19, 2022:
contributor
Approach ACK. Left a few comments and nits - a lot of the std::optional comments recur in many different places in this PR, I didn’t necessarily always point them all out.
brunoerg marked this as a draft
on Oct 20, 2022
brunoerg force-pushed
on Oct 20, 2022
brunoerg marked this as ready for review
on Oct 20, 2022
brunoerg force-pushed
on Oct 20, 2022
brunoerg
commented at 5:09 pm on October 20, 2022:
contributor
Thanks @stickies-v for your valuable review, just addressed your suggestions!
Ready for review!
theStack
commented at 12:40 pm on January 23, 2023:
contributor
Concept ACK
Looks like the first commit still uses the old naming scheme and probably should be adapted (s/vIP/addresses/)?
brunoerg force-pushed
on Jan 23, 2023
brunoerg
commented at 2:05 pm on January 23, 2023:
contributor
Looks like the first commit still uses the old naming scheme and probably should be adapted (s/vIP/addresses/)?
Done, force-pushed addressing it!
in
src/netbase.h:123
in
eb7b7e2b42outdated
125 *
126- * @see LookupHost(const std::string&, std::vector<CNetAddr>&, uint16_t, bool, DNSLookupFn)
127+ * @returns The resulting network addresses to which the specified host
128+ * string resolved.
129+ *
130+ * @see LookupHost(const std::string&, uint16_t, bool, DNSLookupFn)
stickies-v
commented at 2:22 pm on January 23, 2023:
stickies-v
commented at 11:27 am on January 24, 2023:
You’re right - sorry I didn’t test my suggestion. nit: std::make_optional(services.front()) is also possible, to avoid redefining the type (only if you’re force pushing for something else)
brunoerg
commented at 11:57 am on January 24, 2023:
theStack
commented at 4:17 pm on January 23, 2023:
contributor
nit: could introduce the addresses vector later (right before the loop) in LookupIntern and return the (0- or 1-sized) results before via list initialization:
0diff --git a/src/netbase.cpp b/src/netbase.cpp
1index d9bb8ad0f..f2e48ee68 100644
2--- a/src/netbase.cpp
3+++ b/src/netbase.cpp
4@@ -134,11 +134,7 @@ std::vector<std::string> GetNetworkNames(bool append_unroutable)
5 6 static std::vector<CNetAddr> LookupIntern(const std::string& name, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function)
7 {
8- std::vector<CNetAddr> addresses;
9-
10- if (!ContainsNoNUL(name)) {
11- return addresses;
12- }
13+ if (!ContainsNoNUL(name)) return {};
1415 {
16 CNetAddr addr;
17@@ -148,12 +144,11 @@ static std::vector<CNetAddr> LookupIntern(const std::string& name, unsigned int
18 // getaddrinfo to decode them and it wouldn't make sense to resolve
19 // them, we return a network address representing it instead. See
20 // CNetAddr::SetSpecial(const std::string&) for more details.
21- if (addr.SetSpecial(name)) {
22- addresses.push_back(addr);
23- return addresses;
24- }
25+ if (addr.SetSpecial(name)) return {addr};
26 }
2728+ std::vector<CNetAddr> addresses;
29+
30 for (const CNetAddr& resolved : dns_lookup_function(name, fAllowLookup)) {
31 if (nMaxSolutions > 0 && addresses.size() >= nMaxSolutions) {
32 break;
brunoerg force-pushed
on Jan 23, 2023
brunoerg
commented at 11:44 pm on January 23, 2023:
contributor
stickies-v
commented at 1:08 pm on January 24, 2023:
contributor
ACK24adc24a3
Minor suggestion for future PRs: there’s quite a bit of renaming happening (which I like), I think putting them all in a separate (first) commit (ideally scripted-diff if feasible) reduces the review burden for the actual refactoring. In some commits, most of the diff now is just due to renaming.
theStack
commented at 4:18 pm on January 26, 2023:
nit: should either return in the same line as the if-condition or put braces around (OTOH, probably not worth it chaning since it’s removed in a later commit anyways)
theStack
commented at 4:25 pm on January 26, 2023:
nit: Seems like this is a slight change in the logic. On master, we don’t call LookupHost if the address is an empty string, on the PR it is called unconditionally and only then the empty() check is done. The resulting behaviour is the same though, since LookupHost returns early anways if the passed address is empty. Could probably remove the i->first.empty() check here?
brunoerg
commented at 8:12 pm on February 1, 2023:
Yes, you’re right!
stickies-v
commented at 11:49 am on March 30, 2023:
The comment was on an older commit, so apologies if I’m missing something, but I’m not sure it’s fully addressed? In if (i->first.empty() || (addr.has_value() && addr->IsBindAny())), I think the first i->first.empty() is indeed still superfluous as theStack pointed out.
theStack approved
theStack
commented at 4:29 pm on January 26, 2023:
contributor
Looks good overall, and I’m positive that these changes don’t change behaviour. Left some more minor stylistic suggestions below, but I wouldn’t consider any of them blocking (some of them affect changes that are removed in a later commit anyways). I’d also probably take use of the auto keyword a little more often if applicable, but since that’s largely a matter of personal taste, there is not too much point on bikeshedding here.
DrahtBot added the label
Needs rebase
on Jan 31, 2023
brunoerg force-pushed
on Feb 1, 2023
brunoerg
commented at 8:36 pm on February 1, 2023:
contributor
Since I had to rebase it, I took the oportunity to address last @theStack’s review. Thanks!
DrahtBot removed the label
Needs rebase
on Feb 1, 2023
theStack approved
theStack
commented at 2:44 pm on February 2, 2023:
contributor
re-ACK3bb0e074525aa0bb356960ecf38086e8671a78d4
in
src/netbase.h:121
in
3bb0e07452outdated
123 /**
124 * Resolve a host string to its first corresponding network address.
125 *
126- * @see LookupHost(const std::string&, std::vector<CNetAddr>&, uint16_t, bool, DNSLookupFn)
127+ * @returns The resulting network addresses to which the specified host
128+ * string resolved or 'false' if it doesn't contain a value.
stickies-v
commented at 5:13 pm on February 3, 2023:
‘false’ is not the same as std::nullopt - and “not contain a value” is synonymous for std::nullopt
0 * string resolved or std::nullopt if host does not resolve to an address.
stickies-v
commented at 5:15 pm on February 3, 2023:
also open to “empty” or “no value” instead of “std::nullopt”. Just not “false”.
brunoerg
commented at 5:35 pm on February 3, 2023:
Yes, seems better, thanks!
brunoerg force-pushed
on Feb 3, 2023
brunoerg
commented at 5:41 pm on February 3, 2023:
contributor
Force-pushed addressing last @stickies-v’s review.
That looks dangerous – generally I think we shouldn’t ever call .value() on a std::optional object without checking for .has_value() first (similar as for pointer dereferences), potentially risking a crash. In this case the GUI flow seems to ensure that the IP passed here is valid at an earlier stage (via the ProxyAddressValidator) so LookupHostshould always return a value, but I’d prefer to be rather safe than sorry.
Hmm, std::optional without the template parameter? I guess the compiler is smart enough to deduct it, but in other places you used std::optional<CNetAddr>, better add the template parameter here for consistency. Or use const auto addr = ... eveywhere?
The previous code would have returned a default constructed CNetAddr if lookup failed. The new code will throw std::bad_optional_access because it will call addr.value() when the optional has no value. I guess this will fix it:
0return addr.value_or(CNetAddr{});
BOOST_CHECK_MESSAGE() will continue execution if the expression is false. BOOST_REQUIRE_MESSAGE() will stop the execution of the test.
or replace BOOST_CHECK() with BOOST_REQUIRE(). Maybe that was the original intention of this? Looks to me that a successful resolve is a precondition for the test to continue/make sense.
319- if (i->first.empty() || (LookupHost(i->first, addr, false) && addr.IsBindAny())) {
320+ const std::optional<CNetAddr> addr{LookupHost(i->first, false)};
321+ if (addr.has_value() && addr->IsBindAny()) {
322 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
323 }
324 boundSockets.push_back(bind_handle);
The i->first.empty() is missing in the new code. In this case the old code would have printed the warning, but the new code will not because LookupHost("", false) will return an optional without value.
0const std::optional<CNetAddr> addr{LookupHost(i->first, false)};
1if (i->first.empty() && addr.has_value() && addr->IsBindAny()) {
2 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
If the string (i->first) is empty then addr.has_value() will never be true. Thus this condition will never be true and the warning will never be printed. It should be:
123 /**
124 * Resolve a host string to its first corresponding network address.
125 *
126- * @see LookupHost(const std::string&, std::vector<CNetAddr>&, uint16_t, bool, DNSLookupFn)
127+ * @returns The resulting network addresses to which the specified host
128+ * string resolved or 'empty' if host does not resolve to an address.
stickies-v
commented at 12:16 pm on March 30, 2023:
This docstring is updated in ab4257be34209fd61efb409593dc7aa21dba0fb7, but there it still returns a bool. The signature is only updated to return std::optional<CNetAddr> in 30efd6d36e24c83f5c094e30f8cb25e66e913d73
Moreover, the function never returns addresses, it’s only ever singular. I also don’t like the phrasing of ’empty’, so would prefer updating this to:
0 * [@returns](/bitcoin-bitcoin/contributor/returns/) The resulting network address to which the specified host
1 * string resolved or std::nullopt if host does not resolve to an address.
stickies-v approved
stickies-v
commented at 12:45 pm on March 30, 2023:
contributor
re-ACKec28c89ef69cd3fbb1a82785c841d303b2bf4826
I think my comments aren’t too controversial to address but they only affect documentation and style/readability so I’m happy with the status quo and a follow-up, too. We’ve had quite a bit of back and forth already on this PR.
DrahtBot requested review from stickies-v
on Mar 30, 2023
DrahtBot removed review request from stickies-v
on Mar 30, 2023
brunoerg
commented at 12:57 pm on March 31, 2023:
contributor
Thanks, @stickies-v for your review. Will address your latest suggestions in a follow-up PR.
DrahtBot added the label
CI failed
on Apr 22, 2023
brunoerg force-pushed
on Apr 26, 2023
brunoerg force-pushed
on Apr 26, 2023
brunoerg
commented at 8:57 am on April 26, 2023:
contributor
I had to touch bench code to fix an error CI pointed out, so I took the moment to address some suggestions from @stickies-v.
stickies-v
commented at 9:27 am on April 26, 2023:
contributor
re-ACK4eee95e57
Verified that the only changes are a refactoring to bench, removing now unnecessary helper functions ResolveIP and ResolveService, and incorporating my previous outstanding review comments.
DrahtBot requested review from theStack
on Apr 26, 2023
DrahtBot requested review from vasild
on Apr 26, 2023
DrahtBot removed the label
CI failed
on Apr 26, 2023
vasild approved
vasild
commented at 10:27 am on May 12, 2023:
contributor
No stopper, but I think there should be really a good reason to remove existing unit-test checks. For example, if (for whatever reason) LookupHost fails and hence returns std::nullopt, we’d now get a more cryptic error message without seeing the direct line number of the cause:
0unknown location(0): fatal error: in "net_tests/cnetaddr_basic": std::bad_optional_access: bad_optional_access
1test/net_tests.cpp(133): last checkpoint: "cnetaddr_basic" test entry
compared to an explicit
0test/net_tests.cpp(138): fatal error: in "net_tests/cnetaddr_basic": critical check LookupHost("x0.0.0.0", addr, false) has failed
(tested by replacing “0.0.0.0” with “x0.0.0.0” both on the PR and master branch). As we pass fixed valid IP addresses here and the probability of this failing is practically zero, I think your approach is still okay though (I’d only see it as problematic if it could lead to undefined behaviour, e.g. by using std::optional’s operator* instead of .value()).
I agree that harder to debug test failure messages are to be avoided, but given the nature of the statement, I also don’t mind the current approach to avoid having to create a separate var for the optional and then the CNetAddr, even if it’s just the one line.
It’s a shame the BOOST_ functions don’t support pass-through of return values, which would have solved this elegantly. We could use our Assert function (requires #include <util/check.h>), which yields an equally useful error message:
The alternative to keep BOOST_REQUIRE is creating one more variable but I don’t think it’s worth, would have to do it in many places. Using Assert could also be an alternative, is there any harm of using it? Perhaps import util/check just for it?
Since it’s a non-blocker I’m pushing addressing all lastest nits, except this one, to not delay and cause futher rebases conflicts.
theStack approved
theStack
commented at 10:32 pm on May 22, 2023:
contributor
re-ACK4eee95e57bb6f773bcaeb405bca949f158a62134
Left two comments below, one nit and one “thinking out loud what could happen in the worst case”, both no stoppers.
stickies-v
commented at 2:01 pm on May 26, 2023:
contributor
@brunoerg are you going to address nits or leave as is? I’d really like to get this merged asap to prevent further rebase conflicts. Happy to quickly re-ack nits too, though.
p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern`5c1774a563
p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`7799eb125b
p2p, refactor: return vector/optional<CService> in `Lookup`34bcdfc6a6
p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`5c832c3820
brunoerg
commented at 4:42 pm on May 26, 2023:
contributor
@stickies-v I’m addressing them at this moment, will push soon.
brunoerg force-pushed
on May 26, 2023
brunoerg
commented at 5:04 pm on May 26, 2023:
contributor
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-09 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
