Torcontrol opt check

[amadeuszpawlik]

Based on SplitHostPort from #22087

• Adds new utility for easy sanity check of network addresses
• Checks -torcontrol for a valid host:port string
• Adds a return param in StartTorControl so that we can stop init and present a message if torcontrol input is invalid

Solves #23589

[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	vasild
Concept ACK	laanwj
Approach ACK	w0xlt

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:

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

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.

[laanwj]

Concept ACK, thanks for working on this!

[w0xlt]

Approach ACK

[fanquake]

@amadeuszpawlik want to follow up here now that #22087 has been merged?

[achow101]

cc @vasild @ryanofsky @mzumsande

[vasild]

Concept ACK

[amadeuszpawlik]

Thanks a lot @vasild for your review; applied your proposed changes

[fanquake]

@amadeuszpawlik can you rebase again, to fix the failing CI (see #26306).

[vasild]

ACK 5be4336

The code looks ok, but I think it can be better: what about doing the checks in SplitHostPort() and not adding a new function HasValidHostPort()? The former already does some validity checks and it makes sense to do all checks in one place.

Then the code would look something like this:

/**
 * Splits a host:port string into host and port.
 * Validates the host and the port.
 * 
 * @param[in] in    The string to split.
 * @param[out] host Host-portion of the input, if found and valid.
 * @param[out] port Port-portion of the input, if found and valid.
 */
void SplitHostPort(std::string_view in,
                   std::optional<std::string>& host,
                   std::optional<uint16_t>& port);

...

for (const auto& [option, port_required] :
     std::vector<std::tuple<const char*, bool>>{{"-i2psam", true},
                                                {"-onion", true},
                                                {"-proxy", true},
                                                {"-rpcbind", false},
                                                {"-torcontrol", true},
                                                {"-whitebind", false},
                                                {"-zmqpubhashblock", true},
                                                {"-zmqpubhashtx", true},
                                                {"-zmqpubrawblock", true},
                                                {"-zmqpubrawtx", true},
                                                {"-zmqpubsequence", true}}) {
    for (const std::string& str : args.GetArgs(option)) {
        std::optional<std::string> host;
        std::optional<uint16_t> port;
        SplitHostPort(str, host, port);
        if (!host.has_value()) {
            return InitError(InvalidHostErrMsg(option, str));
        }
        if (port_required && !port.has_value()) {
            return InitError(InvalidPortErrMsg(option, str));
        }
    }
}

That would benefit/check also the other options, not just -torcontrol and there would not be a need to modify StartTorControl().

[vasild]

style: case should be below switch

It is good to use clang-format or contrib/devtools/clang-format-diff.py to avoid such suggestions.

[vasild]

style: as per doc/developer-notes.md variables should use snake_case.

[vasild]

nit: for consistency with above

Suggested change

	error = Untranslated("No hostname:port specified in torcontrol");
	error = Untranslated("No hostname:port specified in -torcontrol");

[fanquake]

@amadeuszpawlik are you planning on addressing any of the feedback here?

[vstoyanov]

@amadeuszpawlik @fanquake May I take over and wrap this one up?

[Empact]

@vstoyanov No need to ask for permission to contribute. I'd say go for it.