Concept ACK.

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

Concept ACK

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.

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 (review), 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.)

Updated 6a51a91 -> 93e83a4 (pr/proxy.1 -> pr/proxy.2, compare) dropping InitParameterInteraction change and moving it to separate PR (#24837)
Updated 93e83a4 -> c241778 (pr/proxy.2 -> pr/proxy.3, compare) tweaking error message

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

Conflicts

No conflicts as of last run.

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

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.

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

diff --git a/src/init.cpp b/src/init.cpp
index a3d53c3fae9..8e8db27f676 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -98,6 +98,8 @@
 #include <zmq/zmqrpc.h>
 #endif
 
+using namespace std::literals::string_literals;
+
 using node::CacheSizes;
 using node::CalculateCacheSizes;
 using node::ChainstateLoadVerifyError;
@@ -1025,7 +1027,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 
     nMaxTipAge = args.GetIntArg("-maxtipage", DEFAULT_MAX_TIP_AGE);
 
-    if (args.IsArgSet("-proxy") && args.GetArg("-proxy", "").empty()) {
+    if (args.IsArgSet("-proxy") && args.GetArg("-proxy", "") == "\0"s) {
         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
     }
 
diff --git a/src/util/system.cpp b/src/util/system.cpp
index aa9122106bd..4e89b4cbbd3 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -78,6 +78,8 @@
 #include <thread>
 #include <typeinfo>
 
+using namespace std::literals::string_literals;
+
 // Application startup time (used for uptime calculation)
 const int64_t nStartupTime = GetTime();
 
@@ -360,6 +362,11 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
         std::optional<util::SettingsValue> value = InterpretValue(keyinfo, val, *flags, error);
         if (!value) return false;
 
+        if (is_index == std::string::npos && keyinfo.name == "proxy") {
+            assert(value->get_str() == "");
+            value = "\0"s;
+        }
+
         m_settings.command_line_options[keyinfo.name].push_back(*value);
     }
 

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

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

(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

Suggest cherry-picking tests from 70dc6b6#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380

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

Rebased c241778 -> c657f00 (pr/proxy.3 -> pr/proxy.4, compare) due to conflict with #24789

Updated c657f00 -> 1d4122d (pr/proxy.4 -> pr/proxy.5, compare) fixing accidentally reverted python test change in previous push

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)