refactor: Implement missing error checking for ArgsManager flags

ryanofsky commented at 2:13 pm on August 4, 2019: contributor

Trigger startup errors if bitcoin is configured with bad setting values according to flags. Also raise internal errors if settings are registered and retrieved with inconsistent flags.

This change has no effect on behavior because the new ArgsManager flags added here are not used outside of tests yet.

It’ll probably be more productive to start applying type checking flags to new arguments than existing arguments. But for examples of how type checking flags make sense applied to some existing arguments see:

Type	Example
ALLOW_BOOL	4719738f602a681eb0d1633fbb1651f42cc93129
ALLOW_INT	7f7d82b521b7d78ea9eccab18f068e2881eefafc
ALLOW_INT \| ALLOW_BOOL	6865a198f5db30bd494b3a2540f47ee728963908
ALLOW_STRING	51ca84ecfcad3b8b2a85d75c8c1da61d97c2ca9b
ALLOW_STRING \| ALLOW_LIST	012a320038158a532bd3662c99cd8211941659d2

ryanofsky commented at 2:14 pm on August 4, 2019: contributor

FYI @hebasto, this adds error checking for the flags from #16097

ryanofsky force-pushed on Aug 4, 2019

in src/util/system.h:137 in 1efb9ee41a outdated

138+        NONE = 0x0, //<! Indicates flag lookup failed.
139+
140+        /* Low level validation flags. In most cases these should be avoided,
141+         * and standard TYPE_* flag combinations below should be preferred. */
142+        ALLOW_ANY      = 0x01, //!< disable validation
143+        ALLOW_NEGATED  = 0x02, //!< allow -nofoo and -nofoo=1

hebasto commented at 2:52 pm on August 4, 2019:

Nice to see ALLOW_NEGATED flag again ;)

ryanofsky commented at 8:55 am on August 7, 2019:

re: #16545 (review)

Nice to see ALLOW_NEGATED flag again ;)

It is ok to have this, but like the comment above it says, it should only be used in rare cases, where a negated setting needs to be treated differently than a false/0/empty bool/int/string setting. Normal code for normal options should never have to use ALLOW_NEGATED or IsArgNegated.

in src/util/system.h:149 in 1efb9ee41a outdated

147+        ALLOW_BOOL     = 0x20, //!< allow -foo=0 and -foo=1
148+
149+        /* Standard value types. */
150+        TYPE_STRING = ALLOW_NONEMPTY,
151+        TYPE_OPTIONAL_STRING = ALLOW_NONEMPTY | ALLOW_NEGATED | ALLOW_EMPTY,
152+        TYPE_INT = ALLOW_INT | ALLOW_NEGATED,

hebasto commented at 2:54 pm on August 4, 2019:

Could we drop ALLOW_NEGATED for TYPE_INT now or in the future?

ryanofsky commented at 8:55 am on August 7, 2019:

#16545 (review)

Could we drop ALLOW_NEGATED for TYPE_INT now or in the future?

This seems like a bad thing to provide as a shortcut. If an option is an integer, treating -nonumber exactly the same as -number=0 provides a simple interface to users consistent with what we’ve always provided in the past. Treating these the same is also less error prone for developers than treating them differently. It is possible there may be exceptions where treating -nonumber differently from -number=0 would be a good thing, but I’d honestly be surprised to see one, and ALLOW_ flags and IsArgNegated function are available if this is actually desired.

In case the reason you’re asking about this is because you want range checking for integer options that shouldn’t accept 0, I think it’d be better to implement that in a way that disallows both -nonumber and -number=0 settings, not just one of them. This could be added with a TYPE_POSITIVE_INT shortcut, or letting AddArg take validation callbacks or options structs (IntType{}.MinValue(1).MaxValue(65535)) in place of flags.

hebasto commented at 3:48 pm on August 10, 2019:

If an option is an integer, treating -nonumber exactly the same as -number=0 provides a simple interface to users…

Such practice is a bad habit, IMO.

… consistent with what we’ve always provided in the past. Treating these the same is also less error prone for developers than treating them differently.

Agree. It is important.

DrahtBot commented at 2:59 pm on August 4, 2019: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj
Approach NACK	ajtowns
Stale ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29043 (fuzz: make FuzzedDataProvider usage deterministic by martinus)

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.

DrahtBot added the label Tests on Aug 4, 2019

DrahtBot added the label Utils/log/libs on Aug 4, 2019

hebasto commented at 3:21 pm on August 4, 2019: member

It seems that not providing a new InterpretNegated() function has some benefits:

no need for key_name local variables
diff gets much smaller

ryanofsky force-pushed on Aug 5, 2019

ryanofsky marked this as ready for review on Aug 5, 2019

in src/util/system.cpp:274 in 00973301f6 outdated

259@@ -260,8 +260,60 @@ class ArgsManagerHelper {
260         }
261         return InterpretBool(found_result.second); // is set, so evaluate
262     }
263+
264+    static inline void CheckFlags(const ArgsManager& am, const std::string& key, unsigned int require_any)
265+    {
266+        unsigned int flags = am.FlagsOfKnownArg(key);
267+        if (flags != ArgsManager::NONE && ((flags & require_any) == 0)) {

promag commented at 11:32 pm on August 6, 2019:

00973301f6a1efb6aa2232544a49b080ac0c6c50

Looks like this should be an assertion?

0        if (flags) assert(flags & require_any);

ryanofsky commented at 8:55 am on August 7, 2019:

re: #16545 (review)

Looks like this should be an assertion?

I know you didn’t check the tests yet, but the main reason for this being a logic_error instead of an assert is so the test can require and check this logic so it won’t be accidentally broken in the future. The other reason this is a logic_error is to provide clearer feedback to a developer who accidentally sets the wrong flag or calls the wrong GetArg overload, by providing the option names and flags values in error messages.

I tdo think it makes sense to use assert/abort instead of logic_error in cases where the error is critical (affecting consensus, or data integrity) and it’s important to abort unconditionally and not take a risk that a caller may catch the error and handle it inappropriately. But this case is less critical than that.

Also this case really can and should be a compile error, not a runtime error. But it will take some more work and scripted-diffs to make that happen.

promag commented at 10:53 am on August 7, 2019:

Actually I did check and ran the tests with my change and I understand the idea of testing the exception, which wouldn’t make sense if it was an assertion.

Also this case really can and should be a compile error

Agree.

in src/util/system.h:144 in 1efb9ee41a outdated

145+        ALLOW_NONEMPTY = 0x08, //!< allow -foo=bar
146+        ALLOW_INT      = 0x10, //!< allow -foo=123
147+        ALLOW_BOOL     = 0x20, //!< allow -foo=0 and -foo=1
148+
149+        /* Standard value types. */
150+        TYPE_STRING = ALLOW_NONEMPTY,

promag commented at 11:59 pm on August 6, 2019:

1efb9ee41ab228a73d293603838951af6fb0f59c

It would be nice to see a commit elsewhere using these “types”.

ryanofsky commented at 8:55 am on August 7, 2019:

re: #16545 (review)

It would be nice to see a commit elsewhere using these “types”.

This is a good idea. I can open a draft PR applying these to some wallet flags, since I know hebasto already has good work on node flags like -datadir, -blocksdir, -pid etc that I don’t want to interfere with. I do think any PR actually using flags should be merged after this one since changing flag behavior after flags are already in use would be unnecessarily confusing and risky.

promag commented at 10:54 am on August 7, 2019:

Maybe a brief example in the PR description then?

ryanofsky commented at 1:34 pm on August 8, 2019:

Maybe a brief example in the PR description then?

Another really good idea! Will add this.

promag commented at 0:01 am on August 7, 2019: member

Looks good, read the code and tried locally. Will review tests, but for now just a couple of comments.

ryanofsky commented at 10:35 am on August 7, 2019: contributor

Thanks for reviews!

re: #16545 (comment)

It seems that not providing a new InterpretNegated() function has some benefits:

no need for key_name local variables

diff gets much smaller

The function is only 9 lines long, so I don’t see it having a very big impact on the size of the diff, but there are a few reasons why I think it’s important for this to be separate and not part of FlagsOfKnownArg:

FlagsOfKnownArg is now called at argument retrieval time, not jut argument parsing time, so it’s more efficient and easier to reason about if it is only doing a pure map lookup without string manipulation and parsing.
Having this be a separate function makes InterpretOption shorter and more understandable. Instead of parsing option names, parsing option values, and dealing with flags, it now only parses option values and no longer deals with flags or names at all.
Getting rid of the InterpretNegated function may reduce the size of this diff, but it would increase the diff in #15934, because it would leave more code for that PR to deduplicate and update. It’s nice to take care of some work for #15934 when the changes make sense here as well.

ryanofsky force-pushed on Aug 7, 2019

promag commented at 10:58 am on August 7, 2019: member

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn’t tell if it’s known or not. I’d reword to GetArgFlags.

ryanofsky commented at 1:57 pm on August 8, 2019: contributor

Promag’s suggestion to work on a commit using the flags #16545 (review) was really useful. Turns out after trying this out with wallet flags, the empty string check for TYPE_STRING is a lot less useful than I thought it would be, so I’m going to simplify the flags a little and get rid of it.

It does seem good to just start off with a minimal set of checks,, and then in the future when we think of checks that actually are useful in practice (maybe checks for ip addresses, timestamps, hex strings, value ranges) we can add them at that point.

I’m going to:

Make a little followup PR applying new types to some wallet args
Tweak flags a little in this PR to be less crazy about empty strings
Add example of using flags in the PR description as suggested and link to wallet flag PR
Improve comments, make /* Standard value types. */ comment more descriptive and add notes to IsArgSet and IsArgNegated methods explaining how there’s no need to call these if the argument has a type declared.

Actual changes to this PR will be very small, so it still should be reviewable now.

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn’t tell if it’s known or not. I’d reword to GetArgFlags.

Both seem good to me, but get GetArgFlags does seem a little more standard, so I’ll rename if @hebasto also says it’s better or just as good.

hebasto commented at 4:09 pm on August 10, 2019: member

Concept ACK 01ca54a2411ff8f39fa10974327e882141140739 @promag

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn’t tell if it’s known or not. I’d reword to GetArgFlags.

For unknown arguments it returns ArgsManager::NONE. @ryanofsky

Both seem good to me, but get GetArgFlags does seem a little more standard, so I’ll rename if @hebasto also says it’s better or just as good.

Naming is the hardest part of coding ;) Agree with @promag’s suggestion about renaming.

in src/util/system.h:131 in 01ca54a241 outdated

128@@ -129,12 +129,23 @@ class ArgsManager
129 {
130 public:
131     enum Flags {

hebasto commented at 4:56 pm on August 10, 2019:

Could the underlying type be specified?

0     enum Flags : unsigned int {

Ref: #16097 (review)

ryanofsky commented at 4:40 pm on August 14, 2019:

Could the underlying type be specified?

Done in 9a1e20eef36b9dfff9de7780b0edd30b8ae92487

ryanofsky force-pushed on Aug 13, 2019

ryanofsky referenced this in commit 0779ce57d4 on Sep 9, 2019

ryanofsky force-pushed on Sep 9, 2019

ryanofsky referenced this in commit 36f2aded05 on Sep 9, 2019

ryanofsky commented at 8:49 pm on September 9, 2019: contributor

Updated c7d018d78ef0b78891fbba1c02b775fe156e0a1d -> b5e8b2a406900bb8d3f8bac1992f5367bbb3f2c9 (pr/argcheck.5 -> pr/argcheck.6, compare) with changes described in #16545#pullrequestreview-272573893.

Agree with @promag’s suggestion about renaming.

Now renamed in 0779ce57d4d97040108c81c75bc9a2b332a74ed1.

laanwj commented at 10:58 am on October 23, 2019: member

Concept ACK, consistent argument error checking is good.

DrahtBot added the label Needs rebase on Oct 28, 2019

ryanofsky referenced this in commit 959096cbac on Nov 14, 2019

ryanofsky force-pushed on Nov 14, 2019

in src/util/system.cpp:487 in 4b8149b5aa outdated

409@@ -350,11 +410,28 @@ Optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) const
410     return nullopt;
411 }
412 
413+bool ArgsManager::CheckArgFlags(const std::string& name,
414+    unsigned int require,
415+    unsigned int forbid,
416+    const char* context) const

ryanofsky commented at 5:01 am on November 14, 2019:

In commit “Implement missing error checking for ArgsManager flags” (4b8149b5aae874cffe81423a2d4529c6f5b6e944)

Note: comment describing this function is in the header file shown below on github

DrahtBot removed the label Needs rebase on Nov 14, 2019

ryanofsky referenced this in commit 6974a2fecb on Nov 14, 2019

ryanofsky referenced this in commit e1dd793a94 on Nov 14, 2019

ryanofsky force-pushed on Nov 14, 2019

ryanofsky renamed this:
~~Implement missing error checking for ArgsManager flags~~
refactor: Implement missing error checking for ArgsManager flags
on Nov 24, 2019

ryanofsky referenced this in commit dc0f148074 on Dec 18, 2019

ryanofsky referenced this in commit 425bb30725 on Dec 18, 2019

ryanofsky referenced this in commit cba2710220 on Dec 18, 2019

maflcko referenced this in commit 6677be64f6 on Dec 19, 2019

DrahtBot added the label Needs rebase on Dec 19, 2019

ryanofsky force-pushed on Dec 19, 2019

DrahtBot removed the label Needs rebase on Dec 19, 2019

sidhujag referenced this in commit 33fe88ae5b on Dec 19, 2019

in src/util/system.h:179 in 67518f7cc6 outdated

174@@ -174,8 +175,9 @@ class ArgsManager
175     /**
176      * Get setting value.
177      *
178-     * Result will be null if setting was unset, true if "-setting" argument was passed
179-     * false if "-nosetting" argument was passed, and a string if a "-setting=value"
180+     * Result will be null if setting was unset, true if "-setting" argument
181+     * was passed false if "-nosetting" argument was passed, and a string,

promag commented at 10:30 pm on December 22, 2019:

nit, comma before false?

ryanofsky commented at 10:17 pm on January 6, 2020:

re: #16545 (review)

nit, comma before false?

Thanks, fixed

in src/util/system.cpp:231 in 67518f7cc6 outdated

211+ * Return an error string and nullopt if an invalid value was provided that
212+ * isn't allowed by the flags, otherwise return the parsed value.
213  */
214-static bool CheckValid(const std::string& key, const util::SettingsValue& val, unsigned int flags, std::string& error)
215+static Optional<util::SettingsValue> InterpretValue(const std::string& key,
216+    const std::string* value,

promag commented at 11:42 pm on December 22, 2019:

nit, I’ve changed this to Optional<string>, see https://github.com/promag/bitcoin/commit/71da58bc9437b977e56029bd8d2516bcf61cb823.

ryanofsky commented at 10:17 pm on January 6, 2020:

re: #16545 (review)

nit, I’ve changed this to Optional<string>, see promag@71da58b.

Using an optional wrapper instead of a simple const pointer seems a little worse to me (less efficient, more verbose), but I wouldn’t object if there’s an advantage I’m not seeing.

promag commented at 4:45 pm on January 9, 2020:

Yeah don’t bother

in src/util/system.h:166 in 67518f7cc6 outdated

137-        ALLOW_BOOL = 0x01,
138-        ALLOW_INT = 0x02,
139-        ALLOW_STRING = 0x04,
140-        ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
141+    enum Flags : unsigned int {
142+        ALLOW_ANY = 0x01,    //!< disable validation

promag commented at 11:43 pm on December 22, 2019:

AddArg could disallow any other flag when this one is used?

ryanofsky commented at 10:17 pm on January 6, 2020:

re: #16545 (review)

AddArg could disallow any other flag when this one is used?

Thanks, added checks for useless flag combinations in AddArg and tests for the checks in util_CheckBadFlagCombinations

promag commented at 11:43 pm on December 22, 2019: member

I’ve changed https://github.com/promag/bitcoin/commit/71da58bc9437b977e56029bd8d2516bcf61cb823

in src/util/system.h:180 in 67518f7cc6 outdated

174@@ -174,8 +175,9 @@ class ArgsManager
175     /**
176      * Get setting value.
177      *
178-     * Result will be null if setting was unset, true if "-setting" argument was passed
179-     * false if "-nosetting" argument was passed, and a string if a "-setting=value"
180+     * Result will be null if setting was unset, true if "-setting" argument
181+     * was passed false if "-nosetting" argument was passed, and a string,
182+     * integer, or boolean depending on ALLOW_ flags if a "-setting=value"

hebasto commented at 4:34 pm on January 3, 2020:

0     * integer, or boolean depending on ALLOW_{BOOL|INT|STRING} flags if a "-setting=value"

ryanofsky commented at 10:18 pm on January 6, 2020:

re: #16545 (review)

Thanks, included suggestion

hebasto approved

hebasto commented at 11:40 am on January 6, 2020: member

ACK 67518f7cc61bf59ddfa0fd7c8dbbdec3653b9556, tested on Linux Mint 19.3.

0$ ./src/test/test_bitcoin --run_test=util_tests/util_CheckValue
1Running 1 test case...
2
3*** No errors detected

0$ ./src/test/test_bitcoin --run_test=util_tests/CheckSingleValue
1Running 1 test case...
2
3*** No errors detected

There are two non-blocking suggestions:

add some tests:

 0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index 539e1a972..215057227 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -316,6 +316,8 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
 5     CheckValue(M::ALLOW_BOOL, "-value=1", Expect{true}.Bool(true));
 6     CheckValue(M::ALLOW_BOOL, "-value=2", Expect{{}}.Error("Can not set -value value to '2'. It must be set to 0 or 1"));
 7     CheckValue(M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to 0 or 1"));
 8+    CheckValue(M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to 0 or 1"));
 9+    CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to 0 or 1"));
10 
11     CheckValue(M::ALLOW_INT, nullptr, Expect{{}}.DefaultInt().DefaultBool());
12     CheckValue(M::ALLOW_INT, "-novalue", Expect{false}.Int(0).Bool(false));
13@@ -371,6 +373,8 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
14     CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool());
15     CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=2", Expect{"2"}.String("2").DefaultBool());
16     CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=abc", Expect{"abc"}.String("abc").DefaultBool());
17+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=true", Expect{"true"}.String("true").DefaultBool());
18+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=false", Expect{"false"}.String("false").DefaultBool());
19 
20     CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, nullptr, Expect{{}}.List({}));
21     CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue", Expect{false}.List({}));

move ArgManager class and related functions to its own file (in another PR)

hebasto commented at 12:04 pm on January 6, 2020: member

Is @MarcoFalke’s suggestion still relevant?

ryanofsky force-pushed on Jan 7, 2020

ryanofsky commented at 8:15 pm on January 7, 2020: contributor

Updated 67518f7cc61bf59ddfa0fd7c8dbbdec3653b9556 -> 27beca5ffee664360005f74123dad9107769d048 (pr/argcheck.9 -> pr/argcheck.10, compare) with various suggested changes. Thanks for the reviews and feedback!

re: #16545#pullrequestreview-338599308

0+    CheckValue(M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to 0 or 1"));
1+    CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to 0 or 1"));

Thanks, added these checks in a new test (util_CheckBoolStringsNotSpecial).

re: #16545 (comment)

Is @MarcoFalke’s suggestion still relevant?

Yes, the suggestion still applies to master branch and is now incorporated in this PR.

in src/test/util_tests.cpp:409 in 27beca5ffe outdated

404+    CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to 0 or 1."));
405+
406+    // Similarly, check "true" and "false" are not treated specially when
407+    // ALLOW_BOOL is combined with ALLOW_INT and ALLOW_STRING. (The only
408+    // difference ALLOW_BOOL makes for int and string arguments is that it
409+    // enables "-foo" syntax with no equal sign assiging explicit int or string

hebasto commented at 8:48 pm on January 7, 2020:

assiging ==> assigning

ryanofsky commented at 9:50 pm on January 7, 2020:

re: #16545 (review)

assiging ==> assigning

Thanks, fixed

hebasto approved

hebasto commented at 9:23 pm on January 7, 2020: member

re-ACK 27beca5ffee664360005f74123dad9107769d048, tested on Linux Mint 19.3:

0$ ./src/test/test_bitcoin --run_test=util_tests/util_CheckValue
1Running 1 test case...
2
3*** No errors detected

0$ ./src/test/test_bitcoin --run_test=util_tests/util_CheckBoolStringsNotSpecial
1Running 1 test case...
2
3*** No errors detected

0$ ./src/test/test_bitcoin --run_test=util_tests/util_CheckSingleValue
1Running 1 test case...
2
3*** No errors detected

0$ ./src/test/test_bitcoin --run_test=util_tests/util_CheckBadFlagCombinations
1Running 1 test case...
2
3*** No errors detected

0$ ./src/bitcoind -nolisten=0 -nowallet=0
12020-01-07T21:22:29Z Warning: parsed potentially confusing double-negative -listen=0
22020-01-07T21:22:29Z Warning: parsed potentially confusing double-negative -wallet=0
3...

ryanofsky force-pushed on Jan 7, 2020

ryanofsky commented at 9:52 pm on January 7, 2020: contributor

Thanks for re-ack!

Updated 27beca5ffee664360005f74123dad9107769d048 -> 5bb512aa51ae46350f8527ff7b3817dd719bb455 (pr/argcheck.10 -> pr/argcheck.11, compare) with suggested spelling fix

hebasto approved

hebasto commented at 10:25 pm on January 7, 2020: member

ACK 5bb512aa51ae46350f8527ff7b3817dd719bb455, just fix a typo.

hebasto commented at 10:28 pm on January 7, 2020: member

A note for future: it seems worth adding a functional test for “Warning: parsed potentially confusing double-negative …” in the debug.log.

nm - it is already, see #17893

ajtowns commented at 5:28 am on January 9, 2020: contributor

Rather than changing foo= from being foo=0 to becoming foo=<whatever foo's default was> it might be clearer to write reset-foo for that behaviour (like -nofoo), so that it can be consistent no matter what the type of the setting is?

ryanofsky commented at 7:24 pm on January 9, 2020: contributor

Thanks for looking at this!

Rather than changing foo= from being foo=0 to becoming foo=<whatever foo's default was>

I don’t think that’s really an accurate description. -foo="" is currently interpreted as true for boolean arguments, 0 for integer arguments, and all kinds of random ways for string arguments. The PR doesn’t change this, or affect any existing behavior.

What the PR does is implement flags that let the GetArg family of functions retrieve settings in a consistent way, so when you are trying to retrieve a setting that has a default value, you can just call a GetArg function and not have to deal with IsArgSet IsArgNegated or do manual parsing to get sensible behavior. After porting lots of existing arguments and dealing with bugs like #15864, I think that sensible behavior in most cases means treating -nofoo like false/0/"" for bool/int/string arguments, allowing -foo syntax for bool arguments but not most int/string arguments, and treating empty -foo="" settings like unset settings.

These are just defaults, though. Since #15934, the internal representation of settings is always unambiguous and doesn’t throw away information, and callers should easily be able to implement any behavior they choose when interpretting settings.

it might be clearer to write reset-foo for that behaviour (like -nofoo), so that it can be consistent no matter what the type of the setting is?

I think this could be a reasonable feature to add in its own PR. Reset settings could be represented as null setting values. The GetArg functions already always treat null settings like unset settings, regardless of flags.

in src/util/system.cpp:751 in 5bb512aa51 outdated

504@@ -427,6 +505,16 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
505     if (flags & ArgsManager::NETWORK_ONLY) {
506         m_network_only_args.emplace(arg_name);
507     }
508+
509+    if ((flags & (ALLOW_BOOL | ALLOW_INT | ALLOW_STRING)) && (flags & ALLOW_ANY)) {

ajtowns commented at 2:35 pm on January 23, 2020:

Seems like you might as well have a check for:

0if ((flags & (ALLOW_BOOL | ALLOW_INT | ALLOW_STRING)) && (flags & ALLOW_LIST))

here as well – if you add LIST, you can’t use GetArg to query for a string or int, or GetBoolArg to query for a bool value?

ryanofsky commented at 8:02 pm on January 24, 2020:

re: #16545 (review)

Seems like you might as well have a check for:
0if ((flags & (ALLOW_BOOL | ALLOW_INT | ALLOW_STRING)) && (flags & ALLOW_LIST)) 
here as well – if you add LIST, you can’t use GetArg to query for a string or int, or GetBoolArg to query for a bool value?

If the suggestion is make it impossible to accept lists of strings, I don’t see how that makes sense. If the suggestion is to make it impossible to accept lists of bools or ints, I don’t think that is a good idea. ArgsManager API for retrieving typed lists could be improved but it isn’t something that needs to be explicitly supported or forbidden in this PR

ajtowns commented at 3:42 am on January 28, 2020:

If you want to support lists of bools or ints later, then it’s easy to remove the restriction then; if lists of bools or ints is supported now, it should be tested along with the other cases in util_tests.cpp – but it doesn’t seem like it makes sense to say that it’s supported now when we’ve only got std::vector<std::string> GetArgs() to query it.

ryanofsky commented at 11:47 pm on September 1, 2020:

re: #16545 (review)

If you want to support lists of bools or ints later, then it’s easy to remove the restriction then; if lists of bools or ints is supported now, it should be tested along with the other cases in util_tests.cpp – but it doesn’t seem like it makes sense to say that it’s supported now when we’ve only got std::vector<std::string> GetArgs() to query it.

Am not inclined to add a new restriction that only complicates the implementation and doesn’t provide a benefit to end users or developers trying to use the API. There is no missing test coverage here either. Writing this would be just analogous to building a roadblock in front a road that hasn’t been built.

in src/util/system.cpp:478 in 5bb512aa51 outdated

473@@ -398,15 +474,17 @@ bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strVa
474 
475 bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue)
476 {
477-    if (fValue)
478-        return SoftSetArg(strArg, std::string("1"));
479-    else
480-        return SoftSetArg(strArg, std::string("0"));
481+    LOCK(cs_args);
482+    CheckArgFlags(strArg, /* require= */ ALLOW_BOOL, /* forbid= */ 0, __func__);

ajtowns commented at 2:36 pm on January 23, 2020:

Seems like this should have /* forbid= */ ALLOW_LIST to match GetBoolArg ?

ryanofsky commented at 8:02 pm on January 24, 2020:

re: #16545 (review)

Seems like this should have /* forbid= */ ALLOW_LIST to match GetBoolArg ?

Added suggested restriction

ajtowns commented at 4:15 pm on January 23, 2020: contributor

What the PR does is implement flags that let the GetArg family of functions retrieve settings in a consistent way,

The way I think of args at the moment is that ArgsManager stores a convenient representation of the config file (so it parses out the “-nofoo” stuff, but otherwise just stores strings) before it’s converted into typed info, and then the GetArg functions do that conversion so that the setting can actually be used, often dropping the result into a global/module variable so it doesn’t have to be repeatedly converted.

I guess what I’ve got in mind is eventually ending up with something where args get statically declared along with the variable they should stored in, and parsing/merging the config files and command line just fills in the variables, so that instead of GetBoolArg('-addrmantest', false) you just evaluate g_addrmantest and get your answer more directly. I suppose that would mean instead of querying IsArgSet mean you’d use a std::optional<bool> instead of just bool etc; but otherwise it would give you compile time type checking and mean that you’d be writing the default values where you define the parameters rather than every place you try to access them. (I don’t know if this is all actually possible, it’s just what I’m hoping for)

Anyway, with or without that context, I’m not really seeing that much value in adding the type information while ArgsManager is storing the info; it’s easy enough to say that "" should be interpreted as 0 if asked for as an int and true if asked for as a bool, but when you want to convert the "" to an int, and then convert that int to a bool because you’ve got ALLOW_INT | ALLOW_BOOL set, that starts becoming pretty weird. Weird enough that you’ve implemented it so that in that case -value= returns the defaults instead; but that’s then a user-visible behaviour change for anyone who was using -boolparam= to set it true if it’s default happens to be false when -boolparam is converted from ANY to BOOL.

To try and fully understand your PR, I’ve split it up into bitesize chunks that make sense to me on their own – https://github.com/ajtowns/bitcoin/commits/202001-argsmanflag . It gets most of the PR done, but doesn’t do the coversion to int within SettingsValue generally, so keeps the same semantics for all the calls that wouldn’t pass an error back to the user. I’m not really seeing enough value in those different semantics to justify the change given the chance of breaking someone’s config isn’t zero…

ajtowns commented at 4:15 pm on January 23, 2020: contributor

Couple of potential nits

ryanofsky commented at 7:02 pm on January 23, 2020: contributor

What the PR does is implement flags that let the GetArg family of functions retrieve settings in a consistent way,

The way I think of args at the moment is that ArgsManager stores a convenient representation of the config file (so it parses out the “-nofoo” stuff, but otherwise just stores strings) before it’s converted into typed info, and then the GetArg functions do that conversion so that the setting can actually be used, often dropping the result into a global/module variable so it doesn’t have to be repeatedly converted.

I guess what I’ve got in mind is eventually ending up with something where args get statically declared along with the variable they should stored in, and parsing/merging the config files and command line just fills in the variables, so that instead of GetBoolArg('-addrmantest', false) you just evaluate g_addrmantest and get your answer more directly. I suppose that would mean instead of querying IsArgSet mean you’d use a std::optional<bool> instead of just bool etc; but otherwise it would give you compile time type checking and mean that you’d be writing the default values where you define the parameters rather than every place you try to access them. (I don’t know if this is all actually possible, it’s just what I’m hoping for)

Yes, I have the same thing in mind. But again, like I said in #17580#pullrequestreview-340791706, switching from untyped settings to typed settings is the hard part because it has to do be done on a case-by-case basis (see wallet flag commits in the PR description). Switching from a dynamic representation of types to a static representation of types is a straightforward thing to do after types are in place.

Anyway, with or without that context, I’m not really seeing that much value in adding the type information while ArgsManager is storing the info

I’ve tried to clearly motivate this PR and the PRs that depend on it (#17493 #17580 #17581 and #17783). I’m not trying to convince everyone of everything, but I am happy to try to address specific concerns that can be articulated.

it’s easy enough to say that "" should be interpreted as 0 if asked for as an int and true if asked for as a bool, but when you want to convert the "" to an int, and then convert that int to a bool because you’ve got ALLOW_INT | ALLOW_BOOL set, that starts becoming pretty weird. Weird enough that you’ve implemented it so that in that case -value= returns the defaults instead; but that’s then a user-visible behaviour change for anyone who was using -boolparam= to set it true if it’s default happens to be false when -boolparam is converted from ANY to BOOL.

I’m having a hard time following this and I probably don’t know how to convince someone that something is weird or not weird. I put a lot of thought into the flags, and did a lot of experimentation with the wallet commits linked to in the description, and have a result that I think:

Provides good error and reporting useful behavior to end users
Does not require a boilerplate IsArgSet/IsArgNegated nonsense code in places where arguments are accessed
Is backwards compatible in all cases except some specific and obscure corner cases, and clearly documented where not backwards compatible

To try and fully understand your PR, I’ve split it up into bitesize chunks that make sense to me on their own – https://github.com/ajtowns/bitcoin/commits/202001-argsmanflag . It gets most of the PR done, but doesn’t do the coversion to int within SettingsValue generally, so keeps the same semantics for all the calls that wouldn’t pass an error back to the user.

Can you give a specific example of how this is an improvement? If you can say how it improves rhe wallet changes linked in the PR description, or any of the followup PRs actually making use of these flags, that would be most helpful.

I’m not really seeing enough value in those different semantics to justify the change given the chance of breaking someone’s config isn’t zero…

I’m sure you know this, but just so someone reading doesn’t get the wrong idea, this PR is 100% backwards compatible, and is only defining new type flags that are used in future improvements

ajtowns commented at 9:04 am on January 24, 2020: contributor

https://github.com/ajtowns/bitcoin/commits/202001-argsmanflag

Can you give a specific example of how this is an improvement?

It’s not so much meant as an improvement, but more as “your PR is X=A+B+C, the A+B parts make sense to me, but the C part doesn’t and even seems like a step in the wrong direction, and A+B alone seems to do what the PR was aiming to do – ie, implement missing error checking for ArgsManager flags”.

I’ve added a couple more commits on top now, so that (a) it actually does more of the error checking for the flags and (b) it’s easy to see from the test suite that changing from ANY to BOOL etc only makes some things errors, rather than changing the meaning of user’s configurations. Both these veer more substantially away from your patch than where I was yesterday; consider it executable whiteboarding, I guess?

I’m still uncomfortable with making working configs start giving errors, but changing the meaning of potentially already existing configs really needs a good justification that I’m not seeing. I realise this PR alone doesn’t do that, but it sets up future PRs to do so in ways that aren’t necessarily super-obvious.

I think the approach I linked above would make it much more straightforward to switch away from ALLOW_ANY sooner, since it just means you can say “a bunch of things that were previously allowed in config files that weren’t at all sensible (double negatives, repeatedly setting the same option, giving strings where ints are needed or numbers other than 0 or 1 where a bool is needed, or saying “true” to set a bool to false) will now give errors at startup” and be done.

ryanofsky commented at 12:50 pm on January 24, 2020: contributor

AJ, this critique does not seem substantive because it is too general. Would it be possible for you to cite a specific deficiency in my design, or in any of my PRs, or in any of my wallet flag porting commits that lead to bad user behavior, or an unergnonomic API for developers? I’d be more than happy to adopt suggestions for improvement. Or if we disagree about a specific design decision, I could explain the tradeoffs behind it, and what made me favor one approach. But when you do things like casually state that something in my PR goes in the “wrong direction,” without even telling me what you’re referring to, it does not seem productive.

Again, to be clear to anyone else reading this: This PR is 100% backwards compatible. It adds a new developer feature which I put a lot of thought and effort into designing, testing, and making use of in followup PRs and commits prioritizing 1) backwards compatibilty, 2) good error reporting for users, 3) flexibility for users about how to specify options, for example supporting -foo and -nofoo valueless syntax whenever not ambiguous 4) having ergonomic code at GetArg call sites and avoiding current ubiquitous IsArgNegated IsArgset bugs 5) having ability to add future improvements like custom validation functions and typed int/string/bool/vector<x>/Optional<x>/Variant<x> setting storage

I’m pretty satisfied with design of this PR and happy with the way it functions by itself, but the actual test of the PR is when the features it adds are put to use in followup PRs. This PR by itself is a starting point that doesn’t set anything in stone. If it has deficiencies, they can be addressed now or addressed in future PRs, it’s not some hugely high stakes thing where everything has to be worked out in advance, even though I’ve definitely tried to work out everything in advance, and you can look at all the actual use cases in the followups to see that

ryanofsky commented at 9:19 pm on January 24, 2020: contributor

Updated 5bb512aa51ae46350f8527ff7b3817dd719bb455 -> f42da6bbe3a327a7abe9c29ac1278bd652e86513 (pr/argcheck.11 -> pr/argcheck.12, compare) with suggested softsetbool check

ryanofsky force-pushed on Jan 24, 2020

ajtowns commented at 4:02 am on January 28, 2020: contributor

Would it be possible for you to cite a specific deficiency in my design, or in any of my PRs, or in any of my wallet flag porting commits that lead to bad user behavior, or an unergnonomic API for developers?

I don’t know if I can cite a problem you’ll accept, your PRs are no-ops (“this change has no effect”) because you’re modifying dead code, and you seem to be rejecting criticisms that only take place once the code is used… I think:

making -value= reset things to defaults changes the meaning of existing configs, and while that might be fine if it were a new system, it doesn’t seem useful enough to justify introducing an incompatibility
you’re doing a lot more things in this patch than just implementing missing errors (and given this code doesn’t apply to any existing options, arguably aren’t implementing the missing errors yet!)
it would be better to have ALLOW_BOOL etc work exactly the same way as ALLOW_ANY; change the argument declarations types in the first PR; and then change the meaning of ALLOW_BOOL etc to provide better errors in follow up PRs; that makes it easy to see what the effects of the new behaviours you’re coding up actually is
rather than bit flags it would be better to have a type enumeration something like: BOOL, INT, STRING, OPTIONAL_STRING, STRING_LIST. At that point having unique “GetArg” functions for each type returning exactly the right type seems straightforward; so Optional<std::string> dbg_log = GetOptionalStringArg("-debuglogfile", DEFAULT_DEBUG_LOGFILE); m_print_to_file = dbg_log ? true : false; m_file_path = dbg_log ? *dbg_log : "";
that the IsArgNegated and IsArgSet warts aren’t a priority – they’re largely encapsulated in system.cpp

EDIT: the first one of these prevents me from giving an ACK at least without seeing a good reason for it, the second one made it hard to review, the latter are just IMO

in src/test/util_tests.cpp:504 in f42da6bbe3 outdated

367+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
368+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
369+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value", Expect{true}.DefaultString().Bool(true));
370+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultString().DefaultBool());
371+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=0", Expect{"0"}.String("0").DefaultBool());
372+    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool());

ajtowns commented at 4:04 am on January 28, 2020:

This seems very strange; I don’t see how having “value=1”, GetBoolArg("-value", false)==false for an arg that’s meant to accept a bool or a string could possibly make sense (likewise for value=0, with GetBoolArg("-value", true)==true).

ryanofsky commented at 11:47 pm on September 1, 2020:

re: #16545 (review)

This seems very strange; I don’t see how having “value=1”, GetBoolArg("-value", false)==false for an arg that’s meant to accept a bool or a string could possibly make sense (likewise for value=0, with GetBoolArg("-value", true)==true).

I think this is just strange if you are not thinking about use cases. The use case for BOOL|STRING is for /imperative/ options like -bind -connect -upgrade -reindex where you want naked options with no value to be accepted and you also want verbose options with string values to be accepted. The BOOL flag is what allows the naked option, and the STRING flag is what allows the string option. Just as with any other STRING option, you are responsible for parsing the string and ArgsManager is not going to interpret it for you. Making the change you suggest would require either choosing a new SettingsValue representation or making it impossible to to distinguish -setting from -setting="", and would make the implementation more complicated, and would have no actual utility.

ajtowns commented at 3:12 am on August 20, 2021:

-bind and -connect both support multiple options (require=STRING+LIST for GetArgs) but this is forbidden for GetBoolArg (forbid=LIST); -reindex, -reindexchainstate, and -rescan all don’t seem to be used as anything but a bool as far as I can see. The comments also mention -listen which also seems to be only used as a boolean arg. That does leave -upgradewallet (presuming that’s what -upgrade is referring to) though that option was replaced by an RPC a while ago.

There’s no need to change the SettingsValue representation – if you can differentiate different values using GetBoolArg/GetArg, then you can differentiate them via some different api too.

ryanofsky commented at 7:39 pm on August 21, 2021:

re: #16545 (review)

-bind and -connect

We can go down a rabbit hole about these individual cases, but this is nitpicking which I do not think is relevant to the point I am making about imperative options (which trigger behavior and don’t require but can accept string values) being useful to handle differently from string options whose purpose is to set values.

you can differentiate them via some different api

Right, I do not want to change the API and I do not want to change the representation. The suggestion is not a good suggestion. Even if it was a good suggestion, it would expand the scope of the PR. Even if it was a good suggestion, it could done separately as a followup.

ajtowns commented at 7:10 am on September 1, 2021:

We can go down a rabbit hole about these individual cases, but this is nitpicking which I do not think is relevant to the point I am making about imperative options (which trigger behavior and don’t require but can accept string values)

I went through all the examples I could find of potential “imperative options”, and none of them fit your definition, except for the one that’s been removed. I don’t think “this feature won’t be used at all so there’s no point implementing it” is nitpicking.

DrahtBot added the label Needs rebase on Feb 5, 2020

HashUnlimited referenced this in commit 7a97631cc5 on Apr 17, 2020

HashUnlimited referenced this in commit 02a6fd02c4 on Apr 17, 2020

HashUnlimited referenced this in commit 51d492e9c1 on Apr 17, 2020

deadalnix referenced this in commit 619943a5c9 on Apr 30, 2020

maflcko removed the label Tests on Aug 28, 2020

ryanofsky force-pushed on Sep 2, 2020

DrahtBot removed the label Needs rebase on Sep 2, 2020

ryanofsky force-pushed on Sep 2, 2020

sidhujag referenced this in commit 96245fcfb0 on Nov 10, 2020

DrahtBot added the label Needs rebase on Jan 14, 2021

in src/util/system.cpp:359 in 45beb3bc1a outdated

355@@ -296,7 +356,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
356 #endif
357 
358         if (key == "-") break; //bitcoin-tx using stdin
359-        std::string val;
360+        Optional<std::string> val;

fanquake commented at 2:04 am on March 15, 2021:

Please use std::optional in new code.

ryanofsky force-pushed on Mar 22, 2021

DrahtBot removed the label Needs rebase on Mar 23, 2021

ryanofsky force-pushed on Mar 24, 2021

backpacker69 referenced this in commit 7f404e8d42 on Mar 28, 2021

backpacker69 referenced this in commit 87096f79e1 on Mar 28, 2021

backpacker69 referenced this in commit ba6f9db598 on Mar 28, 2021

ryanofsky force-pushed on Mar 29, 2021

DrahtBot added the label Needs rebase on Apr 20, 2021

ryanofsky force-pushed on Jun 2, 2021

DrahtBot removed the label Needs rebase on Jun 2, 2021

ryanofsky commented at 0:59 am on August 19, 2021: contributor

re: #16545 (comment)

making -value= reset things to defaults changes the meaning of existing configs

No, there is impact no impact on existing settings, which are not consistent in how they treat these values. All this is doing is establishing a convention that lets new settings behave consistently and usefully in the future by default (if you want inconsistent behavior in the future, that is still possible using ALLOW_STRING and std::string::empty()).

you’re doing a lot more things in this patch than just implementing missing errors

The only change in this PR is adding missing validation for argsmanager flags and raising new errors.

it would be better to have ALLOW_BOOL etc work exactly the same way as ALLOW_ANY

I don’t want to change the meaning of the flag over many PRs. I want implement the flag once. If there is something unclear or bad about the behavior that’s being implemented, obviously that would be bad. But it would also be fixable. Clearly I’ve put in the effort here to write extensive tests and apply the flag to real world use-cases in the wallet (see table in PR description), so I don’t think we need to be paralyzed by speculative and vague concerns.

rather than bit flags it would be better to have a type enumeration

I started off thinking similarly until I looked at actual use cases, again see the table in the PR description. There is a balance between having no validation and complete freedom, and rigid validation and not enough freedom to be used everywhere. This PR is taking a step of adding some validation, with a level of API freedom justified by existing use cases, again see table. If you need the API to be more rigid, you can make it more rigid in future PRs. If you need the API to have a different interface without changing the functionality, we can also do that in future PRs. I would like to make the API more ergonomic in the future and would like to check more things at compile time instead of runtime. But the API currently uses bit flags, and bit flags work fine, so this PR is continuing to use bit flags.

that the IsArgNegated and IsArgSet warts aren’t a priority – they’re largely encapsulated in system.cpp

This is not the case. These APIs are widely misused and make the developer and users experiences worse. See actual usages 9c0e6975f42a1197a7b898b19d8af81a03868123 048a4ee606a370d51620e39aecc8f6d4a132c31c and commits in the table in the PR description.

Coincidentally, in a PR I reviewed today there were two new incorrect set/negated usages: #20487 (review), #20487 (review) and I’m sure you could find more fresh examples of incorrect usages in open and merged PRs.

EDIT: the first one of these prevents me from giving an ACK at least without seeing a good reason for it

Again, the reason is just to provide consistent default behavior for future -value="" settings. Current settings are not affected. Future settings that want to override the behavior are not affected. Fixation on this case seems strange to me, and I think it is possibly standing in for some other concern (aesthetic differences? a different understanding of current conventions?) that has not been articulated.

ryanofsky commented at 1:26 am on August 19, 2021: contributor

Rebased 2772948473df34181535558f6b105ca413730053 -> 0a4ded27b7613656f423a07d889a9648b72200e7 (pr/argcheck.22 -> pr/argcheck.23, compare) adding a scripted-diff commit to actually ensure backwards compatibility. Updated 0a4ded27b7613656f423a07d889a9648b72200e7 -> aa3dec078fc464d7ea2b46088c789bfd4a7e2637 (pr/argcheck.23 -> pr/argcheck.24, compare) retaining “Negating of -%s is meaningless” error for compatibility with #19827.

ryanofsky force-pushed on Aug 19, 2021

ryanofsky commented at 6:42 pm on August 19, 2021: contributor

(deleted comment meant for other PR #19827 (comment))

maflcko commented at 6:44 pm on August 19, 2021: member

I looks like the scripted-diff does change behavior, given that at least ALLOW_INT has some additional checks attached?

ryanofsky commented at 6:48 pm on August 19, 2021: contributor

I looks like the scripted-diff does change behavior, given that at least ALLOW_INT has some additional checks attached?

Yeah, it’s been a while I forgot this had dropped the “negating is meaningless” check in some cases. I’ll update the comment to say it avoids breaking existing configs, not that it avoids changing behavior entirely.

ajtowns commented at 5:28 am on August 20, 2021: contributor

Approach NACK aa3dec078fc464d7ea2b46088c789bfd4a7e2637

re: #16545 (comment)

making -value= reset things to defaults changes the meaning of existing configs No, there is impact no impact on existing settings,

There is no impact on existing settings because ALLOW_BOOL, ALLOW_INT, ALLOW_STRING and ALLOW_LIST are unused outside of tests, and that’s only because you’ve specifically converted them all to ALLOW_ANY. If they were to be used, which is presumably the idea, there would be an impact. If it’s not the idea to switch existing options from ALLOW_ANY to the more specific things – which will impact existing settings – then this approach just makes config handling more complicated because of the different behaviour between currently existing settings and the new ones.

(I haven’t re-reviewed to see if there really isn’t an impact on existing settings given those changes)

I started off thinking similarly until I looked at actual use cases, again see the table in the PR description.

The only entry for the “(anything) or BOOL” types is no longer relevant as far as I can see, the only “LIST” type is “LIST of STRING”; and list options can only be access via GetArgs which returns a list of strings anyway. It was worth exploring using flags, but you were right the first time.

that the IsArgNegated and IsArgSet warts aren’t a priority – they’re largely encapsulated in system.cpp This is not the case. These APIs are widely misused and make the developer and users experiences worse. See actual usages 9c0e697

I don’t agree with -nofoo sometimes being “override explicit config settings back to the default”, and particularly having -nosignetchallenge as another way of saying “default signet challenge” doesn’t make much sense to me. So I don’t find that commit super convincing…

EDIT: the first one of these prevents me from giving an ACK at least without seeing a good reason for it

Again, the reason is just to provide consistent default behavior for future -value="" settings. Current settings are not affected. Future settings that want to override the behavior are not affected. Fixation on this case seems strange to me, and I think it is possibly standing in for some other concern (aesthetic differences? a different understanding of current conventions?) that has not been articulated.

I feel like I’ve been pretty verbose already (though if you want to chat further offline or something, I’m happy to) but as far as I can see you’re just ignoring my concerns. All you’re doing in this PR is implementing a new API for arguments, but you’re dismissing criticism of that API (vs the current one) as “too general” because, being unused, the API doesn’t impact “user behaviour”. You could defend gets() on the same grounds – it works great so long as it’s not actually used and you can always implement a better API later. So I really don’t know what else to say. I don’t think it’s a good idea to merge an API and then fix it later after you start trying to use it.

respond to that depressing review comment

I guess I’m sorry I looked at this PR. I like the idea of better typing of config options and improved error handling, and figured reviewing PRs about that was a positive step.

ryanofsky referenced this in commit c53e8f0894 on Aug 21, 2021

ryanofsky marked this as a draft on Aug 21, 2021

ryanofsky commented at 8:37 pm on August 21, 2021: contributor

Setting draft status. Will rebase this on #22766 soon to make this PR smaller and eliminate all confusion/questions/bugs related to backwards compatibility.

Approach NACK aa3dec0

I don’t think this NACK is justified. This PR is implementing an internal API. We disagree on minor points of the API design, notably some default behaviors which can be overridden by API callers. I think we both agree that the API will provide useful features to developers for providing clearer error messages and more consistency when parsing settings, both for new settings where there are not backwards compatibility concerns, and selectively for existing settings like #19827 (and maybe in examples from the PR description), where we think the usability benefits outweigh the compatibility concerns.

In every case where the NACK has raised a specific concern with the API, I’ve always responded with specifics, and have also always reiterated the broad point that because this is an internal API, we can change it and make different choices in the future without ever having to affect end-user backwards compatibility in any case.

There is no impact on existing settings […] this approach just makes config handling more complicated because of the different behaviour between currently existing settings and the new ones.

Thanks for acknowledging there is no impact on existing settings. Now we can stop talking so much about backwards compatibility, and I can stop repeating I am not breaking backwards compatibility.

At the end of this paragraph you are hinting about what the real concern may be, which is a concern about complexity and inconsistency. This is a very subjective thing to begin with, and I think it has never been substantiated with specific examples. It is also ignoring existing footguns and complexity which this PR is trying to address, see #17783 #17508 and #17493 for real examples. Even if any part of this concern is valid, it seems overblown when you try to stack up handwavy disagreements about internal design decisions with concrete improvements made and enabled by this PR.

I don’t agree with -nofoo sometimes

I suspect would not be useful to respond to this, but I can respond if you believe there are implications for this PR. You are saying ArgSet/Negated warts aren’t a priority. I am saying why I think they are worth fixing. I think we can agree to disagree about this if it is as tangential to the PR as it seems to be.

dismissing criticism of that API (vs the current one) as “too general” because, being unused, the API doesn’t impact “user behaviour”.

I defended the API design I chose for the default behavior of these flags AND I’m saying the default behavior does not affect exisitng options and can be overriden where needed. If we ever want to change the default API behavior in the future, we can do this in the future, and never have to break end-user compatibility. I would just like to try my approach to this API design here, which I think is thoughtful and grounded in real world use-cases.

I guess I’m sorry I looked at this PR. I like the idea of better typing of config options and improved error handling, and figured reviewing PRs about that was a positive step.

I should clarify I think it’s definitely a good thing that you’ve reviewed the PR, and the fault is totally mine for getting discouraged and failing to reply to comments promptly.

ryanofsky force-pushed on Aug 23, 2021

ryanofsky referenced this in commit 40243483de on Aug 24, 2021

ryanofsky referenced this in commit eca00d8dfd on Aug 24, 2021

ryanofsky referenced this in commit d5ab248929 on Aug 24, 2021

ryanofsky referenced this in commit 4332297f62 on Aug 26, 2021

ryanofsky referenced this in commit 3ffbb3b828 on Aug 26, 2021

ryanofsky force-pushed on Aug 26, 2021

ajtowns referenced this in commit ae72150769 on Sep 1, 2021

ajtowns commented at 7:00 am on September 1, 2021: contributor

I don’t think this NACK is justified.

To summarise: 1) This PR doesn’t improve behaviour (in this case, actually issuing errors/warnings when people make mistakes in the config, or removing any of the footguns for any existing option), and we should be prioritising patches that do improve things. For example, an earlier version of this API has already spent over a year merged without getting us the intended benefits during that time, and instead has resulted in needing extra PRs to revert people trying to use it. That’s totally backwards. 2) When introducing a new API we should be using it immediately, both because if it’s better then using it is an immediate win, and because that demonstrates the new API actually works in practice, not just in theory. 3) I don’t think the new API as of the commit id I referenced is particularly great, since the obvious way to use it means existing options and new ones will behave differently in subtle and confusing ways.

For comparison, #22766 changes a bunch less code and simplifies the API in a way that’s immediately used and (IMHO) obviously useful, so I’d much rather move the focus there than keep going in circles here. As far as I’m concerned that’s a good approach, this isn’t (well, wasn’t, as of the aforementioned commit id), hence the approach nack here, vs the ack there.

ajtowns referenced this in commit e6f76cbabb on Sep 2, 2021

ryanofsky referenced this in commit 313976dfa2 on Sep 15, 2021

ryanofsky referenced this in commit 54cab91151 on Sep 27, 2021

ryanofsky referenced this in commit 6e613a0a30 on Sep 27, 2021

ryanofsky referenced this in commit 7ce0687ef1 on Oct 16, 2021

ryanofsky referenced this in commit b8c069b7a9 on Oct 25, 2021

ryanofsky referenced this in commit 5c428ceef6 on Oct 25, 2021

fanquake referenced this in commit 3fc3641043 on Nov 1, 2021

ryanofsky force-pushed on Nov 1, 2021

ryanofsky commented at 7:27 pm on November 1, 2021: contributor

Rebased aa3dec078fc464d7ea2b46088c789bfd4a7e2637 -> 9349a02cc41ea537873dd20636aff51e23870cbe (pr/argcheck.24 -> pr/argcheck.25, compare) due to conflict with #22217, on top of new base PR #22766 pr/argscripts.1 Rebased 9349a02cc41ea537873dd20636aff51e23870cbe -> c7ba71fb9165b1aae22d4b55a7f804223d5be3a0 (pr/argcheck.25 -> pr/argcheck.26, compare) on top #22766 pr/argscripts.4 Rebased c7ba71fb9165b1aae22d4b55a7f804223d5be3a0 -> 3345e119b07e6052ea0a6a0cfcc0186e6ddd3a06 (pr/argcheck.26 -> pr/argcheck.27, compare) after #22766 merge Updated 3345e119b07e6052ea0a6a0cfcc0186e6ddd3a06 -> 26f8e60dc6b6bb1d123822e336a93352e38fe148 (pr/argcheck.27 -> pr/argcheck.28, compare) with some minor commit and code cleanups, no changes in behavior. Rebased 26f8e60dc6b6bb1d123822e336a93352e38fe148 -> 770b99ea13d6800534f2aa8b6e65152a92272a94 (pr/argcheck.28 -> pr/argcheck.29, compare) due to various conflicts. Also improved commit messages and added a lot of documentation. Updated 770b99ea13d6800534f2aa8b6e65152a92272a94 -> e6565763879e92d3c17dca296966fbccf6991a83 (pr/argcheck.29 -> pr/argcheck.30, compare) changing TypedArg declaration to avoid signed/unsigned int conversion

ryanofsky force-pushed on Dec 30, 2021

DrahtBot added the label Needs rebase on Feb 9, 2022

uvhw referenced this in commit 47d44ccc3e on Feb 14, 2022

Stackout referenced this in commit caa30cc07d on Feb 17, 2022

Stackout referenced this in commit c8377531b0 on Feb 17, 2022

Stackout referenced this in commit b880790944 on Feb 17, 2022

PastaPastaPasta referenced this in commit e79d73f0fc on Jun 7, 2022

PastaPastaPasta referenced this in commit 78ac0e9361 on Jun 7, 2022

PastaPastaPasta referenced this in commit d1187d70d1 on Jun 10, 2022

PastaPastaPasta referenced this in commit 67f95cb60b on Jun 14, 2022

ryanofsky force-pushed on Sep 22, 2022

DrahtBot removed the label Needs rebase on Sep 22, 2022

ryanofsky force-pushed on Sep 24, 2022

DrahtBot added the label Needs rebase on Nov 15, 2022

ryanofsky force-pushed on Nov 17, 2022

DrahtBot removed the label Needs rebase on Nov 17, 2022

knst referenced this in commit 814f197d85 on Apr 18, 2023

knst referenced this in commit 39082d2565 on Apr 19, 2023

DrahtBot added the label Needs rebase on Apr 21, 2023

knst referenced this in commit 5289219191 on Apr 24, 2023

UdjinM6 referenced this in commit bf9edc1f42 on Apr 25, 2023

ryanofsky force-pushed on May 2, 2023

DrahtBot removed the label Needs rebase on May 2, 2023

DrahtBot added the label Needs rebase on May 9, 2023

achow101 commented at 4:42 pm on September 23, 2023: member

Are you still working on this? This was converted to draft 2 years ago, is it necessary to keep it as one?

Add ArgsManager flags to parse and validate settings on startup

This commit does not change current application behavior. It adds new
ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and ALLOW_LIST flags which are available
to be used by new settings, without affecting parsing of any existing settings.

The new flags validate settings earlier on startup and provide more detailed
error messages to users.

The new flags also provide stricter error checking. For example, a double
negated option like -nosetting=0 is treated like an error instead of true, and
an unrecognized bool value like -setting=true is treated like an error instead
of false. The new flags also trigger errors if a non-list setting is specified
multiple times in the same section of a configuration file, instead of silently
ignoring later values.

The new flags also provide type information that allows ArgsManager
GetSettings() and GetSettingsList() methods to return typed integer and boolean
values instead of unparsed strings.

The changes have no effect on current application behavior because the new
flags are only used in unit tests. The existing ALLOW_ANY checks in the
CheckValueTest unit test confirm that no current validation or parsing behavior
is changing.

a256f42f79

Update ArgManager GetArg helper methods to work better with ALLOW flags

Update GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods to work
conveniently with ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags. This commit
does not change application behavior in any way because these flags are new
and currently only used in unit tests.

The GetArg methods are convenience wrappers around the GetSetting method. The
GetSetting method returns the originally parsed settings values in their
declared bool/int/string types, while the GetArg wrappers provide extra
type-coercion and default-value fallback features as additional conveniences
for callers.

This commit makes two changes to GetArg, GetArgs, GetBoolArg, and GetIntArg
helper methods when BOOL/INT/STRING flags are used:

1. GetArg methods will now raise errors if they are called with inconsistent
   flags. For example, GetArgs will raise a logic_error if it is called on a
   non-LIST setting, GetIntArg will raise a logic_error if it is called
   on a non-INT setting.

2. GetArg methods will now avoid various type coersion footguns when they are
   called on new BOOL/INT/STRING settings. Existing ALLOW_ANY settings are
   unaffected. For example, negated settings will return "" empty strings
   instead of "0" strings (in the past the "0" strings caused strangeness like
   "-nowallet" options creating wallet files named "0"). The new behaviors are
   fully specified and checked by the `CheckValueTest` unit test.

The ergonomics of the GetArg helper methods are subjective and the behaviors
they implement can be nitpicked and debated endlessly. But behavior of these
helper methods does not dictate application behavior, and they be bypassed by
calling GetSetting and GetSettingList methods instead. If it's necessary,
behavior of these helper methods can also be changed again in the future.

The changes have no effect on current application behavior because the new
flags are only used in unit tests. The `setting_args` unit test and ALLOW_ANY
checks in the `CheckValueTest` unit test are unchanged and confirm that
`GetArg` methods behave exactly the same (returning the same values and
throwing the same exceptions) for ALLOW_ANY flags before and after this change.

98d8b63562

ryanofsky force-pushed on Sep 25, 2023

DrahtBot removed the label Needs rebase on Sep 25, 2023

DrahtBot added the label CI failed on Jan 24, 2024

DrahtBot commented at 1:55 am on April 23, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

refactor: Implement missing error checking for ArgsManager flags #16545

Reviews

Conflicts