Track negated options in the option parser

eklitzke commented at 2:43 am on March 18, 2018: contributor

This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a -no prefix. For example, -nofoo is the negated form of -foo. Negated options were originally added in the 0.6 release.

The change here allows code to explicitly distinguish between cases like -nofoo and -foo=0, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before.

The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the -debuglogfile option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in -nodebuglogfile and the code can see that it was explicitly negated, and use that to disable the log file.

This change originally split out from #12689.

fanquake added the label Utils/log/libs on Mar 18, 2018

meshcollider commented at 10:08 am on March 18, 2018: contributor

Looks good, thanks for splitting it out. utACK https://github.com/bitcoin/bitcoin/pull/12713/commits/6ad2a13cfdd46899c28f21899e107ee7d4c07877

MarcoFalke commented at 10:39 am on March 18, 2018: member

utACK 6ad2a13cfdd46899c28f21899e107ee7d4c07877 (Also noting that -a=no sets a to true, now)

eklitzke commented at 7:28 pm on March 18, 2018: contributor

Yeah I didn’t want to add support for strings like “no” “disabled” etc. because I don’t think there was ever an intention to support non-integer parameters in the first place. If we wanted to support that it’s easy enough. You can also use -noa, both now and before.

donaloconnor commented at 7:33 pm on March 18, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/6ad2a13cfdd46899c28f21899e107ee7d4c07877

jimpo commented at 10:24 pm on March 18, 2018: contributor

ACK 6ad2a13cfdd46899c28f21899e107ee7d4c07877.

in src/util.cpp:425 in 6ad2a13cfd outdated

421@@ -422,16 +422,14 @@ void ReleaseDirectoryLocks()
422 /** Interpret string as boolean, for argument parsing */
423 static bool InterpretBool(const std::string& strValue)
424 {
425-    if (strValue.empty())
426-        return true;
427-    return (atoi(strValue) != 0);
428+    // These values mean false, anything else (including empty string) is true.

promag commented at 10:51 pm on March 18, 2018:

Why not throw an error if it’s a stupid value? IMHO, for instance, -txindex=foo should fail.

MarcoFalke commented at 11:27 pm on March 18, 2018:

Thinking more about this, with proper argument validation we might as well reject false as a value for a boolean argument. This was never supported, so it feels weird to special case it now.

eklitzke commented at 4:31 am on March 19, 2018:

@promag Two reasons. First, such a change would not be backwards compatible (a minor point, see below). A better reason is that it enables the -printtoconsole change to have a way of disabling the debug log file without actually adding new command line arguments. In other words, I first test if -debuglogfile boolean parses as “false” (i.e. -nodebuglogfile or -debuglogfile=0), and if it’s not false I re-parse it as a string. There are already a lot of possible flags to give Bitcoin, and if we can re-use existing functionality without adding new flags that seems better to me. @MarcoFalke I actually agree, as accepting false opens the door for disabled, untrue, nil, void, zip, null, non, nein, ad infinitum.

My preferred semantics would be: -nofoo sets foo to false. Also -foo=0 sets foo to false. Which is exactly what the intention of the existing code seems to be. I only handled the false flag as there is existing code committed in the Bitcoin repository that relies on it.

I started this endeavor just wanting to get bitcoind printing to stdout when run in interactive mode. Whatever semantics allow that to happen are fine with me.

ryanofsky commented at 2:20 pm on March 19, 2018:

I first test if -debuglogfile boolean parses as “false” (i.e. -nodebuglogfile or -debuglogfile=0), and if it’s not false I re-parse it as a string.

Feel free to discount this opinion, but if the option is -debuglogfile=<filename>, I would hope that -debuglogfile=0 or -debuglogfile=false would just create log debug log files 0 or false respectively, instead of disabling logging.

Like João and Marco, I would also want ambiguous values passed as boolean options to trigger errors.

I guess I’m not so much a fan of option parsing trying to be very clever.

hkjn commented at 3:48 pm on March 19, 2018:

+1 to what @ryanofsky says; if backwards-compatability is not a constraint, doing the least clever thing seems most consistent and likely to lead to the most maintainable code, and long term least confusion for users.

(This PR seems like an improvement in readability of the argument parsing code, so any further simplification might be for another changeset.)

eklitzke force-pushed on Mar 19, 2018

MarcoFalke added the label Refactoring on Mar 19, 2018

MarcoFalke commented at 4:02 pm on March 19, 2018: member

Added “refactoring” label, since it doesn’t change behavior

utACK e944ec998fbd232b05ec9471ae6f19d1d32e0719

Would you mind updating the OP, which ends up in the merge commit?

jnewbery commented at 9:09 pm on March 19, 2018: member

since it doesn’t change behavior

This is a change in behaviour. If I take the changes to the unit test, but not the changes to util.cpp, the unit tests fail. If I take the changes to util.cpp, but not the change to wallet_resendwallettransactions.py, then wallet_resendwallettransactions.py fails.

EDIT: the reason for this is that atoi("false") will return 0, so -walletbroadcast=false is equivalent to -walletbroadcast=0`

atoi reference: http://www.cplusplus.com/reference/cstdlib/atoi/ . The first non-whitespace character in false is not a digit, so atoi(“false’) = 0

MarcoFalke removed the label Refactoring on Mar 19, 2018

eklitzke commented at 7:55 am on March 20, 2018: contributor

These are all easy to implement. I can make -debuglogfile=0 log to a file named 0, I can make it not log at all, I can special case -nodebuglogfile and have that not create a file, I can parse false as false or parse it as true, I can make a backwards compatible change and I can make a backwards incompatible change.

I’m not really sure what to do, as this has gotten an unusual amount of debate for something that is frankly quite banal. I got three different suggestions for how to disable the flag in the original PR:

-nodebuglogfile
-debuglogfile=0
-debuglogfile=/dev/null

If there is not consensus on whether we should change this I will just drop this change altogether and add a brand new flag in the original PR (-disablelogfile).

ryanofsky commented at 9:40 am on March 20, 2018: member

I’m not really sure what to do, as this has gotten an unusual amount of debate for something that is frankly quite banal. I got three different suggestions for how to disable the flag in the original PR:

From my reading of the feedback, I think if you go with -nodebuglogfile to disable logging, -debuglogfile=<file> to enable logging to a custom location, and -debuglogfile and -debuglogfile= to enable logging to the default location, you will have no objections. The ambivalence just came from zip/nein/0 stuff, which can be avoided by treating all nonempty string values the same.

hkjn commented at 11:27 am on March 20, 2018: contributor

ACK e944ec9.

This behavior as well as code seems slightly simpler than before, with more tests.

I’ll note we accept some pretty wacky combinations even after this change though, that we may want to make errors in the future, at least for boolean options:

0$ bitcoind -printtoconsole=0       # false
1$ bitcoind -noprinttoconsole       # false
2$ bitcoind -noprinttoconsole=no    # false
3$ bitcoind -noprinttoconsole=false # false
4$ bitcoind -noprinttoconsole=1     # false
5$ bitcoind -printtoconsole=1       # true
6$ bitcoind -printtoconsole=no      # true
7$ bitcoind -printtoconsole=false   # true
8$ bitcoind -noprinttoconsole=0     # true

eklitzke commented at 5:29 pm on March 20, 2018: contributor

@ryanofsky I will put up another version of this diff with that change.

eklitzke force-pushed on Mar 20, 2018

eklitzke commented at 7:23 pm on March 20, 2018: contributor

Handle arguments like -nofoo more explicitly.

This change keeps track of whether an option was passed with the -no semantics. This way in #12689 I can do something like:

0if (gArgs.IsArgInverted("-debuglogfile")) {
1   ....
2}

to see if the debug log file should be disabled.

I also removed some gratuitous boost and string copies. @jnewbery @MarcoFalke You guys figure out how you want the -foo=false case to be handled and let me know what you decide on.

eklitzke force-pushed on Mar 21, 2018

eklitzke commented at 5:41 am on March 21, 2018: contributor

Updated for more consistent naming – instead of “negated” and “inverted” I now always use the negated terminology.

eklitzke renamed this:
~~Fix unintuitive boolean option parsing behavior~~
Track negated options better
on Mar 21, 2018

eklitzke force-pushed on Mar 21, 2018

in src/util.h:257 in 6b95d6bd6e outdated

250+     * i.e. -nofoo.
251+     *
252+     * @param strArg Argument to get (e.g. "-foo")
253+     * @return true if the argument was passed negated
254+     */
255+    bool IsArgNegated(const std::string& strArg) const;

MarcoFalke commented at 12:31 pm on March 21, 2018:

I don’t think we should expose this in the code base. Historically, -nofoo always implied -foo=0, so use cases within the code base should always compare with the string "0" to check if foo was unset.

Yes, I know that that would forbid use cases, such as -myfile=0, but imo that is acceptable and desirable to keep previous behavior.

Edit: This is bike shedding, so feel free to ignore.

ryanofsky commented at 2:56 pm on March 21, 2018:

Historically, -nofoo always implied -foo=0, so use cases within the code base should always compare with the string “0” to check if foo was unset.

This is true for boolean options, and should still be true with this PR. I don’t think it was ever historically true that specifying “0” or “false” for path type options would disable those options. At least this isn’t the case currently for -conf -debuglogfile -pid -rpccookiefile -wallet or other path options. So whether or not special path strings are a good idea (I think they are a bad idea), I don’t think there is a historic argument for having them.

MarcoFalke commented at 3:59 pm on March 21, 2018:

I wanted to raise the point that we specifically check for the string “0” in some non-bool arguments to see if they are disabled. E.g. -connect=0 or -debug=0. So in case someone were to use IsArgNegated for those, it would break it.

Again, only bike-shedding here, so please ignore.

eklitzke commented at 1:03 am on March 22, 2018:

I agree with @ryanofsky that not being able to create a log file named “0” (or “false”) is weird. Tracking negation makes the arguments more strongly typed in a sense. I also felt like it made my printtoconsole change a little easier to understand, because it removes the code that calls GetBoolArg() on a non-boolean field.

MarcoFalke commented at 12:32 pm on March 21, 2018: member

re-utACK 6b95d6bd6e81030605d6b4ad42dd83cd3bc69bfd (Changes were to get rid of some boost usage and expose IsArgNegated, which I slightly object to)

jnewbery commented at 3:27 pm on March 21, 2018: member

Concept ACK the behaviour change. Making only the exact string “0” evaluate to false seems like better behaviour to me.

Tested ACK the negated argument change.

ACK the variable name changes.

Tested ACK the WIN32 boost::algorithm -> std::transform change on linux (by commenting out the #ifdef WIN32 in ArgsManager::ParseParameters() and IsSwitchChar()).

This needs release notes since it’s a subtle change to the way that arguments are parsed.

in src/util.cpp:423 in 6b95d6bd6e outdated

422+static bool InterpretBool(const std::string& val)
423 {
424-    if (strValue.empty())
425-        return true;
426-    return (atoi(strValue) != 0);
427+    return val != "0";

ryanofsky commented at 3:38 pm on March 21, 2018:

I think would be a little better to not change InterpretBool here. This seems making a backwards incompatible change in a PR that would otherwise be backwards compatible. If there is actually a reason to make a change here, it would be good to document it. I think the following table might summarize:

Argument	Previous	Current
`-foo`	true	true
`-foo=`	true	true
`-foo=1`	true	true
`-foo=0`	false	false
`-foo=-0`	false	true
`-foo=+0`	false	true
`-foo=00`	false	true
`-foo=bar`	false	true
`-foo=0bar`	false	true
`-foo=1bar`	true	true

Also, if you want to change this function here, I don’t think there is any downside to treating strings like false and no as false, since GetBoolArg() should only be called on bool options, not path options.

in src/util.cpp:476 in 6b95d6bd6e outdated

444+            // Double negatives like -nofoo=0 are supported (but discouraged)
445+            LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
446+        }
447+        key.erase(1, 2);
448+        m_negated_args.insert(key);
449+        val = bool_val ? "0" : "1";

ryanofsky commented at 3:53 pm on March 21, 2018:

Instead of munging the string value, it seems like it would be slightly better just to avoid inserting new entries into the arg maps when bool_val is true. But this would require updating Get*Arg functions to check the m_negated_args map in addition to the normal maps, so I understand why you didn’t do this here. And it’s pretty much an internal implementation detail, so it could be saved for a future improvement.

ryanofsky commented at 4:05 pm on March 21, 2018: member

utACK 6b95d6bd6e81030605d6b4ad42dd83cd3bc69bfd.

It seems to me though that this is doing two different things that don’t need to be tied to together:

Introducing a nice IsArgNegated API that provides a consistent way of disabling non-bool options, especially features that read or write to files.
Interpreting -foo=bar and similar strings as false instead of true.

Second change seems fine, but IMO would probably be better off in a separate PR. It could also be tweaked to accept more false strings, and should definitely be documented.

in src/test/util_tests.cpp:220 in 6b95d6bd6e outdated

216@@ -223,6 +217,54 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
217     BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
218 }
219 
220+BOOST_AUTO_TEST_CASE(util_GetBoolArg)

ryanofsky commented at 4:24 pm on March 21, 2018:

Great tests!

hkjn commented at 7:52 pm on March 21, 2018: contributor

Re-ACK 6b95d6b.

+1 on documenting the change in behavior for flags in release notes.

eklitzke commented at 1:41 am on March 22, 2018: contributor

Last night I had the same realization that @ryanofsky pointed out here, which is that the change to InterpretBoolArg() is no longer necessary at all, as my printtoconsole change now uses the new IsNegatedArg() method (also originally suggested by Russ). I am just going to revert all of the unnecessary changes, and then this change won’t change any backwards or forwards compatibility expectations. The code will still use atoi() as it does now. Users in an alternate universe who have txindex=-0 in their bitcoin.conf won’t have to worry about what happens if they upgrade to Bitcoin 0.17. I will add a comment in this area of the code summarizing the discussion here for the next person who wants to wade into this.

I do want to point out:

It’s pretty clear the the original authors of this code didn’t think about these edge cases, the existing tests don’t check any of them, and this code makes no sense at all when you start thinking about the cases we’ve talked about here.
The return value of atoi() is undefined if the string it’s given represents a value outside the range of INT_MIN to INT_MAX. So if we’re going to worry about edge cases, it’s not just that the existing code has strange semantics, it has undefined behavior. Almost all real-world computers today are LLP64 (Windows) or LP64 (Unix). This means that txindex=2147483647 is well-defined on most people’s computers, but txindex=2147483648 is not. In my opinion this is a bug, and fixing a bug of this nature requires changing backwards compatibility expectations (for some definition of backwards compatibility).
In almost any other part of the code, if we found out that Bitcoin behaved in an unexpected (or undefined) way when given pathological input we would fix it even though that’s not strictly backwards compatible. My opinion (and it is an opinion only) is that changing how the option parser interprets data that it didn’t expect to see is the same as changing other kinds of code that didn’t properly validate input. I don’t think there would be any hesitation to change code in other areas if we found out it didn’t properly validate input values or check return codes, and to me this is the same variety of change.

eklitzke force-pushed on Mar 22, 2018

eklitzke renamed this:
~~Track negated options better~~
Track negated options in the option parser
on Mar 22, 2018

eklitzke commented at 2:33 am on March 22, 2018: contributor

This branch has two commits:

The first is just a change to test_util.cpp with the new tests I added.
The second change adds the negated argument tracking, and more tests.

The reason this is two commits is to prove that the semantics of boolean option parsing is unchanged in this branch. I have tested both commits locally and both pass the test suite.

There are no longer any changes to InterpretBool(), other than comments summarizing the discussion here.

Rebasing and splitting out these commits is a lot of work. Please do not request additional changes unless there are any serious errors here.

ryanofsky commented at 1:01 pm on March 22, 2018: member

utACK ec1e226e33713366410e7d19a287be521169cd5f. The only change since my last review was reverting the InterpretBool diff and adding comments. Now this PR no longer changes behavior, only adding tests, comments, and a new ArgsManager::InterpretNegatedOption() method that makes it straightforward to implement options that can be disabled, like -debuglogfile=<path> / -nodebuglogfile.

Rebasing and splitting out these commits is a lot of work. Please do not request additional changes unless there are any serious errors here.

Thanks for your work. I usually only suggest changes, not request them. You should always feel free to ignore any suggestions that come from me, or disagree with them, or respond with boilerplate like “this seems like a good idea but [would delay other work I’m doing / could be implemented later / does not seem worth the required effort right now].”

jnewbery commented at 2:15 pm on March 22, 2018: member

ugh. Why? The previous branch fixed undesirable behaviour and already had 4 ACKs.

MarcoFalke commented at 2:42 pm on March 22, 2018: member

re-utACK ec1e226e33713366410e7d19a287be521169cd5f

Needs simple rebase, it seems.

ryanofsky commented at 3:08 pm on March 22, 2018: member

ugh. Why? The previous branch fixed undesirable behaviour and already had 4 ACKs.

To be fair, only 6 lines of code changed between the current and previous versions of this PR. 3 lines were reverted in InterpretBool, 1 line was reverted in wallet_resendwallettransactions.py, and 2 GetBoolArg checks in the test changed from true to false.

I don’t think the previous behavior change was an unambiguous improvment either, and it wasn’t documented or directly related to the feature introduced here.

jnewbery commented at 3:17 pm on March 22, 2018: member

Fine. Re-utACK ec1e226e33713366410e7d19a287be521169cd5f.

Agree that documentation should be added when the InterpretBool() behaviour is changed.

hkjn commented at 3:13 pm on March 23, 2018: contributor

ACK ec1e226.

Here’s the current interpretation of flags that I showed in my earlier comment (https://github.com/bitcoin/bitcoin/pull/12713#issuecomment-374563792), noting the differences from the earlier reviewed commit 6b95d6b:

0$ bitcoind -printtoconsole=0       # false 
1$ bitcoind -noprinttoconsole       # false 
2$ bitcoind -noprinttoconsole=1     # false 
3$ bitcoind -printtoconsole=no      # false (earlier true) 
4$ bitcoind -printtoconsole=false   # false (earlier true) 
5$ bitcoind -printtoconsole=1       # true 
6$ bitcoind -noprinttoconsole=no    # true (earlier false) 
7$ bitcoind -noprinttoconsole=0     # true 
8$ bitcoind -noprinttoconsole=false # true (earlier false)

MarcoFalke commented at 3:16 pm on March 23, 2018: member

@eklitzke Mind doing the rebase?

ryanofsky commented at 3:31 pm on March 23, 2018: member

@eklitzke, you may want to note in the PR description that this PR doesn’t change current behavior, except to now log warnings if double-negative -nofoo=0 options are encountered. The changes that this PR does make are:

Adding a convenient way to check for disabled, non-boolean option pairs like -debuglogfile=<path>, -nodebuglogfile.
Cleaning up code and variable names
Adding lots of comments.

jnewbery commented at 4:05 pm on March 23, 2018: member

Also:

removing a boost algorithm #include

MarcoFalke added the label Refactoring on Mar 27, 2018

Add additional tests for GetBoolArg()

This is meant to be an intermediate commit to prove that the next does not
introduce any changes in the semantics of boolean option parsing.

4f872b2450

Track negated arguments in the argument paser.

This commit adds tracking for negated arguments. This change will be used in a
future commit that allows disabling the debug.log file using -nodebuglogfile.

f7683cba7b

eklitzke force-pushed on Mar 28, 2018

eklitzke commented at 5:48 am on March 28, 2018: contributor

Rebased, tests passed locally for me. Travis has been kind of flaky recently so hopefully it gives me a green build. I’ve also updated the PR description.

hkjn commented at 12:27 pm on March 29, 2018: contributor

Re-utACK f7683cb.

promag commented at 12:44 pm on March 29, 2018: member

utACK f7683cb.

MarcoFalke commented at 3:05 pm on March 29, 2018: member

utACK f7683cba7b070b722a2e0641f4d1516112392ed6

MarcoFalke merged this on Mar 30, 2018

MarcoFalke closed this on Mar 30, 2018

MarcoFalke referenced this in commit 4490871ed7 on Mar 30, 2018

laanwj referenced this in commit 728e8c4dde on Aug 29, 2018

laanwj referenced this in commit 2739cff13a on Aug 29, 2018

laanwj referenced this in commit 9e7ddf97d5 on Aug 29, 2018

laanwj referenced this in commit e9a78e9b3b on Sep 5, 2018

MarcoFalke referenced this in commit 95d731d9b3 on Sep 5, 2018

MarcoFalke referenced this in commit 2936dbc557 on Sep 5, 2018

HashUnlimited referenced this in commit 2d0f2e338f on Sep 14, 2018

uhliksk referenced this in commit da0356d1ef on Oct 18, 2018

jfhk referenced this in commit 3765cd7fff on Nov 14, 2018

HashUnlimited referenced this in commit d91956abc4 on Nov 26, 2018

UdjinM6 referenced this in commit d52809132a on Apr 29, 2020

PastaPastaPasta referenced this in commit 6289bda49e on Apr 30, 2020

PastaPastaPasta referenced this in commit 7a89b916d1 on May 9, 2020

furszy referenced this in commit de5b240881 on Oct 28, 2020

ckti referenced this in commit 51e01f00f5 on Mar 28, 2021

gades referenced this in commit 5886e19161 on Jun 30, 2021

Munkybooty referenced this in commit 492bc9374c on Jul 1, 2021

Munkybooty referenced this in commit e07f2cf0fd on Jul 1, 2021

Munkybooty referenced this in commit 9b3d0a2c97 on Jul 2, 2021

Munkybooty referenced this in commit 44b6238ddb on Jul 3, 2021

UdjinM6 referenced this in commit fe87c21bf2 on Jul 3, 2021

Munkybooty referenced this in commit a7f6bc83c5 on Jul 3, 2021

UdjinM6 referenced this in commit 76378771c5 on Jul 3, 2021

Munkybooty referenced this in commit 13eb54157c on Jul 7, 2021

Munkybooty referenced this in commit 7405b236e2 on Jul 8, 2021

MarcoFalke locked this on Sep 8, 2021

Track negated options in the option parser #12713