net: Exit with error message if -proxy is specified without arguments (instead of continuing without proxy server) by practicalswift · Pull Request #20003 · bitcoin/bitcoin

practicalswift · 2020-09-23T12:34:44Z

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

laanwj · 2020-09-23T12:39:04Z

Thank you. Tested ACK 3bca86ae6b7cc5b2f5bb9ee4567052b34f613ba1.

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

re-ACK 9b4fa0a

laanwj · 2020-09-23T12:44:56Z

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.

practicalswift · 2020-09-23T13:46:51Z

@laanwj Added :)

hebasto · 2020-09-23T14:11:05Z

Concept ACK. Testing...

hebasto

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);

src/init.cpp

hebasto

ACK e40cecf2c6fc88fe39849c3f291abcd165be0f25

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

…stead of continuing without proxy server)

practicalswift · 2020-09-23T15:44:12Z

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

DrahtBot · 2020-09-23T16:21:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

kristapsk

ACK 9b4fa0a, I have tested the code.

hebasto

re-ACK 9b4fa0a

That would break -noproxy, no?

Correct. Sorry for noise.

ghost

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?

laanwj · 2020-09-29T13:16:03Z

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.

…ied without arguments (instead of continuing without proxy server) 9b4fa0a net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server) (practicalswift) Pull request description: 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 ``` ACKs for top commit: laanwj: re-ACK 9b4fa0a kristapsk: ACK 9b4fa0a, I have tested the code. hebasto: re-ACK 9b4fa0a Tree-SHA512: 4ba7a011991699a54b5bb87ec68367c681231bf5dcd36f8c89ff9ddc2e8d29df453817b7e362597e652ad6b341a22b7274be0fd78d435e5f0fd8058e5221c4ce

…stead of continuing without proxy server) Summary: This is a backport of [[bitcoin/bitcoin#20003 | core#20003]] Test Plan: `ninja && test/functional/test_runner.py feature_config_args` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10400

This drops the "No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>" error when "-noproxy" or "-proxy=" command line arguments are specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin#20003 to prevent a bare "-proxy" argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking "-noproxy" and "-proxy=" arguments as well. The motivation for this change is actually to prevent a GUI bug that happens with bitcoin#15936, reported in bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message, and make parsing of the "-proxy" setting handling consistent within the InitParameterInteraction, AppInitParameterInteraction, and AppInitMain functions.

This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin#15936, reported in bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.

1d4122d init: Allow -proxy="" setting values (Ryan Ofsky) Pull request description: This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin/bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin/bitcoin#15936, reported in bitcoin/bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message. ACKs for top commit: hebasto: re-ACK 1d4122d, only rebased since my recent [review](bitcoin/bitcoin#24830 (review)). Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53

This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin/bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin/bitcoin#15936, reported in bitcoin/bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.

practicalswift changed the title ~~net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server)~~ net: Exit with error message if -proxy is specified without arguments (instead of continuing without proxy server) Sep 23, 2020

fanquake added the P2P label Sep 23, 2020

hebasto requested changes Sep 23, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the error-on-empty-proxy branch from f3ee9bd to e40cecf Compare September 23, 2020 14:39

hebasto approved these changes Sep 23, 2020

View reviewed changes

net: Print error message if -proxy is specified without arguments (in…

9b4fa0a

…stead of continuing without proxy server)

practicalswift force-pushed the error-on-empty-proxy branch from e40cecf to 9b4fa0a Compare September 23, 2020 15:42

kristapsk approved these changes Sep 23, 2020

View reviewed changes

hebasto approved these changes Sep 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 24, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

ghost reviewed Sep 25, 2020

View reviewed changes

laanwj merged commit 8aa6178 into bitcoin:master Sep 29, 2020

practicalswift deleted the error-on-empty-proxy branch April 10, 2021 19:42

ryanofsky mentioned this pull request Apr 12, 2022

init: Allow -proxy="" setting values #24830

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

net: Exit with error message if -proxy is specified without arguments (instead of continuing without proxy server)#20003
laanwj merged 1 commit intobitcoin:masterfrom
practicalswift:error-on-empty-proxy

practicalswift commented Sep 23, 2020 •

edited

Loading

Uh oh!

laanwj commented Sep 23, 2020 •

edited

Loading

Uh oh!

laanwj commented Sep 23, 2020 •

edited

Loading

Uh oh!

practicalswift commented Sep 23, 2020

Uh oh!

hebasto commented Sep 23, 2020

Uh oh!

hebasto left a comment

Uh oh!

Uh oh!

hebasto left a comment

Uh oh!

practicalswift commented Sep 23, 2020

Uh oh!

DrahtBot commented Sep 23, 2020

Uh oh!

kristapsk left a comment

Uh oh!

hebasto left a comment

Uh oh!

ghost left a comment •

edited by ghost

Loading

Uh oh!

laanwj commented Sep 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

practicalswift commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 23, 2020

Uh oh!

hebasto commented Sep 23, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 23, 2020

Uh oh!

DrahtBot commented Sep 23, 2020

Conflicts

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Sep 23, 2020 •

edited

Loading

laanwj commented Sep 23, 2020 •

edited

Loading

laanwj commented Sep 23, 2020 •

edited

Loading

ghost left a comment •

edited by ghost

Loading