net: Exit with error message if -proxy is specified without arguments (instead of continuing without proxy server) #20003

Exit with error message if -proxy is specified without arguments (instead of continuing without proxy server).

Continuing without a proxy server when the end-user has specified -proxy may result in accidental loss of privacy. (The end-user might think he/she is using a proxy when he/she is not.)

Before this patch:

$ src/bitcoind -proxy
…
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -listen=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -upnp=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
…
2020-09-23T00:24:33Z init message: Starting network threads...

bitcoind is now running without a proxy server (GetProxy(…, …) == false, HaveNameProxy() == false, etc.).

Note that the "-proxy set" log messages above which the end-user might interpret as "good, my traffic is now routed via the proxy".

After this patch:

$ src/bitcoind -proxy
Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.
$ echo $?
1

Thank you. Tested ACK 3bca86ae6b7cc5b2f5bb9ee4567052b34f613ba1.

$ src/bitcoind -proxy
Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.

re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7

We might want a functional test for this? This kind of parameter handling is easy to accidentally break when refactoring init, especially with fragments concerning the same parameters spread over the place.

@laanwj Added :)

Concept ACK. Testing...

Tested f3ee9bd29d22e32356edb2fa92eb62d229626c38 on Linux Mint 20 (x86_64).

nit:

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -456,7 +456,7 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, regtest: %u)", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_STRING, OptionsCategory::CONNECTION);
     argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-seednode=<ip>", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);

ACK e40cecf2c6fc88fe39849c3f291abcd165be0f25

Why the test for the -proxy command line option is placed in test_config_file_parser functiion?

-    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_STRING, OptionsCategory::CONNECTION);

That would break -noproxy, no? Is it worth doing?

Why the test for the -proxy command line option is placed in test_config_file_parser functiion?

Good point. Moved to newly introduced test_invalid_command_line_options.

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

• #10443 (Add fee_est tool for debugging fee estimation code 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.

ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7, I have tested the code.

re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7

That would break -noproxy, no?

Correct. Sorry for noise.

Concept ACK

Instead of asking the user to enter proxy details in correct format can we pause and give user the choice to continue without proxy by pressing any key or else follow the correct format to specify proxy?

Instead of asking the user to enter proxy details in correct format can we pause and give user the choice to continue without proxy by pressing any key or else follow the correct format to specify proxy?

No, we don't offer that kind of interaction in bitcoind, which is supposed to be used as a background process / daemon, not as a user facing program. For the GUI there is already an "options" dialog to set it which disallows invalid entry.