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

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/proxy changing 4 files +14 −13
  1. ryanofsky commented at 7:20 am on April 12, 2022: member

    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 #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 #15936, reported in #15936#pullrequestreview-937685759 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.

  2. in src/init.cpp:665 in 6a51a91932 outdated 
    659@@ -660,7 +660,8 @@ void InitParameterInteraction(ArgsManager& args)
660             LogPrintf("%s: parameter interaction: -connect set -> setting -listen=0\n", __func__);
661     }
662 
663-    if (args.IsArgSet("-proxy")) {
664+    std::string proxy_arg = args.GetArg("-proxy", "");
665+    if (proxy_arg != "" && proxy_arg != "0") {
    ryanofsky commented at 7:41 am on April 12, 2022:

    In commit “init: Allow -noproxy and -proxy=”" setting values" (6a51a9193236345fbd3ff06e9006fc2a6e7a61a4)

    Note: These lines are copied from AppInitMain: https://github.com/bitcoin/bitcoin/blob/2b5a741e98f186e50d9fbe1ceadcc8b8c91547f7/src/init.cpp#L1330-L1331

  3. DrahtBot added the label Utils/log/libs on Apr 12, 2022
  4. hebasto commented at 10:05 am on April 12, 2022: member
    Concept ACK.
  5. hebasto commented at 10:17 am on April 12, 2022: member
    Should -proxy= in the command line still ends with an error for reasons discussed in #20003?
  6. jonatack commented at 11:12 am on April 12, 2022: member
    Concept ACK
  7. joshuadbryant1985 approved
  8. brunoerg commented at 12:52 pm on April 12, 2022: member
    Concept ACK
  9. ryanofsky commented at 2:46 pm on April 12, 2022: member
    Renamed PR from “init: Allow -noproxy and -proxy=”" setting values" to “init: Allow -proxy=”" setting values" because I was wrong that -noproxy was broken before this PR. -noproxy was actually working correctly.
  10. ryanofsky commented at 3:20 pm on April 12, 2022: member

    Should -proxy= in the command line still ends with an error for reasons discussed in #20003?

    Bitcoin treats bare arguments (-txindex, -blocksonly, -server, etc) as being true, and it’s reasonable a user might see a bare -proxy argument and interpret that as meaning “use a proxy” instead of “don’t use a proxy”. That is the case which #20003 describes, and makes into an error to alert the user. This case is still an error after this PR.

    The problem is #20003 also forbids setting no proxy with -proxy= or -proxy="" shell arguments that are explicit assignments. I think it would be better in those cases to allow the assignments than disallow them. Obviously there is some chance a user could be confused in those cases as well, but keeping ability to assign an empty value makes sense logically, avoids needing to add special case logic in Qt to fix #15936#pullrequestreview-937685759, and is a nice convenience.

    More broadly, I think it is a bad thing that ArgsManager does not make a distinction between -foo and -foo= command line arguments. I think generally for boolean settings -mybool should be allowed and -mybool= should be treated as an ambiguous error. For string settings -mystring= should be allowed and -mystring should be treated as an ambiguous error. This PR enables making this distinction for the -proxy argument in a general way that could be applied to other string arguments. (It’s less general and flexible than #16545 but still compatible with it and other approaches.)

  11. ryanofsky renamed this:
    init: Allow -noproxy and -proxy="" setting values
    init: Allow -proxy="" setting values
    on Apr 12, 2022
  12. fanquake deleted a comment on Apr 12, 2022
  13. ryanofsky force-pushed on Apr 12, 2022
  14. ryanofsky commented at 6:23 pm on April 12, 2022: member
    Updated 6a51a9193236345fbd3ff06e9006fc2a6e7a61a4 -> 93e83a41fa8e4fd60c84fb2b14e1ba2a501b585a (pr/proxy.1 -> pr/proxy.2, compare) dropping InitParameterInteraction change and moving it to separate PR (#24837) Updated 93e83a41fa8e4fd60c84fb2b14e1ba2a501b585a -> c2417785824ee98b942665b8c2de8297ba26164b (pr/proxy.2 -> pr/proxy.3, compare) tweaking error message
  15. DrahtBot commented at 6:25 pm on April 12, 2022: member

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

    Conflicts

    No conflicts as of last run.

  16. ryanofsky force-pushed on Apr 12, 2022
  17. in src/util/system.h:178 in c241778582 outdated 
    174@@ -175,6 +175,7 @@ class ArgsManager
175         // ALLOW_STRING = 0x08,   //!< unimplemented, draft implementation in #16545
176         // ALLOW_LIST = 0x10,     //!< unimplemented, draft implementation in #16545
177         DISALLOW_NEGATION = 0x20, //!< disallow -nofoo syntax
178+        DISALLOW_ELISION = 0x40,  //!< disallow -foo syntax that doesn't assign any value
    hebasto commented at 2:35 pm on April 13, 2022:
    I like this idea.
  18. in src/util/system.cpp:373 in c241778582 outdated 
    369@@ -366,7 +370,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
370             return false;
371         }
372 
373-        std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val, *flags, error);
374+        std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val ? &*val : nullptr, *flags, error);
    hebasto commented at 2:38 pm on April 13, 2022:

    Why use nullptr in replace of nullopt_t?

    Maybe

     0--- a/src/util/system.cpp
 1+++ b/src/util/system.cpp
 2@@ -236,7 +236,7 @@ KeyInfo InterpretKey(std::string key)
 3  * [@return](/bitcoin-bitcoin/contributor/return/) parsed settings value if it is valid, otherwise nullopt accompanied
 4  * by a descriptive error string
 5  */
 6-static std::optional<util::SettingsValue> InterpretValue(const KeyInfo& key, const std::string* value,
 7+static std::optional<util::SettingsValue> InterpretValue(const KeyInfo& key, const std::optional<std::string> value,
 8                                                          unsigned int flags, std::string& error)
 9 {
10     // Return negated settings as false values.
11@@ -370,7 +370,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
12             return false;
13         }
14 
15-        std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val ? &*val : nullptr, *flags, error);
16+        std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val, *flags, error);
17         if (!value) return false;
18 
19         m_settings.command_line_options[keyinfo.name].push_back(*value);
20@@ -891,7 +891,7 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
21         KeyInfo key = InterpretKey(option.first);
22         std::optional<unsigned int> flags = GetArgFlags('-' + key.name);
23         if (flags) {
24-            std::optional<util::SettingsValue> value = InterpretValue(key, &option.second, *flags, error);
25+            std::optional<util::SettingsValue> value = InterpretValue(key, option.second, *flags, error);
26             if (!value) {
27                 return false;
28             }

    ?

    And the combined diff will smaller.

    ryanofsky commented at 4:25 pm on April 13, 2022:

    re: #24830 (review)

    Why use nullptr in replace of nullopt_t? … And the combined diff will smaller.

    Wow, I think you know me pretty well, because it is hard for me to resist a 2-line smaller diff! If other reviewers have a preference, I can change this, but I do like the plain pointer approach better. I generally don’t like making copies of things that can be pointed to, especially since val in this case is a user-supplied value that doesn’t have a predetermined size. I also like functions that take plain T* or T& types as inputs, because it means can just as efficiently be passed values whether they are coming from std::optional containers, other containers like std::map, or directly from the heap or stack. It’s harder to break code up into functions that can be composed if functions assume unnecessary things about their inputs. These are just mild preferences, though, so can change if anyone feels more strongly.

    luke-jr commented at 10:38 pm on April 14, 2022:

    I think the pointer approach is better, even if it makes the diff slightly bigger.

    Kinda surprised std::optional lacks an operator T* to return a pointer to its value or nullptr…

  19. hebasto commented at 2:42 pm on April 13, 2022: member
    Approach ACK c2417785824ee98b942665b8c2de8297ba26164b.
  20. hebasto approved
  21. hebasto commented at 4:31 pm on April 13, 2022: member
    ACK c2417785824ee98b942665b8c2de8297ba26164b
  22. luke-jr commented at 10:41 pm on April 14, 2022: member

    utACK

    But I would prefer a two-commit approach, fixing the bug simpler in commit 1, and refactoring in commit 2. (And only backporting the first commit)

  23. ryanofsky commented at 10:58 pm on April 14, 2022: member

    But I would prefer a two-commit approach, fixing the bug simpler in commit 1, and refactoring in commit 2. (And only backporting the first commit)

    This sounds appealing, but what is the simpler fix that could be backported? The problem is that ArgsManager currently stores -proxy= and -proxy command line arguments the exact same way with any way to distinguish them. So you need some change to ArgsManager in order to allow -proxy= again without allowing -proxy. So I could split this up into two commits, the first changing ArgsManager, and the second allowing -proxy= again, but you’d still need to backport both commits. (Also, I’m not sure this PR strictly needs to be backported, but I can see how it’s a good candidate.)

  24. luke-jr commented at 11:23 pm on April 14, 2022: member
    I thought maybe a default/compare with “\0"s, and/or a combination with GetBoolArg might do the trick, but you’re right, I don’t see a clean solution.
  25. luke-jr commented at 0:12 am on April 15, 2022: member

    This hack seems to work, but maybe not worth it? idk

     0diff --git a/src/init.cpp b/src/init.cpp
 1index a3d53c3fae9..8e8db27f676 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -98,6 +98,8 @@
 5 #include <zmq/zmqrpc.h>
 6 #endif
 7 
 8+using namespace std::literals::string_literals;
 9+
10 using node::CacheSizes;
11 using node::CalculateCacheSizes;
12 using node::ChainstateLoadVerifyError;
13@@ -1025,7 +1027,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
14 
15     nMaxTipAge = args.GetIntArg("-maxtipage", DEFAULT_MAX_TIP_AGE);
16 
17-    if (args.IsArgSet("-proxy") && args.GetArg("-proxy", "").empty()) {
18+    if (args.IsArgSet("-proxy") && args.GetArg("-proxy", "") == "\0"s) {
19         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
20     }
21 
22diff --git a/src/util/system.cpp b/src/util/system.cpp
23index aa9122106bd..4e89b4cbbd3 100644
24--- a/src/util/system.cpp
25+++ b/src/util/system.cpp
26@@ -78,6 +78,8 @@
27 #include <thread>
28 #include <typeinfo>
29 
30+using namespace std::literals::string_literals;
31+
32 // Application startup time (used for uptime calculation)
33 const int64_t nStartupTime = GetTime();
34 
35@@ -360,6 +362,11 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
36         std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val, *flags, error);
37         if (!value) return false;
38 
39+        if (is_index == std::string::npos && keyinfo.name == "proxy") {
40+            assert(value->get_str() == "");
41+            value = "\0"s;
42+        }
43+
44         m_settings.command_line_options[keyinfo.name].push_back(*value);
45     }
46
  26. ryanofsky commented at 0:40 am on April 15, 2022: member

    This hack seems to work, but maybe not worth it? idk

    This is pretty twisted and I love it. It solves the issue described minimally, like a key in a lock. But even the current full PR is only 14 lines, and I’m assuming straightforward to backport, and I think doesn’t even need to be backported, so I think there’s not a need for it to be so minimal. But the diff is good to have in case any issues come up, or to provide an example of how to usefully abuse char(0).

  27. luke-jr commented at 0:59 am on April 15, 2022: member
    (It might or might not break the actual proxy code tho)
  28. ryanofsky commented at 1:19 am on April 15, 2022: member

    (It might or might not break the actual proxy code tho)

    I was assuming it wouldn’t do that because it keeps the InitError, but it’s true init codepath is not straightforward so it’s not obvious this is safe

  29. MarcoFalke referenced this in commit 2074d7df20 on Apr 17, 2022
  30. sidhujag referenced this in commit 291694c25f on Apr 18, 2022
  31. luke-jr referenced this in commit 70dc6b65e5 on Apr 19, 2022
  32. luke-jr commented at 8:10 pm on April 19, 2022: member

    Suggest cherry-picking tests from https://github.com/bitcoin/bitcoin/commit/70dc6b65e5dfc203cbc25a47076d475158c5f8ea#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380

    Edit: Nevermind, those don’t work since all the settings are set

  33. DrahtBot added the label Needs rebase on Apr 26, 2022
  34. init: Allow -proxy="" setting values 
    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 https://github.com/bitcoin/bitcoin/pull/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
https://github.com/bitcoin/bitcoin/pull/15936, reported in
https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 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.
    1d4122dfef
  35. ryanofsky force-pushed on Apr 26, 2022
  36. ryanofsky commented at 2:33 pm on April 26, 2022: member
    Rebased c2417785824ee98b942665b8c2de8297ba26164b -> c657f005eb7ffd70f17a05c9405f6f29d40f3d57 (pr/proxy.3 -> pr/proxy.4, compare) due to conflict with #24789
  37. DrahtBot removed the label Needs rebase on Apr 26, 2022
  38. ryanofsky force-pushed on Apr 26, 2022
  39. ryanofsky commented at 6:35 pm on April 26, 2022: member
    Updated c657f005eb7ffd70f17a05c9405f6f29d40f3d57 -> 1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764 (pr/proxy.4 -> pr/proxy.5, compare) fixing accidentally reverted python test change in previous push
  40. ryanofsky commented at 7:25 pm on May 19, 2022: member

    Ping for maintainers. Can this be merged? It’s a small change that’s had two acks and a decent amount of discussion. It should be a straightforward (if minor) UX improvement, and it helps with #15936.

    (I can ask for more reviewers if needed. Just want to make sure this didn’t fall of the radar)

  41. hebasto approved
  42. hebasto commented at 8:46 pm on May 19, 2022: member

    re-ACK 1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764, only rebased since my recent review.

    Tested on Ubuntu 22.04.

  43. MarcoFalke merged this on May 20, 2022
  44. MarcoFalke closed this on May 20, 2022
  45. luke-jr referenced this in commit 17c0fc3fd8 on May 21, 2022
  46. sidhujag referenced this in commit b96201ffa0 on May 28, 2022
  47. DrahtBot locked this on May 20, 2023


ryanofsky hebasto jonatack joshuadbryant1985 brunoerg DrahtBot luke-jr

Labels
Utils/log/libs

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-24 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me