refactor: Implement missing error checking for ArgsManager flags #16545

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 easier to start applying type checking flags to new options than existing options. But for examples of how type checking flags can make sense applied to existing options, see the new example_options unit test added in this PR.

Followups:

• Support for flag combinations is possible and is added in commit c919f51c044f0e80ecd301f9c9d396ddff2331ba (branch)
• ALLOW_LIST flags are added and enforced in #17580
• Bad IsArgSet usages with ALLOW_LIST are removed in #30529 and prevented in #17783
• Confusing and ignored multiple assignments in config files are disallowed in #17493
• Confusing reverse-precedence settings merging code is removed in #17581
• Type flags added in this PR implement runtime part of a change to add more compile-time checking in #22978

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/16545.

Reviews

See the guideline for information on the review process.

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

Conflicts

No conflicts as of last run.

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

• no need for key_name local variables
• diff gets much smaller

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.

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.

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.

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.

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:

1. 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({}));

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

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.

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

Thanks for re-ack!

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

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

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.

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…

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

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.

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

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

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.

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.

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.

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.

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.

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

re: #16545 (comment)

Are you still working on this, or do you want someone to pick it up?

I would like code review, and I probably need to respond to some old comments and questions. There are two parts to this PR:

• The first part of this PR, implemented in the first commit, uses the ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags to validate command line and config file settings, and save them into UniValue values as typed values, rather than strings.

• The second part of this PR, implemented in the second commit, uses the ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and ALLOW_LIST flags to provide extra safety to the GetArg, GetArgs, GetBoolArg, and GetIntArg accessors. For example it prevents GetIntArg accessor from being called on non-int settings, and disables confusing legacy coercion behavior like making GetBoolArg return false for -setting=0 and true for -setting=1 but false for -setting=true and true for -setting=.

I think the first commit is less controversial, but the second commit has been nitpicked because it is a developer API and developers have different preferences and concerns.

Unfortunately, I don’t think it is safe to merge the first commit without the second commit, because the second commit disables unsafe coercions in the Get*Arg functions.

But fortunately I do not think we need to nitpick and argue about behavior the Get*Arg functions because if any developer does not like the behavior of the Get*Arg functions they can call the GetSetting function instead which gives access to the underlying, validated UniValue setting and allows any developers to implement any behavior they would like to implement to parse their settings using the UniValue API. Also, because this second commit only affects an internal API, it is possible to change its behavior in the future, if necessary, without affecting users.

I have done substantial work on top of this PR (in #16545 #17580 #17493 #17581 4719738f602a681eb0d1633fbb1651f42cc93129 7f7d82b521b7d78ea9eccab18f068e2881eefafc 6865a198f5db30bd494b3a2540f47ee728963908 51ca84ecfcad3b8b2a85d75c8c1da61d97c2ca9b and 012a320038158a532bd3662c99cd8211941659d2) porting real settings to use the API defined here, and I shaped the API around this work, and I believe if this PR could make its way beyond the nitpicking trap it is in, it would substantially improve user and developer experience and safety. Or, if I am wrong, it would fail to live up to this promise and will need subsequent improvements. Either way I think this PR is a positive step towards improving the current situation where settings are parsed in unsafe and surprising ways.

But fortunately I do not think we need to nitpick and argue about behavior the Get*Arg functions because if any developer does not like the behavior of the Get*Arg functions they can call the GetSetting function instead which gives access to the underlying, validated UniValue setting and allows any developers to implement any behavior they would like to implement to parse their settings using the UniValue API.

I looked at the feedback so far, and I wonder if it could make sense to simplify the changes here for now to only allow an enum class (and not a flags field). The reason is that the controversy seems to stem from where flags are combined. Eg.

0    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=0", Expect{"0"}.String("0").DefaultBool());
1    CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool());

(falling back to the default seems controversial).

This way, flags can be implemented later (or implemented via GetSetting), if needed, but the simple stuff (like just a ALLOW_BOOL) should be easily usable in new options?

Concept ACK 9196cfe26da4784029500da482c6f7d55fca5ac2

Concept

In #16545 (review) you say:

Also this case really can and should be a compile error, not a runtime error.

That’s along the lines of what I was going to bring up before reading the comments too. Hopefully this PR will take us further in that direction as you say.

Commit messages

Both commits mention not changing application behavior 2 times each, seems a bit much.

Commit message 9196cfe26da4784029500da482c6f7d55fca5ac2

and they be bypassed -> and they can be bypassed

Behavior changes once types are applied

This thread gives me some concern: #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).

Implementing a BOOL|STRING example might help assuage some fears.

Updated 9196cfe26da4784029500da482c6f7d55fca5ac2 -> 29d932b18559a360f7fade67ad8172ff04f80345 (pr/argcheck.35 -> pr/argcheck.36, compare) adding an initial documentation commit, and a final test commit showing more realistic examples of how the flags are intended to be used. Also implemented all the suggestions from hodlinator’s very helpful review and made small tweaks based on the other changes.

re: #16545 (comment)

I looked at the feedback so far, and I wonder if it could make sense to simplify the changes here for now to only allow an enum class (and not a flags field)

It’s an interesting idea, but I wonder if there’s any particular problem you think it would solve?

I don’t see why it should be a problem to combine the ALLOW flags in general because the flags control which syntaxes are allowed, so combining the flags just allows more syntaxes.

It is true that in most cases you don’t want to combine STRING, INT, and BOOL flags. The one case where it’s useful to combine BOOL with INT or STRING, is when you have some imperative setting that causes a new action to happen, and can take an optional int or string value. I think of settings like -ipcbind=<address> and -ipcconnect=<address> where you want to be able to accept an address to connect or bind to, so you want ALLOW_STRING. But you also want to the address to be optional and for -ipcbind and -ipcconnect to work so you need to add ALLOW_BOOL.

I tried to document this better and also added a new example with -ipcbind in the last commit.

re: #16545#pullrequestreview-2194180912

Also this case really can and should be a compile error, not a runtime error.

That’s along the lines of what I was going to bring up before reading the comments too. Hopefully this PR will take us further in that direction as you say.

Yes, basically my idea for implementing this is in #22978, and there are other ideas listed there as well. This PR is parsing the settings into typed objects, but the types are determined at runtime because the settings are only defined at runtime when they are registered. If the types were available earlier before the settings are registered, it could be a compile error instead of a runtime error to call the wrong GetArg function.

Both commits […]

Thanks for the helpful suggestion and correction. Issues should be fixed now.

Implementing a BOOL|STRING example might help assuage some fears.

This is a good idea and it prompted me to add a new commit with more realistic examples of how the API is intended to be used. So hopefully that can help.

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28126603473

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Reviewed 29d932b18559a360f7fade67ad8172ff04f80345

Thanks for refining the commit messages and breaking out one commit out of another + adding the new test commit!

See you fixed a missing ALLOW_BOOL in GetBoolArg().

From my prior review:

This thread gives me some concern: #16545 (comment)

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 see your possible counterpoint - when we have BOOL|INT args, the bool component should only be set using simply “-arg” or “-noarg”, while “-arg=1” or “-arg=0” means setting the int component. It would be more consistent to make BOOL-only args disallow “-arg=1” or “-arg=0” but I understand if you don’t want to take that fight now.

Updated 29d932b18559a360f7fade67ad8172ff04f80345 -> 7c5cf89ca9bcb62e11cce1c9a711ebd8450d2be0 (pr/argcheck.36 -> pr/argcheck.37, compare) with review suggestions and other minor corrections like spelling fixes.

Thanks again for the detailed review! I think I implemented all the suggestions except one.

re: #16545#pullrequestreview-2208698806

See you fixed a missing ALLOW_BOOL in GetBoolArg().

Yes, the previous version did not require the ALLOW_BOOL flag to be set to call GetBoolArg(). This behavior was actually intended when the PR was written, because it was before GetArg functions returning std::optional were introduced in fc02f77ca604f0221171bfde3059b34f5d0fb1cd from #25290, so GetBoolArg() was useful as a way to see if an argument was set and nonempty, and it was always allowed to be called regardless of flags. But now that std::optional is available, there is no need for GetBoolArg() to be special.

It would be more consistent to make BOOL-only args disallow “-arg=1” or “-arg=0” but I understand if you don’t want to take that fight now.

Ideally, I would like to go the opposite direction and allow -arg=0/1/false/true/yes/no/on/off boolean syntax like git but the problem is InterpretBool() interprets all of these values except 1 as false right now, and probably there are existing configurations inadvertently relying on this behavior, so this change would have to be rolled out thoughtfully. One way to do it would be to first make it an error to use “true” “yes” or “on” to set false values, and make it a warning to specify other, unrecognized boolean values. Then after some time we could start interpreting and “true” “yes” or “on” as true and “false” “no” “off” as false and make it an error instead of a warning to specify unrecognized values. These changes could happen independently of this PR if we wanted to pursue them. But this PR just wants to interpret boolean values safely without changing the way they are normally specified.

Concept ACK 1e37bcf9fc11562baaedea24685c31f60ef2de31

The current state of things with no/ad-hoc validation of command line args since #22766 in Nov 2021 is not a good place to be. There can be a tendency to neglect stringent argument parsing on larger projects as long as the happy path of correctly specified args still works.

Other more embryonic and radical initiatives exist to make args more strictly typed at compile time (#22978, and @ajtowns https://github.com/ajtowns/bitcoin/pull/8). I think the incremental approach of this PR + gradually activating it for existing args will achieve a better baseline to approach those future initiatives from.

Approach concerns

nit: d03b7e94cd4f89b46f658bb3f2afbb24b26e84fa - Title of the commit message could be “Add ArgsManager flags to parse and validate settings on startup” -> “Implement ArgsManager flags to parse and validate settings on startup” as it was the prior commit that uncommented the flags themselves.

Beyond my inline comment regarding the ALLOW_INT | ALLOW_BOOL example, I think the next steps for me to reach an Approach A-C-K would be to try converting some existing complex args to be typed.

Why this PR hasn’t seen more activity lately

maflcko:

I know that you [(ryanofsky)] disagree and want to introduce all features in one go all at once, but the fact that this pull request is sitting for more than half a decade without a single full review ACK could be a mild hint that the current state may not be the one that reviewers want to review? @maflcko I am grateful you pointed me to this PR based on a comment I made in another PR. Had I started on Bitcoin Core earlier, I would have pushed for some version of this sooner. I’ve been doing passes on the PR lately with ryanofsky in the hope of getting traction on it from more contributors.

I think ryanofsky is correct in that a big factor in lacking reviews was the somewhat heated discussions earlier on.

ryanofsky has been responsive throughout the PR lifetime and provided example commits to further help reviewers.

Difficulty of review

maflcko:

I agree, but I don’t think it is incompatible with my suggestion. (In fact, what you say in the quote is what I was trying to say myself). It seems possible to just implement bool error checking (and only that, or any other single feature), and make sure it is coherent and will be used, and then roll out the feature to one bool-only setting at a time, changing the behavior only once for this setting.

maflcko:

Personally, I just find it a bit hard to read the docs in this change (even if they are great and self-consistent) and then say “Yes, …

I second ryanofsky’s suggestion to look at the recently added ExampleOptions test in 16fe0c57637dbc83c569a407b1f36c30a24081f5 do help make things more real. Maybe focus on reviewing those before wading through the docs? But I agree it would make the PR easier to review if @ryanofsky could find ways to compress the comments.

It is useful to evaluate a tentative complete version of the API. If the tactic of breaking the PR down into smaller steps is still a requirement for maflcko then maybe we should take a stab at it. If ryanofsky doesn’t feel like doing that then maybe I can volunteer (although he’d probably be 20x quicker).

Wacky idea

The number of args are finite (~270). To inspire confidence in this and later PRs, maybe one could build an --extended functional test harness that runs through all ways they might be used, in all expected valid variants (-norescan, -rescan, -rescan=500000, -rescan=0, -rescan=1), adding debug output where there isn’t some already in order to perform primitive log-based validation that the arg was applied in the expected way.

If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() and the second function reading args and applying them to each component’s options-struct, then it could even be run as unit tests, and might also take us one step closer to compile-time type safety for args.

Updated 1e37bcf9fc11562baaedea24685c31f60ef2de31 -> 41bdf3d025f900a59ec14d5b497a31a2d84eea52 (pr/argcheck.39 -> pr/argcheck.40, compare) dropping support for flag combinations and fixing a commit message as suggested.

Support for flag combinations is added back in commit df5c3e123227c1cd01a02b970f63c3682a1522d4 (branch) which could be a followup PR.

Thanks maflcko and hodlinator for the very helpful feedback. I dropped support for flag combinations which should make this PR easier to merge and start using, if we want to to do that. In parallel, though I’m also working on a scripted-diff change based on #22978 (comment) that will associate C++ types with all existing ArgsManager options while being fully backwards compatible, to make the new behaviors implemented here easier to see and enable in specific cases, without having to think abstractly about flags. So if we can’t make progress here, we could have a another way of adding type checking soon that doesn’t require using the flags directly.

re: #16545#pullrequestreview-2240110993

Thank you very much for that summary and perspective!

nit: d03b7e9 - Title of the commit message could be […]

Thanks! Updated

Wacky idea

The number of args are finite (~270). To inspire confidence in this and later PRs, maybe one could build an --extended functional test harness that runs through all ways they might be used, in all expected valid variants (-norescan, -rescan, -rescan=500000, -rescan=0, -rescan=1), adding debug output where there isn’t some already in order to perform primitive log-based validation that the arg was applied in the expected way.

This seems pretty feasible to implement, though I imagine a naive implementation could be pretty slow since there are a lot of ways to specify options. Seems ok for an extended test though, and I do think it’s worth thinking about how to test at least some options comprehensively even if not every option.

If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() and the second function reading args and applying them to each component’s options-struct, then it could even be run as unit tests, and might also take us one step closer to compile-time type safety for args.

Would be curious about this idea if you can elaborate. I guess I’m not sure how you would want to check the result of the second function.

git range-diff master 1e37bcf 41bdf3d

Adds code disallowing combining ALLOW_BOOL with ALLOW_INT and ALLOW_STRING. Removes now invalid tests and adds 2 BOOST_CHECK_THROW()s.

Curious to hear maflcko’s thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.

Making the applying of arguments testable

hodlinator:

If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() and the second function reading args and applying them to each component’s options-struct, then it could even be run as unit tests, and might also take us one step closer to compile-time type safety for args.

ryanofsky:

Would be curious about this idea if you can elaborate. I guess I’m not sure how you would want to check the result of the second function.

Rough existing example:

 0struct BlockManagerOpts {
 1    const CChainParams& chainparams;
 2    uint64_t prune_target{0};
 3    bool fast_prune{false};
 4    const fs::path blocks_dir;
 5    Notifications& notifications;
 6};
 7
 8class BlockManager
 9{
10    ...
11    using Options = kernel::BlockManagerOpts;
12    ...
13};
14
15util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
16{
17    // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
18    int64_t nPruneArg{args.GetIntArg("-prune", opts.prune_target)};
19    if (nPruneArg < 0) {
20        return util::Error{_("Prune cannot be configured with a negative value.")};
21    }
22    uint64_t nPruneTarget{uint64_t(nPruneArg) * 1024 * 1024};
23    if (nPruneArg == 1) { // manual pruning: -prune=1
24        nPruneTarget = BlockManager::PRUNE_TARGET_MANUAL;
25    } else if (nPruneTarget) {
26        if (nPruneTarget < MIN_DISK_SPACE_FOR_BLOCK_FILES) {
27            return util::Error{strprintf(_("Prune configured below the minimum of %d MiB.  Please use a higher number."), MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024)};
28        }
29    }
30    opts.prune_target = nPruneTarget;
31
32    if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
33
34    return {};
35}

Only 2/5 of the fields are actually set by ApplyArgsManOptions, and prune_target is even being read from first. Ideally the function prototype would be closer to:

0util::Result<BlockManager::Options> DeriveBlockManagerOptsFromArgs(const ArgsManager& args, const CChainParams& chainparams, int default_prune_target, const fs::path& blocks_dir, Notifications& notifications);

and should also have a separate:

0void RegisterBlockManagerOptsArgs(const ArgsManager& args)
1{
2    args.AddArg("-fastprune", "Use smaller block files and lower minimum prune height for testing purposes", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
3    args.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
4            "Warning: Reverting this setting requires re-downloading the entire blockchain. "
5            "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
6}

Then one could create tests such as:

 0BOOST_AUTO_TEST_CASE(util_datadir)
 1{
 2	ArgsManager args;
 3	RegisterBlockManagerOptsArgs(args);
 4
 5	{
 6		args.ForceSetArg("-fastprune", false);
 7		BlockManagerOpts& opts = DeriveBlockManagerOptsFromArgs(args, ...).value();
 8		BOOST_CHECK_EQUAL(opts.prune_target, 1);
 9		BOOST_CHECK_EQUAL(opts.fastprune, false);
10		...
11	}
12
13	{
14		args.ForceSetArg("-fastprune", true);
15		BlockManagerOpts& opts = DeriveBlockManagerOptsFromArgs(args, ...).value();
16		BOOST_CHECK_EQUAL(opts.prune_target, 0);
17		BOOST_CHECK_EQUAL(opts.fastprune, true);
18		...
19	}
20
21	...
22}