p2p: cleanup LookupIntern, Lookup and LookupHost

[brunoerg]

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.

[stickies-v]

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]

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

[stickies-v]

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]

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.

[DrahtBot]

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.

[stickies-v]

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]

Thanks @stickies-v for your valuable review, just addressed your suggestions!

Ready for review!

[brunoerg]

Thanks, @stickies-v for your review. Will address your latest suggestions in a follow-up PR.

[brunoerg]

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 (comment)
#26261 (comment)
#26261 (comment)

[stickies-v]

re-ACK 4eee95e

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.

[vasild]

ACK 4eee95e

[theStack]

re-ACK 4eee95e

Left two comments below, one nit and one "thinking out loud what could happen in the worst case", both no stoppers.

[stickies-v]

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

[brunoerg]

@stickies-v I'm addressing them at this moment, will push soon.

[brunoerg]

Force-pushed addressing:

• p2p: cleanup LookupIntern, Lookup and LookupHost #26261 (comment)
• p2p: cleanup LookupIntern, Lookup and LookupHost #26261 (comment)

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

[stickies-v]

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

git range-diff 8b59231641845f71df37e163bf5b8157fb197d05 4eee95e57bb6f773bcaeb405bca949f158a62134 5c832c3820253affc65c0ed490e26e5b0a4d5c9b

1:  c0cf3d70e = 1:  5c1774a56 p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern`
2:  94b5edbf6 = 2:  7799eb125 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`
3:  617bc481e ! 3:  34bcdfc6a p2p, refactor: return vector/optional<CService> in `Lookup`
    @@ src/bench/addrman.cpp: static void AddrManSelectFromAlmostEmpty(benchmark::Bench
     -    CService addr = ResolveService("250.3.1.1", 8333);
     -    addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333));
     +    CService addr = Lookup("250.3.1.1", 8333, false).value();
    -+    addrman.Add({CAddress(addr, NODE_NONE)}, Lookup("250.3.1.1", 8333, false).value());
    ++    addrman.Add({CAddress(addr, NODE_NONE)}, addr);

          bench.run([&] {
              (void)addrman.Select();
4:  4eee95e57 ! 4:  5c832c382 p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`
    @@ src/rpc/net.cpp: static RPCHelpMan setban()
     -        CNetAddr resolved;
     -        LookupHost(request.params[0].get_str(), resolved, false);
     -        netAddr = resolved;
    -+        const std::optional<CNetAddr> addr = LookupHost(request.params[0].get_str(), false);
    ++        const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
     +        if (addr.has_value()) {
     +            netAddr = addr.value();
     +        }

[theStack]

re-ACK 5c832c3

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

Agree!

[achow101]

ACK 5c832c3