util: Add TorControlArgumentCheck function #23605

This PR fixes #23589

This PR adds a new utility function, TorControlArgumentCheck, which can be used to check if a given string is a valid <host>:<port> pair or not. This PR also uses this new function to check the validity of the -torcontrol argument and raises an InitError in case the value of this argument is invalid.

Master	PR

Wouldn't it be better to return the error from StartTorControl? This way, other error conditions that are currently silently ignored are reported to the user.

In any case, please properly format new code you add.

You may install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

Updated from 46cdabc to 503359e Addressed @MarcoFalke comment

Changes:

1. Corrected Clang Formatting

Wouldn't it be better to return the error from StartTorControl? This way, other error conditions that are currently silently ignored are reported to the user.

Let me take a look and try to figure it out.

Wouldn't it be better to return the error from StartTorControl? This way, other error conditions that are currently silently ignored are reported to the user.

A drawback of this is that TorControl is called fairly late in the initialization process. To provide quick feedback to the user, I like the current early validation in that regard. (the implementation logic could still be moved to a function in torcontrol.cpp ofc).

Sure, I am not against early sanity checks. Though, some errors can only be checked when the thread is started, so I don't think there is any way to avoid the delay (in general). Currently all errors are ignored, so anything that turns them into actual init errors is an improvement, regardless of when that happens.

the implementation logic could still be moved to a function in torcontrol.cpp ofc

So would you suggest I should move isValidHostPort under the torcontrol.cpp file?

Though, some errors can only be checked when the thread is started, so I don't think there is any way to avoid the delay (in general)

Right. I'm fine with moving the check there, to be clear.

So would you suggest I should move isValidHostPort under the torcontrol.cpp file?

No, not really. If it's a generally useful utility function it makes sense to put it under util. Though utilstrencodings.cpp is somewhat debatable, it's network utility functionality not general number/string parsing.

It was more of a question of where to put the argument checking logic itself, with regard to responsibilities. Init shouldn't "know" the implementation details of everything. It's already a big mess in that regard. So you could add a function, say, TorControlArgumentChecks that calls IsValidHostPort, logs a message and returns the status, and call that from Init.

But I agree with @MarcoFalke 's solution.

Though utilstrencodings.cpp is somewhat debatable, it's network utility functionality not general number/string parsing.

That's true. I was not sure where to put the function since none of the util/ files felt right for this function. So what do you think would be the right place to move it to:

1. Move it under util/url.cpp
2. Create a new util/network.cpp file and move this fn under it.

(p.s. I know I should try figuring out myself instead of asking. But I want to get this PR right, and a little expert's advice always helps)

So you could add a function, say, TorControlArgumentChecks, that calls IsValidHostPort

I got your point here. Let me fix this while we think about the right place for the IsValidHostPort function.

Concept ACK

Tried experimenting with PR branch and use -torcontrol. Can't use domain anymore which was added in #22288

bitcoind -torcontrol=localhost:9051

Error: -torcontrol has to be in the form <host>:<port>

Also not sure about use of regex or if this can be implemented without using it. However I will leave that for others to comment who understand C++ better. Its used in src/bench/bench.cpp so maybe its okay if done correctly.

@prayank23 Thanks for testing. Handling domains is a good point. We really need a test for this.

Also not sure about use of regex or if this can be implemented without using it.

Doing it without introducing use of regex should definitely be possible. We already necessarily parse the host:port spec after all, the thing missing is error feedback.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25136 (Torcontrol opt check by amadeuszpawlik)
• #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

I agree with previous comments in that init.cpp shouldn't need to know the implementation details for specific options, like in this case torcontrol. That's up to StartTorControl to check, perhaps in TorControlConnection::Connect(...). Following that, perhaps having HasValidHost(...) and HasValidPort(...) makes more sense in that sometimes you only pass a port, sometimes a full address socket? Another way would be to modify Lookup() in netbase.cpp, so that if no default port is given AND no port is parsed in SplitHostPort, we fail with a suitable message. I find it a bit confusing that TorControl would absolutely require host:port yet supply a default port to fall back on.

Sanity checking for early rejection is imo more suitable for init, see #22087.

If I recall correctly, valid ports are <= 65535 and tor blocks port 0 completely.

Adding some test to the new utility function would be useful.

Updated from 503359e510f4491101580bb6d3a92bb698426bbb to 7a0c95a9f899eb42dc268a0a4e93e0464bf89401 (pr23605.02 -> pr23605.03)

Changes:

1. Scraped the regex based IsValidHostPort implementation.
2. Introduced a new function TorControlArgumentCheck function in torcontrol.cpp. Which will check if the given arguments are valid or not by using the LookUp function.
3. Updated PR and commit description accordingly.

Adding some test to the new utility function would be useful.

That’s a good idea. I am working on those and will include them in PR as soon as they are done. (p.s.: It would be fantastic if someone could give some hints on how to write a test. Thanks! :beers: )

@shaavan check out src/test/ and for sure the documentation for inspiration, an idea could be:

bool static TestTorControlArgumentCheck(const std::string& test, bool expected)                                                                                                                                                     
{                                                                                                                                                                                                                                   
  return TorControlArgumentCheck(test) == expected;                                                                                                                                                                                 
}                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                    
BOOST_AUTO_TEST_CASE(util_TorControlArgumentCheck)                                                                                                                                                                                  
{                                                                                                                                                                                                                                   
  BOOST_CHECK(TestTorControlArgumentCheck("192.168.0.12:123", true));                                                                                                                                                               
  BOOST_CHECK(TestTorControlArgumentCheck("123", false));                                                                                                                                                                           
  // add more 
}   

This test ofc fails because of Lookup, please refer to my comment above (Lookup takes default port so it doesn't do what you want it to do).

I am not sure having a default port (9051) is a problem.

And even if it is, I think it is beyond the scope of this PR, as the goal of this PR was to check if the -torcontrol argument is valid or not. The function TorControlArgumentCheck will maintain its behavior if the Lookup is modified in the future.

However, I still had one concern, though. Is 123 valid hostname? Or say 123:123 a valid host:port argument?

I am not sure having a default port (9051) is a problem.

I don't think it is a problem, but if you're going to have one, it should be defined as a constant (for re-use) and used consistently between the argument pre-check and the eventual use in TorController::TorController.

But it's also fine to require users to provide the entire hostname:port always. It's only a marginal increase in user-friendliness to have a default port here, not worth complicating the code for.

However, I still had one concern, though. Is 123 valid hostname? Or say 123:123 a valid host:port argument?

That's a hairy issue. I would say it is out of scope for bitcoind to have an opinion on what are valid hostnames. I'd say it's a valid hostname if the OS DNS resolver can resolve it.

Concept ack https://github.com/bitcoin/bitcoin/pull/23605/commits/7a0c95a9f899eb42dc268a0a4e93e0464bf89401

Good to see some test units implemented.

I am not sure having a default port (9051) is a problem.

It's not 9051 specifically that is a problem. Lookup itself has a default port, so it doesn't care if you've forgotten to add the port, is what I mean. Maybe I misunderstand, but you write:

check if a given string is a valid host:port pair or not

and comment

//if torcontrol given, it needs to be in form of <host>:<port>

As well as the original issue described by @laanwj in #23589:

I accidentally -torcontrol=1 today [...] it was accepted [...] (even though) the argument needs to be host:port

I tested with -torcontrol=1 and it doesn't raise an error.

Concept ACK

@laanwj

That's a hairy issue. I would say it is out of scope for bitcoind to have an opinion on what are valid hostnames. I'd say it's a valid hostname if the OS DNS resolver can resolve it.

That makes sense. However, since OS DNS resolver can resolve hostname = 1, I think the argument value -torcontrol=1 should be theoretically correct, right? @amadeuszpawlik

I can now understand that in our case, falling on a default port if no ports are provided seems like a suboptimal behavior.

I find it a bit confusing that TorControl would absolutely require host:port yet supply a default port to fall back on.

Let me add a preliminary check for if a valid port is provided in the argument before passing it through the LookUp in the TorControlArgumentCheck function.

Updated from 7a0c95a to 4925963 (pr23605.03 -> pr23605.04) Addressed @vincenzopalazzo and @amadeuszpawlik comments

Changes:

1. Modified LookUp and TorControlArgument function appropriately so that passing the -torcontrol argument without a valid port now causes an error.
2. Fixed one syntax error in the added comment.

I shall add the unit test in a subsequent commit.

I shall add the unit test in a subsequent commit. @shaavan are you still working on this? Looks like the current branch has issues in any case.

open

@shaavan are you still working on this? Looks like the current branch has issues in any case.

Closing for now. Let us know if you'd like this reopened.

Note that this same issue might be better solved by #25136 in any case.