Refactor: Add Flags enum to ArgsManager class

[hebasto]

This PR adds the Flags enum to the ArgsManager class. Also the m_flags member is added to the Arg struct. Flags denote an allowed type of an arg value and special hints.

This PR is only a refactoring and does not change behavior.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16527 (B: Get rid of Params().RequireStandard() by jtimon)
• #16526 (A: Refactor: Chainparams: readability by jtimon)
• #16524 (Wallet: Disable -fallbackfee by default by jtimon)
• #16411 (Signet support by kallewoof)
• #16115 (On bitcoind startup, write config args to debug.log by LarryRuane)
• #16060 (Bury bip9 deployments by jnewbery)
• #15934 (Separate settings merging from parsing by ryanofsky)
• #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
• #15421 (torcontrol: Launch a private Tor instance when not already running by luke-jr)
• #15367 (feature: Added ability for users to add a startup command by benthecarman)
• #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
• #13716 (bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords by kallewoof)
• #13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)
• #12557 ([WIP] 64 bit iOS device support by Sjors)
• #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
• #8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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.

[ryanofsky]

The problem described in the PR is real but I don't think the change implemented is the simplest or best fix. Having a new whitelist of options located in a separate part of the code from where the options are used and defined adds complexity, inconsistency, and action at distance to already very confusing code.

If someone specifies -nopid, it is wrong to create a pid file called 0, but a simpler and more useful fix would be to be not write a pid file instead of adding a new logic in a different part of the code to print a "negating not allowed" error.

But the -pid argument case actually seems atypical here. More cases where negated arguments are handled incorrectly (like -incrementalrelayfee) seem like simple misuses of the IsArgSet function, which is misnamed and doesn't do what people think it does. I'd replace the IsArgSet function with separate ArgHasValue and ArgIsSpecified functions and use ArgHasValue instead of ArgIsSpecified everywhere or almost everywhere the code currently using ArgIsSet.

[hebasto]

m_allowed_negated_args is used in the same manner as m_network_only_args (see #11862) and it seems consistent, IMO.

[promag]

adds complexity, inconsistency, and action at distance to already very confusing code

Agree with @ryanofsky. I'd rather see an approach like #16416 (comment)

Beside the fact that m_network_only_args is very small, I think it could be improved like my comment above - move that flag to the Arg structure.

Currently ArgsManager::Arg has:

        bool m_debug_only;

And I think it could be changed to something like:

   enum Flags {
      NONE = 0x00,
      DEBUG_ONLY = 0x01,
      NETWORK_ONLY = 0x2,
      ALLOW_NEGATED = 0x04,      
   };

   Flags m_flags;

Or just add more members.

[hebasto]

@ryanofsky Thank you for your review.

If someone specifies -nopid, it is wrong to create a pid file called 0, but a simpler and more useful fix would be to be not write a pid file instead of adding a new logic in a different part of the code to print a "negating not allowed" error.
But the -pid argument case actually seems atypical here.

Agree that -nopid should have a special behavior. And it deserves a separate PR.

More cases where negated arguments are handled incorrectly (like -incrementalrelayfee) seem like simple misuses of the IsArgSet function, which is misnamed and doesn't do what people think it does. I'd replace the IsArgSet function with separate ArgHasValue and ArgIsSpecified functions and use ArgHasValue instead of ArgIsSpecified everywhere or almost everywhere the code currently using ArgIsSet.

I don't think this approach will compeltely fix the problem described in this PR.

[hebasto]

@promag

And I think it could be changed to something like:

   enum Flags {
      NONE = 0x00,
      DEBUG_ONLY = 0x01,
      NETWORK_ONLY = 0x2,
      ALLOW_NEGATED = 0x04,      
   };

   Flags m_flags;

Done.

[hebasto]

Commit 50a7f97 needs rebase on top of #16366. Going to make it tonight.

[ryanofsky]

Thanks for the update! I still see several problems with this approach, but I think they can be fixed and I have specific suggestions below that I think will put this PR in a good state and make it easier to review. Here are the problems that I see presently (90dfd23):

• Interaction between boolean option detection via help string and ALLOW_NEGATED errors via flags is confusing and I think will lead to bugs in the future.
• PR has no release notes, no clear description of changes in behavior understandable to a normal user.
• PR mixes refactoring changes with behavioral changes, and makes behavior changes in commits that affects swaths of options rather than individual options, so it is difficult as a reviewer to determine how behavior is changing and verify the changes are good without searching and looking at every option.

I think all of these problems are fixable. Here are my suggestions:

1. Drop the detection of boolean arguments through help strings and drop the ALLOW_NEGATED flag. The only flags I would suggest keeping here are:

ALLOW_BOOL = 0x01,
ALLOW_INT = 0x02,
ALLOW_STRING = 0x4,
ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
DEBUG_ONLY = 0x100,
NETWORK_ONLY = 0x200,

2. Do not change any behavior in this PR. Set every existing option to ALLOW_ANY by default, with | DEBUG_ONLY and | NETWORK_ONLY flags added as necessary to preserve behavior.

3. Raise an error during startup if a -noXXX option is given and the ALLOW_BOOL flag is not present. This should have no effect on behavior of existing options because ALLOW_ANY should be set as described above.

4. Optionally, if you feel like implementing extra error checking that could be useful in the future, check for properly formatted ints and bools and nonempty strings based on the int/bool/string flags. Again these checks should not change behavior of existing options because ALLOW_ANY should always be set here.

5. In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

6. Make separate PRs for help text cleanups, any other unrelated cleanups.

Overall, I think this is moving in a good direction and adding the flags really helps. But the more we can move in the direction of simple bool/int/string option types, and have negation taken care of on the front end, rather than leaking into entire codebase, the better state this code will be in, and less weird behavior we will have in the future.

[hebasto]

@ryanofsky Thank you for your review. I really appreciate it.

5. In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

We have over two hundred target lines of code. Proposed approach does not allow to apply a scripted-diff, unfortunately. Thoughts?

[hebasto]

@ryanofsky

Drop the detection of boolean arguments through help strings ...

Agree. Going to implement it.

... and drop the ALLOW_NEGATED flag.

I'd prefer to use it. It complements the IsArgNegated() function. Also, consider the following two options:

• -nowallet has a special meaning, -wallet=0 points to a wallet named 0 and -nowallet=0 is an error; possible flags are ALLOW_BOOL | ALLOW_STRING
• -noprune equals to -prune=0; -noprune=0 is not an error; possible flags are ALLOW_BOOL | ALLOW_INT

Using the ALLOW_NEGATED flag makes things simpler, IMO:

• -wallet has flags ALLOW_STRING | ALLOW_NEGATED
• -prune has flags ALLOW_INT | ALLOW_NEGATED

[ryanofsky]

5. In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

We have over two hundred target lines of code. Proposed approach does not allow to apply a scripted-diff, unfortunately. Thoughts?

That's the whole point! It's good to use scripted diffs in refactoring commits, but there should be no scripted diff for the behavior change. The code changes in the behavior change PR should just look like:

-gArgs.AddArg("-changetype=<n>", ALLOW_ANY, ...);
+gArgs.AddArg("-changetype=<n>", ALLOW_STRING, ...);

-gArgs.AddArg("-dbcache=<n>", ALLOW_ANY, ...);
+gArgs.AddArg("-dbcache=<n>", ALLOW_INT, ...);

So it is easy for reviewers to verify that new errors triggered on options that currently allow negation are appropriate and appropriately documented in release notes.

There should not be 200 lines targeted. Every existing boolean option, every existing list option, most string options, and many int options that accept 0 should continue to allow negation.

... and drop the ALLOW_NEGATED flag.

I'd prefer to use it. It complements the IsArgNegated() function.

Just like IsArgSet which is misused many places, misunderstood and should be eliminated.
IsArgNegated is an awkward API that should go away, and ALLOW_NEGATED should never be introduced.

If you look at google flags api, qsettings, python argparse, the standard everywhere is bool/number/string representations, not "is negated" queries for code reading the values. My refactoring PR #15934 is a major step in this direction internally replacing our vector representation of negated values with plain, typed false values.

Nothing I'm suggesting here involves changing or limiting behavior in any way. I'm just suggesting to use normal, sane terminology, so we have a more usable api and so the IsArgSet IsArgNegated bugs you are fixing now don't get reintroduced in the future.

Using the ALLOW_NEGATED flag makes things simpler, IMO:

• -wallet has flags ALLOW_STRING | ALLOW_NEGATED
• -prune has flags ALLOW_INT | ALLOW_NEGATED

That's exactly what I'm suggesting, except spelling ALLOW_NEGATED as ALLOW_BOOL.

I didn't fully describe in my previous comment how the suggested flags should behave, so I'll do that now to avoid unnecessary confusion. I don't think the extra error checks I'm describing here need to be implemented now. It'd be fine to implement them in future PRs that actually start adding ALLOW_BOOL / ALLOW_INT / ALLOW_STRING options instead of ALLOW_ANY for everything. Anyway, here's how the flags are meant to behave:

• -noOPTION and -noOPTION=1 negation syntax should be accepted when ALLOW_BOOL is present.
• -OPTION and -OPTION= empty string syntax be accepted when ALLOW_BOOL is present.
• -OPTION=0 and -OPTION=1 syntax should be accepted when ALLOW_BOOL is present.
• -OPTION=123 where 123 is an integer should be accepted when ALLOW_INT is present.
• -OPTION=abc where abc is a nonempty string should be accepted when ALLOW_STRING is present.
• Anything not accepted above should trigger an error.

Except for one detail, adding these checks should have no effect when ALLOW_ANY is specified, so even if these checks are added now, this can remain a pure refactoring PR and not change behavior.

The detail is that we currently accept -noOPTION=abc syntax for strange values of abc, sometimes interpreting abc as false and showing a warning, sometimes interpreting it as true and not warning. This behavior should be left untouched here, and cleaned up in a separate PR (making any abc that is not the empty string or 1 an error).

[promag]

Really nice @ryanofsky. Maybe it could also have a MULTIPLE flag?

[ryanofsky]

Maybe it could also have a MULTIPLE flag?

Yes I think that'd be a good flag, and I also like your idea adding support to declare default values. Best if this PR scope doesn't expand too much, but going forward, the more things about options that can be declared up front, the less complicated code reading options has to be and the less chance for inconsistencies and bugs.

[promag]

Best if this PR scope doesn't expand too much

👍

Another flag (otherwise I may forget it) could be SENSITIVE so that its value wouldn't be dumped.

[hebasto]

@ryanofsky

6. Make separate PRs for help text cleanups, any other unrelated cleanups.

The "refactoring: Use direct list initialization" commit (b89eb79) has split out to #16469.

[ryanofsky]

@ryanofsky

6. Make separate PRs for help text cleanups, any other unrelated cleanups.

The "refactoring: Use direct list initialization" commit (b89eb79) has split out to #16469.

Sorry, I'd suggest closing #16469. The quote above is a little ambiguous, but I was trying to suggest splitting behavior changes into a separate PRs from the requested flags refactor, not creating multiple refactoring PRs.

It's good to make behavior changes in small independent PRs so they don't get missed by reviewers and so there aren't multiple confusing discussion about different behaviors. It generally takes a lot less mental effort to review a pure refactoring PR than it does to review a refactoring PR mixed with behavior changes, so there shouldn't be any problem with including a small list initialization refactor in the same PR that does the requested flag refactor.

[maflcko]

Agree with @ryanofsky