p2p, refactor: tidy up LookupSubNet() #23219

 712 | -                // Never allow lookup for netmask
 713 | -                if (LookupHost(strNetmask, vIP, 1, false, dns_lookup_function)) {
 714 | -                    ret = CSubNet(network, vIP[0]);
 715 | +            } else {
 716 | +                // Invalid number; try full netmask syntax. Never allow lookup for netmask.
 717 | +                CNetAddr full_netmask{addr};

vasild commented at 1:28 PM on October 7, 2021:

This variable is used as an output parameter. Need not be set to addr.

                CNetAddr full_netmask;

jonatack commented at 2:09 PM on October 7, 2021:

indeed! done

 692 | -            std::string strNetmask = strSubnet.substr(slash + 1);
 693 | -            uint8_t n;
 694 | -            if (ParseUInt8(strNetmask, &n)) {
 695 | -                // If valid number, assume CIDR variable-length subnet masking
 696 | -                ret = CSubNet(network, n);
 697 | +    const size_t slash{strSubnet.find_last_of('/')};

vasild commented at 1:31 PM on October 7, 2021:

Maybe also rename the parameter strSubnet?

jonatack commented at 2:08 PM on October 7, 2021:

done, plus same param naming between function declaration and definition (subnet vs ret -> just subnet), and updated the doxygen

 713 | -                if (LookupHost(strNetmask, vIP, 1, false, dns_lookup_function)) {
 714 | -                    ret = CSubNet(network, vIP[0]);
 715 | +            } else {
 716 | +                // Invalid number; try full netmask syntax. Never allow lookup for netmask.
 717 | +                CNetAddr full_netmask{addr};
 718 | +                if (LookupHost(/*name=*/str_netmask, /*addr=*/full_netmask, /*fAllowLookup=*/false, dns_lookup_function)) {

vasild commented at 1:41 PM on October 7, 2021:

IMO /*name=*/ and /*addr=*/ is excessive here. I can understand using these comments in places where constants are passed, e.g.

func(5, true);

may be better as

func(/*timeout=*/5, /*allow_lookup=*/true);

But when using variables that have their own names, /*foo=*/ should not be necessary:

int timeout = ...
bool allow_lookup = ...
// good enough
func(timout, allow_lookup);

jonatack commented at 2:07 PM on October 7, 2021:

done

 180 | + * @param[out] subnet_out  Internal subnet representation, if parsable/resolvable
 181 | + *                         from `subnet_str`.
 182 | + * @returns whether the operation succeeded or not.
 183 |   */
 184 | -bool LookupSubNet(const std::string& strSubnet, CSubNet& subnet, DNSLookupFn dns_lookup_function = g_dns_lookup);
 185 | +bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out, DNSLookupFn dns_lookup_function = g_dns_lookup);

vasild commented at 11:21 AM on December 7, 2021:

The third argument looks unnecessary: DNSLookupFn dns_lookup_function = g_dns_lookup. All the callers use just 2 arguments but more importantly, this function calls LookupHost(..., /*fAllowLookup=*/false, dns_lookup_function) so it does not allow lookups. I think it might as well call just LookupHost(..., /*fAllowLookup=*/false); and ditch its 3rd argument.

jonatack commented at 11:58 AM on December 7, 2021:

Good point. Done.

  63 | @@ -64,7 +64,7 @@ FUZZ_TARGET(netbase_dns_lookup)
  64 |      }
  65 |      {
  66 |          CSubNet resolved_subnet;
  67 | -        if (LookupSubNet(name, resolved_subnet, fuzzed_dns_lookup_function)) {
  68 | +        if (LookupSubNet(name, resolved_subnet)) {

laanwj commented at 12:55 PM on December 8, 2021:

What is the reasoning behind this parameter no longer being needed?

vasild commented at 1:40 PM on December 8, 2021:

It is here: #23219 (review), LookupSubNet() never does lookups.

 675 | @@ -676,40 +676,36 @@ bool ConnectThroughProxy(const proxyType& proxy, const std::string& strDest, uin
 676 |      return true;
 677 |  }
 678 |  
 679 | -bool LookupSubNet(const std::string& strSubnet, CSubNet& ret, DNSLookupFn dns_lookup_function)
 680 | +bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)

laanwj commented at 12:56 PM on December 8, 2021:

Thinking of it, a better API would be to return option<CSubNet> instead of a bool and output parameter. But not necessarily in this PR. It's more consistent with the other functions in netbase.h as it is now.

jonatack commented at 4:05 PM on December 8, 2021:

Good idea. I have a branch of netbase follow-ups, could do it there.