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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:error-on-empty-proxy changing 2 files +11 −0
  1. practicalswift commented at 12:34 pm on September 23, 2020: contributor

    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:

    0$ src/bitcoind -proxy
1…
22020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -listen=0
32020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -upnp=0
42020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
52020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
6…
72020-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:

    0$ src/bitcoind -proxy
1Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.
2$ echo $?
31
  2. practicalswift renamed this:
    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)
    on Sep 23, 2020
  3. laanwj commented at 12:39 pm on September 23, 2020: member

    Thank you. Tested ACK 3bca86ae6b7cc5b2f5bb9ee4567052b34f613ba1.

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

    re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7

  4. laanwj commented at 12:44 pm on September 23, 2020: member
    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.
  5. fanquake added the label P2P on Sep 23, 2020
  6. practicalswift commented at 1:46 pm on September 23, 2020: contributor
    @laanwj Added :)
  7. hebasto commented at 2:11 pm on September 23, 2020: member
    Concept ACK. Testing…
  8. in src/init.cpp:1180 in 3bca86ae6b outdated 
    1175@@ -1176,6 +1176,10 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1176 
1177     nMaxTipAge = args.GetArg("-maxtipage", DEFAULT_MAX_TIP_AGE);
1178 
1179+    if (args.IsArgSet("-proxy") && args.GetArg("-proxy", "").empty()) {
1180+        return InitError(Untranslated("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
    hebasto commented at 2:25 pm on September 23, 2020:

    3bca86ae6b7cc5b2f5bb9ee4567052b34f613ba1

    0        return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));

    as the message is presented to a user in GUI:

    Screenshot from 2020-09-23 17-22-22

    practicalswift commented at 2:39 pm on September 23, 2020:
    Done!
  9. hebasto changes_requested
  10. hebasto commented at 2:26 pm on September 23, 2020: member

    Tested f3ee9bd29d22e32356edb2fa92eb62d229626c38 on Linux Mint 20 (x86_64).

    nit:

     0--- a/src/init.cpp
 1+++ b/src/init.cpp
 2@@ -456,7 +456,7 @@ void SetupServerArgs(NodeContext& node)
 3     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 4     argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 5     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);
 6-    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 7+    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_STRING, OptionsCategory::CONNECTION);
 8     argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 9     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);
10     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
  11. practicalswift force-pushed on Sep 23, 2020
  12. hebasto approved
  13. hebasto commented at 2:48 pm on September 23, 2020: member

    ACK e40cecf2c6fc88fe39849c3f291abcd165be0f25

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

  14. net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server) 9b4fa0af40
  15. practicalswift force-pushed on Sep 23, 2020
  16. practicalswift commented at 3:44 pm on September 23, 2020: contributor 
    0-    argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
1+    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.

  17. DrahtBot commented at 4:21 pm on September 23, 2020: member

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

    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.

  18. kristapsk approved
  19. kristapsk commented at 10:34 pm on September 23, 2020: contributor
    ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7, I have tested the code.
  20. hebasto approved
  21. hebasto commented at 3:44 am on September 24, 2020: member

    re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7

    That would break -noproxy, no?

    Correct. Sorry for noise.

  22. unknown commented at 1:04 pm on September 25, 2020: none

    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?

  23. laanwj commented at 1:16 pm on September 29, 2020: member

    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.

  24. laanwj merged this on Sep 29, 2020
  25. laanwj closed this on Sep 29, 2020
  26. sidhujag referenced this in commit 788946df3e on Sep 29, 2020
  27. practicalswift deleted the branch on Apr 10, 2021
  28. Fabcien referenced this in commit eb09296a32 on Oct 29, 2021
  29. ryanofsky referenced this in commit 6a51a91932 on Apr 12, 2022
  30. ryanofsky referenced this in commit 93e83a41fa on Apr 12, 2022
  31. ryanofsky referenced this in commit c241778582 on Apr 12, 2022
  32. ryanofsky referenced this in commit fdbdcd744d on Apr 26, 2022
  33. ryanofsky referenced this in commit c657f005eb on Apr 26, 2022
  34. ryanofsky referenced this in commit 1d4122dfef on Apr 26, 2022
  35. MarcoFalke referenced this in commit 4a8709821e on May 20, 2022
  36. naumenkogs referenced this in commit 614afa5c42 on May 23, 2022
  37. janus referenced this in commit 8b785de388 on Aug 4, 2022
  38. DrahtBot locked this on Aug 18, 2022


practicalswift laanwj hebasto DrahtBot kristapsk

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me