p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`

brunoerg commented at 4:35 pm on October 5, 2022: contributor

Continuation of #26078.

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

@stickies-v

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.

in src/netbase.cpp:137 in 312415f070 outdated

131@@ -132,12 +132,12 @@ std::vector<std::string> GetNetworkNames(bool append_unroutable)
132     return names;
133 }
134 
135-static bool LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function)
136+static std::vector<CNetAddr> LookupIntern(const std::string& name, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function)
137 {
138-    vIP.clear();
139+    std::vector<CNetAddr> vIP;

stickies-v commented at 10:31 am on October 7, 2022:

We don’t really use this naming style anymore for new code, perhaps just addresses?

0    std::vector<CNetAddr> addresses;

brunoerg commented at 2:00 pm on October 7, 2022:

Cool, gonna adopt it

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

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.

Type	Reviewers
ACK	stickies-v, theStack, achow101
Stale ACK	vasild

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

in src/netbase.cpp:190 in e4e03e9935 outdated

200-        return false;
201-    addr = vIP.front();
202-    return true;
203+    const std::vector<CNetAddr> addresses{LookupHost(name, 1, fAllowLookup, dns_lookup_function)};
204+    if(addresses.empty())
205+        return std::nullopt;

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 e4e03e9935 outdated

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)

0            const std::vector<CNetAddr> addresses{LookupHost(host, nMaxIPs, true)};

stickies-v commented at 10:31 am on October 19, 2022:

nit (and in a few other places)

0            const auto vIPs{LookupHost(host, nMaxIPs, true)};

in src/netbase.cpp:204 in e4e03e9935 outdated

224-        return false;
225-    vAddr.resize(vIP.size());
226-    for (unsigned int i = 0; i < vIP.size(); i++)
227-        vAddr[i] = CService(vIP[i], port);
228-    return true;
229+    const std::vector<CNetAddr> IPs{LookupIntern(hostname, nMaxSolutions, fAllowLookup, dns_lookup_function)};

stickies-v commented at 10:27 am on October 19, 2022:

nit: consistent naming

0    const std::vector<CNetAddr> addresses{LookupIntern(hostname, nMaxSolutions, fAllowLookup, dns_lookup_function)};

in src/netbase.h:121 in e4e03e9935 outdated

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.

in src/netbase.cpp:207 in e4e03e9935 outdated

227-        vAddr[i] = CService(vIP[i], port);
228-    return true;
229+    const std::vector<CNetAddr> IPs{LookupIntern(hostname, nMaxSolutions, fAllowLookup, dns_lookup_function)};
230+    if (IPs.empty())
231+        return addresses;
232+    std::vector<CService> vAddr;

stickies-v commented at 11:59 am on October 19, 2022:

unused

in src/netbase.cpp:196 in e4e03e9935 outdated

207 }
208 
209-bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function)
210+std::vector<CService> Lookup(const std::string& name, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function)
211 {
212+    std::vector<CService> addresses;

stickies-v commented at 11:59 am on October 19, 2022:

0    std::vector<CService> services;

in src/netbase.cpp:237 in e4e03e9935 outdated

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 }
17 
18 /** SOCKS version */

brunoerg commented at 1:48 pm on October 20, 2022:

Great, gonna implement it! Simpler!

in src/netbase.cpp:235 in e4e03e9935 outdated

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:

0    const std::optional<CService> addr_opt{Lookup(name, portDefault, /*fAllowLookup=*/false, dns_lookup_function)};

in src/net.cpp:139 in e4e03e9935 outdated

138+        const std::optional<CService> bind_addr{Lookup(bind_arg, dummy_port, /*fAllowLookup=*/false)};
139+        if (bind_addr.has_value()) {
140+            if (bind_addr.value().GetPort() != dummy_port) {
141+                return bind_addr.value().GetPort();
142             }
143         }

stickies-v commented at 1:17 pm on October 19, 2022:

0        if (bind_addr.has_value() && bind_addr->GetPort() != dummy_port) return bind_addr->GetPort();

in src/init.cpp:1373 in e4e03e9935 outdated

1368@@ -1369,9 +1369,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1369     }
1370 
1371     for (const std::string& strAddr : args.GetArgs("-externalip")) {
1372-        CService addrLocal;
1373-        if (Lookup(strAddr, addrLocal, GetListenPort(), fNameLookup) && addrLocal.IsValid())
1374-            AddLocal(addrLocal, LOCAL_MANUAL);
1375+        const std::optional<CService> addrLocal{Lookup(strAddr, GetListenPort(), fNameLookup)};
1376+        if (addrLocal.has_value() && addrLocal.value().IsValid())

stickies-v commented at 1:21 pm on October 19, 2022:

nit: we’re ensuring it has a value already

0        if (addrLocal.has_value() && addrLocal->IsValid())

in src/test/netbase_tests.cpp:30 in e4e03e9935 outdated

28-    LookupHost(ip, addr, false);
29-    return addr;
30+    std::optional<CNetAddr> addr{LookupHost(ip, false)};
31+    if (addr.has_value())
32+        return addr.value();
33+    return CNetAddr();

stickies-v commented at 1:27 pm on October 19, 2022:

0    return LookupHost(ip, false).value_or(CNetAddr{});

in src/test/net_tests.cpp:326 in e4e03e9935 outdated

326+        BOOST_REQUIRE(net_addr.value().IsIPv6());
327+        BOOST_CHECK_EQUAL(net_addr.value().ToString(), expected_canonical_representation_output);
328     }
329 }
330-
331+/*

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 eb7b7e2b42 outdated

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:

0 * [@see](/bitcoin-bitcoin/contributor/see/) LookupHost(const std::string&, unsigned int, bool, DNSLookupFn)

in src/netbase.h:151 in eb7b7e2b42 outdated

152 
153 /**
154  * Resolve a service string to its first corresponding service.
155  *
156- * @see Lookup(const std::string&, std::vector<CService>&, uint16_t, bool, unsigned int, DNSLookupFn)
157+ * @see Lookup(const std::string&, int16_t, bool, unsigned int, DNSLookupFn)

stickies-v commented at 2:56 pm on January 23, 2023:

0 * [@see](/bitcoin-bitcoin/contributor/see/) Lookup(const std::string&, uint16_t, bool, unsigned int, DNSLookupFn)

in src/netbase.cpp:221 in eb7b7e2b42 outdated

253+
254+    const std::vector<CService> services{Lookup(name, portDefault, fAllowLookup, 1, dns_lookup_function)};
255+    if (services.empty())
256+        return std::nullopt;
257+
258+    return services[0];

stickies-v commented at 3:03 pm on January 23, 2023:

0    return services.empty() ? std::nullopt : services.front()

brunoerg commented at 9:29 pm on January 23, 2023:

I think it should be:

0return services.empty() ? std::nullopt : std::optional<CService>(services.front());

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:

make_optional seems more elegant, gonna adopt it.

in src/netbase.cpp:215 in eb7b7e2b42 outdated

241+std::optional<CService> Lookup(const std::string& name, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function)
242 {
243     if (!ContainsNoNUL(name)) {
244-        return false;
245+        return std::nullopt;
246     }

stickies-v commented at 3:08 pm on January 23, 2023:

I think this entire block can be just removed? We’re checking that in the overloaded Lookup function already?

brunoerg commented at 11:21 pm on January 23, 2023:

Yes, perfect!

in src/netbase.cpp:190 in eb7b7e2b42 outdated

204-        return false;
205-    addr = vIP.front();
206-    return true;
207+    const std::vector<CNetAddr> addresses{LookupHost(name, 1, fAllowLookup, dns_lookup_function)};
208+    if (addresses.empty()) return std::nullopt;
209+    return addresses.front();

stickies-v commented at 3:26 pm on January 23, 2023:

0    return addresses.empty() ? std::nullopt : addresses.front();

in src/httpserver.cpp:168 in eb7b7e2b42 outdated

169-    rpc_allow_subnets.push_back(CSubNet(localv4, 8));      // always allow IPv4 local subnet
170-    rpc_allow_subnets.push_back(CSubNet(localv6));         // always allow IPv6 localhost
171+    const std::optional<CNetAddr> localv4{LookupHost("127.0.0.1", false)};
172+    const std::optional<CNetAddr> localv6{LookupHost("::1", false)};
173+    rpc_allow_subnets.push_back(CSubNet(localv4.value(), 8));      // always allow IPv4 local subnet
174+    rpc_allow_subnets.push_back(CSubNet(localv6.value()));         // always allow IPv6 localhost

stickies-v commented at 3:32 pm on January 23, 2023:

perhaps no need for intermediary variables?

0    rpc_allow_subnets.push_back(CSubNet{LookupHost("127.0.0.1", false).value(), 8});      // always allow IPv4 local subnet
1    rpc_allow_subnets.push_back(CSubNet{LookupHost("::1", false).value()});               // always allow IPv6 localhost

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 {};
14
15     {
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     }
27
28+    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

Force-pushed addressing @stickies-v and @theStack review, thanks!

in src/netbase.cpp:141 in 1f3962cb24 outdated

137 {
138-    vIP.clear();
139-
140-    if (!ContainsNoNUL(name)) {
141-        return false;
142-    }

stickies-v commented at 11:33 am on January 24, 2023:

I don’t think you can remove this check. What @theStack suggested is this, which I agree with:

0-    if (!ContainsNoNUL(name)) {
1-        return addresses;
2-    }
3+    if (!ContainsNoNUL(name)) return {};

brunoerg commented at 12:10 pm on January 24, 2023:

Yes, sorry, I accidentally removed it. I’m pushing it back.

in src/netbase.cpp:182 in 1f3962cb24 outdated

198-    if(vIP.empty())
199-        return false;
200-    addr = vIP.front();
201-    return true;
202+    const std::vector<CNetAddr> addresses{LookupHost(name, 1, fAllowLookup, dns_lookup_function)};
203+    return addresses.empty() ? std::nullopt : std::optional<CNetAddr>(addresses.front());

stickies-v commented at 11:34 am on January 24, 2023:

nit: std::make_optional as #26261 (review)

brunoerg force-pushed on Jan 24, 2023

brunoerg commented at 12:18 pm on January 24, 2023: contributor

Force-pushed addressing #26261 (review) and #26261 (review).

Thanks so much again for the review, @stickies-v and @theStack.

brunoerg force-pushed on Jan 24, 2023

stickies-v approved

stickies-v commented at 1:08 pm on January 24, 2023: contributor

ACK 24adc24a3

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.

in src/netbase.cpp:172 in c330298734 outdated

170+        return addresses;
171     }
172     std::string strHost = name;
173     if (strHost.empty())
174-        return false;
175+        return addresses;

theStack commented at 3:05 pm on January 25, 2023:

nit: the addresses vector is not needed anymore in the first LookupHost function and can be removed:

0    if (!ContainsNoNUL(name)) return {};
1    std::string strHost = name;
2    if (strHost.empty()) return {};

brunoerg commented at 9:03 pm on January 25, 2023:

Nice, addressed!

in src/test/fuzz/netbase_dns_lookup.cpp:36 in c330298734 outdated

33-        if (LookupHost(name, resolved_addresses, max_results, allow_lookup, fuzzed_dns_lookup_function)) {
34+        const std::vector<CNetAddr> resolved_addresses{LookupHost(name, max_results, allow_lookup, fuzzed_dns_lookup_function)};
35+        if (!resolved_addresses.empty()) {
36             for (const CNetAddr& resolved_address : resolved_addresses) {
37                 assert(!resolved_address.IsInternal());
38             }

theStack commented at 3:09 pm on January 25, 2023:

nit: Note that the empty-check is not needed anymore. If the vector is empty then iterating it will do nothing anyways:

0        for (const CNetAddr& resolved_address : resolved_addresses) {
1            assert(!resolved_address.IsInternal());
2        }

brunoerg commented at 9:03 pm on January 25, 2023:

Nice, addressed!

theStack commented at 3:13 pm on January 25, 2023: contributor

LGTM, will do a final review round tomorrow. Left two non-blocking nits below, feel free to ignore.

brunoerg force-pushed on Jan 25, 2023

brunoerg commented at 9:07 pm on January 25, 2023: contributor

Just addressed last @theStack’s review! Thanks, @stickies-v and @theStack for the reviews!

stickies-v approved

stickies-v commented at 9:16 am on January 26, 2023: contributor

re-ACK db27abe

in src/netbase.cpp:204 in 1745634263 outdated

198@@ -204,13 +199,12 @@ bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t port
199     std::string hostname;
200     SplitHostPort(name, port, hostname);
201 
202-    std::vector<CNetAddr> vIP;
203-    bool fRet = LookupIntern(hostname, vIP, nMaxSolutions, fAllowLookup, dns_lookup_function);
204-    if (!fRet)
205+    const std::vector<CNetAddr> addresses{LookupIntern(hostname, nMaxSolutions, fAllowLookup, dns_lookup_function)};
206+    if (addresses.empty())
207         return false;

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)

in src/net.cpp:2202 in 3e39ec3e7e outdated

2149@@ -2150,10 +2150,10 @@ void Discover()
2150     char pszHostName[256] = "";
2151     if (gethostname(pszHostName, sizeof(pszHostName)) != SOCKET_ERROR)
2152     {
2153-        std::vector<CNetAddr> vaddr;
2154-        if (LookupHost(pszHostName, vaddr, 0, true))
2155+        const std::vector<CNetAddr> addresses{LookupHost(pszHostName, 0, true)};
2156+        if (!addresses.empty())
2157         {

theStack commented at 4:19 pm on January 26, 2023:

nit: this non-empty check can be removed, as the iteration of an empty vector is a no-op anyways

in src/netbase.h:121 in 3e39ec3e7e outdated

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.

theStack commented at 4:20 pm on January 26, 2023:

nit: this doesn’t seem right, since at this point the function still returns bool?

in src/netbase.cpp:190 in 53ce96156c outdated

184@@ -185,48 +185,43 @@ bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup, DNSL
185     return true;
186 }
187 
188-bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function)
189+std::vector<CService> Lookup(const std::string& name, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function)
190 {
191+    std::vector<CService> services;

theStack commented at 4:22 pm on January 26, 2023:

nit: could introduce this vector later below, right before the for iteration, and simply return {} before

in src/test/fuzz/netbase_dns_lookup.cpp:46 in 53ce96156c outdated

41@@ -42,18 +42,18 @@ FUZZ_TARGET(netbase_dns_lookup)
42         }
43     }
44     {
45-        std::vector<CService> resolved_services;
46-        if (Lookup(name, resolved_services, default_port, allow_lookup, max_results, fuzzed_dns_lookup_function)) {
47+        const std::vector<CService> resolved_services{Lookup(name, default_port, allow_lookup, max_results, fuzzed_dns_lookup_function)};
48+        if (!resolved_services.empty()) {

theStack commented at 4:22 pm on January 26, 2023:

nit: This non-empty check can be removed.

in src/httpserver.cpp:338 in 55d991af40 outdated

314@@ -319,8 +315,8 @@ static bool HTTPBindAddresses(struct evhttp* http)
315         LogPrintf("Binding RPC on address %s port %i\n", i->first, i->second);
316         evhttp_bound_socket *bind_handle = evhttp_bind_socket_with_handle(http, i->first.empty() ? nullptr : i->first.c_str(), i->second);
317         if (bind_handle) {
318-            CNetAddr addr;
319-            if (i->first.empty() || (LookupHost(i->first, addr, false) && addr.IsBindAny())) {
320+            const std::optional<CNetAddr> addr{LookupHost(i->first, false)};
321+            if (i->first.empty() || (addr.has_value() && addr->IsBindAny())) {

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

Code-review ACK db27abee09243ee65fe80838eb5784b37f5bee2f

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-ACK 3bb0e074525aa0bb356960ecf38086e8671a78d4

in src/netbase.h:121 in 3bb0e07452 outdated

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.

I appreciate a final review from you @stickies-v and @theStack, thanks a lot!

stickies-v approved

stickies-v commented at 5:45 pm on February 3, 2023: contributor

re-ACK 84b3ab4

theStack approved

theStack commented at 2:05 pm on February 4, 2023: contributor

re-ACK 84b3ab40d5480862f3f7e08c4ad1f9124990ba74

DrahtBot added the label Needs rebase on Feb 17, 2023

brunoerg force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

brunoerg force-pushed on Feb 19, 2023

brunoerg force-pushed on Feb 20, 2023

brunoerg commented at 1:04 pm on February 20, 2023: contributor

Force-pushed rebasing.

stickies-v approved

stickies-v commented at 5:03 pm on February 24, 2023: contributor

re-ACK 0566f17

I verified that the only changes since https://github.com/bitcoin/bitcoin/commit/84b3ab40d5480862f3f7e08c4ad1f9124990ba74 are:

resolving the merge conflict from #25619
in ProcessUpnp(), replace resolved.value(). with resolved-> which is reasonable since we check for has_value() the line above it:

DrahtBot requested review from theStack on Feb 24, 2023

brunoerg commented at 8:41 pm on March 6, 2023: contributor

@theStack have in mind re-ACK it?

in src/qt/optionsdialog.cpp:410 in e6ae61eaff outdated

405@@ -406,9 +406,8 @@ 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()};
412+    const std::optional<CNetAddr> ui_proxy_netaddr{LookupHost(ui->proxyIp->text().toStdString(), /*fAllowLookup=*/false)};
413+    const CService ui_proxy{ui_proxy_netaddr.value(), ui->proxyPort->text().toUShort()};

theStack commented at 2:32 am on March 7, 2023:

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 LookupHost should always return a value, but I’d prefer to be rather safe than sorry.

brunoerg commented at 6:22 pm on March 7, 2023:

You’re right. Considering it comes from GUI, it’s better to check whether it has value. Addressed it!

DrahtBot requested review from theStack on Mar 7, 2023

brunoerg force-pushed on Mar 7, 2023

brunoerg commented at 6:22 pm on March 7, 2023: contributor

Force-pushed addressing #26261 (review)

in src/netbase.h:112 in 9890fbdf0b outdated

112- *          any resulting network addresses.
113+ * @returns The resulting network addresses to which the specified host
114+ *          string resolved.
115  *
116- * @see Lookup(const std::string&, std::vector<CService>&, uint16_t, bool, unsigned int, DNSLookupFn)
117+ * @see Lookup(const std::string&, uint16_t, bool, unsigned int, DNSLookupFn)

vasild commented at 4:25 pm on March 16, 2023:

This change in the comment about Lookup() was done in commit

05a656f60fb p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`

but it belongs to the next commit

0786ad94614 p2p, refactor: return vector/optional<CService> in `Lookup`

brunoerg commented at 10:04 pm on March 16, 2023:

Even being a comment in LookupHost, you’re right, the change is done in the next commit. Thanks

in src/netbase.cpp:666 in 9890fbdf0b outdated

662@@ -689,27 +663,27 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
663 
664     const size_t slash_pos{subnet_str.find_last_of('/')};
665     const std::string str_addr{subnet_str.substr(0, slash_pos)};
666-    CNetAddr addr;
667+    std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};

vasild commented at 4:33 pm on March 16, 2023:

nit:

0    const std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};

in src/rpc/net.cpp:715 in 9890fbdf0b outdated

711@@ -712,9 +712,10 @@ static RPCHelpMan setban()
712         isSubnet = true;
713 
714     if (!isSubnet) {
715-        CNetAddr resolved;
716-        LookupHost(request.params[0].get_str(), resolved, false);
717-        netAddr = resolved;
718+        const std::optional addr = LookupHost(request.params[0].get_str(), false);

vasild commented at 4:44 pm on March 16, 2023:

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?

brunoerg commented at 10:07 pm on March 16, 2023:

Seems a leftover from my part, didn’t did it intentionally, thanks for noticing it. Fixed.

in src/test/addrman_tests.cpp:38 in 9890fbdf0b outdated

32@@ -33,16 +33,16 @@ static int32_t GetCheckRatio(const NodeContext& node_ctx)
33 
34 static CNetAddr ResolveIP(const std::string& ip)
35 {
36-    CNetAddr addr;
37-    BOOST_CHECK_MESSAGE(LookupHost(ip, addr, false), strprintf("failed to resolve: %s", ip));
38-    return addr;
39+    const std::optional<CNetAddr> addr{LookupHost(ip, false)};
40+    BOOST_CHECK_MESSAGE(addr.has_value(), strprintf("failed to resolve: %s", ip));
41+    return addr.value();

vasild commented at 4:50 pm on March 16, 2023:

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.

brunoerg commented at 10:09 pm on March 16, 2023:

Nice find, I think it’s better to leave BOOST_CHECK_MESSAGE() and using return addr.value_or(CNetAddr{});. Addressing it.

in src/test/addrman_tests.cpp:45 in 9890fbdf0b outdated

46-    CService serv;
47-    BOOST_CHECK_MESSAGE(Lookup(ip, serv, port, false), strprintf("failed to resolve: %s:%i", ip, port));
48-    return serv;
49+    const std::optional<CService> serv{Lookup(ip, port, false)};
50+    BOOST_CHECK_MESSAGE(serv.has_value(), strprintf("failed to resolve: %s:%i", ip, port));
51+    return serv.value();

vasild commented at 5:00 pm on March 16, 2023:

Same here:

0    return serv.value_or(CService{});

brunoerg commented at 10:10 pm on March 16, 2023:

Nice!

in src/test/addrman_tests.cpp:1017 in 9890fbdf0b outdated

936+    const std::optional<CService> serv{Lookup("252.1.1.1", 7777, false)};
937+    BOOST_CHECK(serv.has_value());
938+    CAddress addr = CAddress(serv.value(), NODE_NONE);
939+    std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)};
940+    BOOST_CHECK(resolved.has_value());
941+    AddrInfo info = AddrInfo(addr, resolved.value());

vasild commented at 5:37 pm on March 16, 2023:

0    const std::optional<CService> serv{Lookup("252.1.1.1", 7777, false)};
1    BOOST_CHECK(serv.has_value());
2    CAddress addr{serv.value_or(CService{}), NODE_NONE};
3    std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)};
4    BOOST_CHECK(resolved.has_value());
5    AddrInfo info{addr, resolved.value_or(CNetAddr{})};

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.

brunoerg commented at 10:45 pm on March 16, 2023:

Replaced it with BOOST_REQUIRE()

in src/httpserver.cpp:341 in 9890fbdf0b outdated

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);

vasild commented at 5:52 pm on March 16, 2023:

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.

vasild commented at 9:49 am on March 17, 2023:

Now it is:

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:

0if (i->first.empty() || (add.has_value() && addr->IsBindAny())) {

brunoerg commented at 10:17 am on March 17, 2023:

Fixed it, thanks!

in src/test/netbase_tests.cpp:485 in 9890fbdf0b outdated

485+    addr = LookupHost("127.0.0.1\0"s, false);
486+    BOOST_CHECK(!addr.has_value());
487+    addr = LookupHost("127.0.0.1\0example.com"s, false);
488+    BOOST_CHECK(!addr.has_value());
489+    addr = LookupHost("127.0.0.1\0example.com\0"s, false);
490+    BOOST_CHECK(!addr.has_value());

vasild commented at 6:00 pm on March 16, 2023:

This can be shorter:

0    BOOST_CHECK(LookupHost("127.0.0.1"s, false).has_value());
1    BOOST_CHECK(!LookupHost("127.0.0.1\0"s, false).has_value());
2    BOOST_CHECK(!LookupHost("127.0.0.1\0example.com"s, false).has_value());
3    BOOST_CHECK(!LookupHost("127.0.0.1\0example.com\0"s, false).has_value());

brunoerg commented at 10:14 pm on March 16, 2023:

Much more cleaner, nice!

in src/zmq/zmqpublishnotifier.cpp:101 in 9890fbdf0b outdated

 98@@ -99,9 +99,8 @@ static bool IsZMQAddressIPV6(const std::string &zmq_address)
 99     const size_t colon_index = zmq_address.rfind(":");
100     if (tcp_index == 0 && colon_index != std::string::npos) {
101         const std::string ip = zmq_address.substr(tcp_prefix.length(), colon_index - tcp_prefix.length());
102-        CNetAddr addr;
103-        LookupHost(ip, addr, false);
104-        if (addr.IsIPv6()) return true;
105+        const std::optional<CNetAddr> addr{LookupHost(ip, false)};
106+        if (addr.has_value() && addr.value().IsIPv6()) return true;

vasild commented at 6:03 pm on March 16, 2023:

There was a bug in the old code - if lookup failed it would have returned true (i.e. pretend that it is IPv6). The new code fixes this, nice!

in src/netbase.cpp:221 in 9890fbdf0b outdated

234-bool Lookup(const std::string& name, CService& addr, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function)
235+std::optional<CService> Lookup(const std::string& name, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function)
236 {
237-    if (!ContainsNoNUL(name)) {
238-        return false;
239-    }

vasild commented at 6:09 pm on March 16, 2023:

This is removed in its own commit

0p2p: remove duplicated `ContainsNoNUL` calls in `Lookup`

but I guess that removal can be squashed into one of the preceding commits:

0p2p, refactor: return vector/optional<CService> in `Lookup`

like the same check is removed from LookupHost() in

0p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`

vasild commented at 6:09 pm on March 16, 2023: contributor

Approach ACK 9890fbdf0b702ccb77a3b54d116cd5d7e95ecffa

brunoerg force-pushed on Mar 16, 2023

brunoerg commented at 10:54 pm on March 16, 2023: contributor

Force-pushed addressing @vasild’s review. Thanks @vasild for your in-depth review and considerations!

brunoerg force-pushed on Mar 17, 2023

vasild commented at 10:24 am on March 17, 2023: contributor

ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826

DrahtBot requested review from stickies-v on Mar 17, 2023

vasild approved

in src/netbase.h:151 in ec28c89ef6 outdated

152 
153 /**
154  * Resolve a service string to its first corresponding service.
155  *
156- * @see Lookup(const std::string&, std::vector<CService>&, uint16_t, bool, unsigned int, DNSLookupFn)
157+ * @see Lookup(const std::string&, uint16_t, bool, unsigned int, DNSLookupFn)

stickies-v commented at 2:32 pm on March 17, 2023:

There’s another such update needed for the LookupNumeric docstring which currently reads:

0* [@see](/bitcoin-bitcoin/contributor/see/) Lookup(const std::string&, std::vector<CService>&, uint16_t, bool, unsigned int, DNSLookupFn)
1* for additional parameter descriptions.

theStack approved

theStack commented at 0:08 am on March 21, 2023: contributor

re-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826

in src/httpserver.cpp:337 in ec28c89ef6 outdated

314@@ -319,8 +315,8 @@ static bool HTTPBindAddresses(struct evhttp* http)
315         LogPrintf("Binding RPC on address %s port %i\n", i->first, i->second);
316         evhttp_bound_socket *bind_handle = evhttp_bind_socket_with_handle(http, i->first.empty() ? nullptr : i->first.c_str(), i->second);
317         if (bind_handle) {
318-            CNetAddr addr;
319-            if (i->first.empty() || (LookupHost(i->first, addr, false) && addr.IsBindAny())) {
320+            const std::optional<CNetAddr> addr{LookupHost(i->first, false)};

stickies-v commented at 11:43 am on March 30, 2023:

Largely unrelated, so just leaving this here for follow-up. I think this can be cleaned up quite a bit:

 0diff --git a/src/httpserver.cpp b/src/httpserver.cpp
 1index 49c2a85e1..46d095633 100644
 2--- a/src/httpserver.cpp
 3+++ b/src/httpserver.cpp
 4@@ -311,17 +311,17 @@ static bool HTTPBindAddresses(struct evhttp* http)
 5     }
 6 
 7     // Bind addresses
 8-    for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
 9-        LogPrintf("Binding RPC on address %s port %i\n", i->first, i->second);
10-        evhttp_bound_socket *bind_handle = evhttp_bind_socket_with_handle(http, i->first.empty() ? nullptr : i->first.c_str(), i->second);
11+    for (const auto& [raw_addr, port] : endpoints) {
12+        LogPrintf("Binding RPC on address %s port %i\n", raw_addr, port);
13+        evhttp_bound_socket *bind_handle = evhttp_bind_socket_with_handle(http, raw_addr.empty() ? nullptr : raw_addr.c_str(), port);
14         if (bind_handle) {
15-            const std::optional<CNetAddr> addr{LookupHost(i->first, false)};
16-            if (i->first.empty() || (addr.has_value() && addr->IsBindAny())) {
17+            const std::optional<CNetAddr> addr{LookupHost(raw_addr, false)};
18+            if (raw_addr.empty() || (addr.has_value() && addr->IsBindAny())) {
19                 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
20             }
21             boundSockets.push_back(bind_handle);
22         } else {
23-            LogPrintf("Binding RPC on address %s port %i failed.\n", i->first, i->second);
24+            LogPrintf("Binding RPC on address %s port %i failed.\n", raw_addr, port);
25         }
26     }
27     return !boundSockets.empty();

in src/qt/optionsdialog.cpp:410 in ec28c89ef6 outdated

405@@ -406,9 +406,8 @@ 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()};
412+    const std::optional<CNetAddr> ui_proxy_netaddr{LookupHost(ui->proxyIp->text().toStdString(), /*fAllowLookup=*/false)};
413+    const CService ui_proxy{ui_proxy_netaddr.has_value() ? ui_proxy_netaddr.value() : CNetAddr{}, ui->proxyPort->text().toUShort()};

stickies-v commented at 11:52 am on March 30, 2023:

0    const CService ui_proxy{ui_proxy_netaddr.value_or(CNetAddr{}), ui->proxyPort->text().toUShort()};

in src/netbase.cpp:196 in ec28c89ef6 outdated

225+    const std::vector<CNetAddr> addresses{LookupIntern(hostname, nMaxSolutions, fAllowLookup, dns_lookup_function)};
226+    if (addresses.empty()) return {};
227+    std::vector<CService> services;
228+    services.resize(addresses.size());
229+    for (unsigned int i = 0; i < addresses.size(); i++)
230+        services[i] = CService(addresses[i], port);

stickies-v commented at 12:04 pm on March 30, 2023:

A bit more readable and avoids default initializing CServices:

0    services.reserve(addresses.size());
1    for (const auto& addr : addresses)
2        services.emplace_back(addr, port);

in src/netbase.h:121 in ec28c89ef6 outdated

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-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826

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

Addressed: #26261 (review) #26261 (review) #26261 (review)

in src/bench/addrman.cpp:108 in 4eee95e57b outdated

103@@ -118,8 +104,8 @@ static void AddrManSelectFromAlmostEmpty(benchmark::Bench& bench)
104     AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
105 
106     // Add one address to the new table
107-    CService addr = ResolveService("250.3.1.1", 8333);
108-    addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333));
109+    CService addr = Lookup("250.3.1.1", 8333, false).value();
110+    addrman.Add({CAddress(addr, NODE_NONE)}, Lookup("250.3.1.1", 8333, false).value());

stickies-v commented at 9:17 am on April 26, 2023:

Any reason why not just reuse the already looked up service?

0    const CService service{Lookup("250.3.1.1", 8333, false).value()};
1    addrman.Add({CAddress(service, NODE_NONE)}, service);

brunoerg commented at 4:40 pm on May 26, 2023:

No, going to change it

stickies-v approved

stickies-v commented at 9:27 am on April 26, 2023: contributor

re-ACK 4eee95e57

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

ACK 4eee95e57bb6f773bcaeb405bca949f158a62134

in src/rpc/net.cpp:715 in 4eee95e57b outdated

711@@ -712,9 +712,10 @@ static RPCHelpMan setban()
712         isSubnet = true;
713 
714     if (!isSubnet) {
715-        CNetAddr resolved;
716-        LookupHost(request.params[0].get_str(), resolved, false);
717-        netAddr = resolved;
718+        const std::optional<CNetAddr> addr = LookupHost(request.params[0].get_str(), false);

theStack commented at 9:51 pm on May 22, 2023:

nit, if you need to retouch:

0        const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};

in src/test/net_tests.cpp:138 in 4eee95e57b outdated

134@@ -135,7 +135,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
135     CNetAddr addr;
136 
137     // IPv4, INADDR_ANY
138-    BOOST_REQUIRE(LookupHost("0.0.0.0", addr, false));
139+    addr = LookupHost("0.0.0.0", false).value();

theStack commented at 10:20 pm on May 22, 2023:

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

stickies-v commented at 11:25 am on May 23, 2023:

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:

0    addr = Assert(LookupHost("x0.0.0.0", false)).value();

0test/net_tests.cpp:139 test_method: Assertion `LookupHost("x0.0.0.0", false)' failed.
1unknown location:0: fatal error: in "net_tests/cnetaddr_basic": signal: SIGABRT (application abort requested)
2test/net_tests.cpp:134: last checkpoint: "cnetaddr_basic" test entry

brunoerg commented at 4:59 pm on May 26, 2023:

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?

brunoerg commented at 5:02 pm on May 26, 2023:

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-ACK 4eee95e57bb6f773bcaeb405bca949f158a62134

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

Force-pushed addressing:

Maybe done with nits, and we can have it merged?

stickies-v approved

stickies-v commented at 5:12 pm on May 26, 2023: contributor

re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b - just addressing two nits, no other changes

 01:  c0cf3d70e = 1:  5c1774a56 p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern`
 12:  94b5edbf6 = 2:  7799eb125 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`
 23:  617bc481e ! 3:  34bcdfc6a p2p, refactor: return vector/optional<CService> in `Lookup`
 3    @@ src/bench/addrman.cpp: static void AddrManSelectFromAlmostEmpty(benchmark::Bench
 4     -    CService addr = ResolveService("250.3.1.1", 8333);
 5     -    addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333));
 6     +    CService addr = Lookup("250.3.1.1", 8333, false).value();
 7    -+    addrman.Add({CAddress(addr, NODE_NONE)}, Lookup("250.3.1.1", 8333, false).value());
 8    ++    addrman.Add({CAddress(addr, NODE_NONE)}, addr);
 9
10          bench.run([&] {
11              (void)addrman.Select();
124:  4eee95e57 ! 4:  5c832c382 p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`
13    @@ src/rpc/net.cpp: static RPCHelpMan setban()
14     -        CNetAddr resolved;
15     -        LookupHost(request.params[0].get_str(), resolved, false);
16     -        netAddr = resolved;
17    -+        const std::optional<CNetAddr> addr = LookupHost(request.params[0].get_str(), false);
18    ++        const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
19     +        if (addr.has_value()) {
20     +            netAddr = addr.value();
21     +        }

DrahtBot requested review from theStack on May 26, 2023

DrahtBot requested review from vasild on May 26, 2023

theStack approved

theStack commented at 2:13 pm on May 30, 2023: contributor

re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b

Maybe done with nits, and we can have it merged?

Agree!

achow101 commented at 3:28 pm on May 30, 2023: member

ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b

achow101 merged this on May 30, 2023

achow101 closed this on May 30, 2023

sidhujag referenced this in commit e0f2a7bc41 on May 30, 2023

bitcoin locked this on May 29, 2024

p2p: cleanup LookupIntern, Lookup and LookupHost #26261

Reviews

Conflicts

p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost` #26261