init: adding check for : for -torcontrol flag #28018

right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :

closes #23589

<!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

<!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

<!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<img width="500" alt="Screenshot 2023-06-30 at 4 40 24 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d95ba8f9-dad2-4ada-b235-41acf8b58aa4">

<img width="500" alt="Screenshot 2023-06-30 at 4 40 12 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d2280a41-b1c2-44e6-b32a-9112321b1664">

<img width="500" alt="Screenshot 2023-06-30 at 4 39 51 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/ec94182a-b9dd-4701-b57d-6a45ad3487b3">

Overall, I think the code is good. However, there have been some earlier remarks in this comment on where to place this code.

Someone with a better understanding of the entire codebase should comment on the placement of this.

In any case, I'm testing it locally and will report back on whether it meets the requirements soon (one suggestion I think you should avoid using / in the branch name)

Overall, I think the code is good. However, there have been some earlier remarks in this comment on where to place this code.

Someone with a better understanding of the entire codebase should comment on the placement of this.

In any case, I'm testing it locally and will report back on whether it meets the requirements soon (one suggestion I think you should avoid using / in the branch name)

Yea I could totally move the logic to a util or somewhere other than init.cpp since there might be too much logic in there. Not exactly sure where would be the best spot.

Also another thing I wanted to clarify is that will -torcontrol always be in the format of host:port or is there ever a chance of being abe to just provide one or the other? because then we can try splitting and validating each part individually

Tested this locally.

1. Any string that is not of the type <host>:<port> is rejected. Example: ./src/bitcoind -torcontrol=1.

2. Also works for Domain names. Example: ./src/bitcoind -torcontrol=localhost:9051 is accepted.

3. But multiple instances of : are also accepted. Example: ./src/bitcoind -torcontrol=localhost:9051:23 is also accepted. This should also be avoided, right?

Also another thing I wanted to clarify is that will -torcontrol always be in the format of host:port or is there ever a chance of being abe to just provide one or the other? because then we can try splitting and validating each part individually

Yes, I think -torcontrol should always be of form <host>:<port>.

Concept NACK. Port isn't required (the default is 9051). Furthermore, even "1" is a valid value (it would be the same as 0.0.0.1:9051), though I suppose it wouldn't hurt to check for it specifically and reject it.

A gentle ping to @luke-jr, what about this issue then? 23589

Unrelated to the actual implementation in this PR, but I disagree with some of Luke's points:

Port isn't required (the default is 9051).

The way this is currently documented, both host and port should be required. However, it does align with many of our other options to have a default port, and if that makes sense here too then the help for torcontrol arg should be updated to show port as optional as a first step.

Furthermore, even "1" is a valid value (it would be the same as 0.0.0.1:9051), though I suppose it wouldn't hurt to check for it specifically and reject it.

Whilst this is technically true for low-level networking (code), IMO it's extremely un-common to not require users to use dot-decimal notation for hosts, and I think we should reject non dot-decimal hosts to prevent exactly the issue in #23589. We'll lose no flexibility by doing so as users who want to use 0.0.0.1 can just, set the option to that directly?

The way this is currently documented, both host and port should be required. However, it does align with many of our other options to have a default port, and if that makes sense here too then the help for torcontrol arg should be updated to show port as optional as a first step.

I think it makes sense to make the port optional if that is also what is being done for the other options.

Whilst this is technically true for low-level networking (code), IMO it's extremely un-common to not require users to use dot-decimal notation for hosts, and I think we should reject non dot-decimal hosts to prevent exactly the issue in #23589. We'll lose no flexibility by doing so as users who want to use 0.0.0.1 can just, set the option to that directly?

one of my concerns is that if there are users not using non dot-decimal hosts then it would cause breakage for those users if we begin to reject non dot-decimal hosts

do we reject non dot-decimal hosts anywhere else in this codebase?

closing this PR for now since this #28101 should fix the issue, the PR just changes the helper text to explain that a default port is included but no additional checks are added