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.

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

[brunoerg]

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

Ready for review!

[brunoerg]

Looks like the first commit still uses the old naming scheme and probably should be adapted (s/vIP/addresses/)?

Done, force-pushed addressing it!

[brunoerg]

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

• https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1265909107
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084117650
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084158719
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084168091
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084174333
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084197338
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1084205334

[brunoerg]

Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1085160548 and https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1085162148.

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

[brunoerg]

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

[brunoerg]

Since I had to rebase it, I took the oportunity to address last @theStack's review. Thanks!

[brunoerg]

Force-pushed addressing last @stickies-v's review.

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

[brunoerg]

Force-pushed rebasing.

[brunoerg]

@theStack have in mind re-ACK it?

[brunoerg]

Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1127278080

[brunoerg]

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

[vasild]

ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826

[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: https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153145784 https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153157545 https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153170805

[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:

• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201199915
• https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1177587552

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

[achow101]

ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b