Use ArgsManager::ALLOW_STRING flag explicitly in simple cases #16476

1. 
   hebasto commented at 8:27 pm on July 27, 2019: member

   This PR replaces permissive ArgsManager::ALLOW_ANY flag with more restrictive ArgsManager::ALLOW_STRING flag for string type configuration options in simple cases.

   With this PR:

   0$ src/bitcoind -nodatadir
   1Error: Error parsing command line arguments: Negating of -datadir is meaningless and therefore forbidden
   2
   3$ src/bitcoind -noblocksdir
   4Error: Error parsing command line arguments: Negating of -blocksdir is meaningless and therefore forbidden
   

   Special cases, like -nowallet, are out of this PR’s scope.

   NOTE for reviewers. This PR is based on top of #16508 and #16097, so only the last four commits are to be reviewed here.

2. refactoring: Check IsArgKnown() early e0e18a1017
3. Refactor InterpretNegatedOption() function

   - added args parameter
   - renamed to InterpretOption()
   - removed code duplication

   e0d187dfeb
4. Add Flags enum to ArgsManager 265c1b58d8
5. scripted-diff: Use Flags enum in AddArg()

   -BEGIN VERIFY SCRIPT-
   sed -i 's/const bool debug_only,/unsigned int flags, &/' src/util/system.h src/util/system.cpp
   sed -i -E 's/(true|false), OptionsCategory::/ArgsManager::ALLOW_ANY, &/' $(git grep --files-with-matches 'AddArg(' src)
   -END VERIFY SCRIPT-

   1b4b9422ca
6. scripted-diff: Use ArgsManager::DEBUG_ONLY flag

   -BEGIN VERIFY SCRIPT-
   sed -i 's/unsigned int flags, const bool debug_only,/unsigned int flags,/' src/util/system.h src/util/system.cpp
   sed -i 's/ArgsManager::NONE, debug_only/flags, false/' src/util/system.cpp
   sed -i 's/arg.second.m_debug_only/(arg.second.m_flags \& ArgsManager::DEBUG_ONLY)/' src/util/system.cpp
   sed -i 's/ArgsManager::ALLOW_ANY, true, OptionsCategory::/ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::/' $(git grep --files-with-matches 'AddArg(' src)
   sed -i 's/ArgsManager::ALLOW_ANY, false, OptionsCategory::/ArgsManager::ALLOW_ANY, OptionsCategory::/' $(git grep --files-with-matches 'AddArg(' src)
   -END VERIFY SCRIPT-

   fb4b9f9e3b
7. Remove unused m_debug_only member from Arg struct 9a12733508
8. Use ArgsManager::NETWORK_ONLY flag dde80c272a
9. Replace IsArgKnown() with FlagsOfKnownArg() db08edb303
10. 
    hebasto commented at 8:29 pm on July 27, 2019: member

    ping @MarcoFalke @promag @ryanofsky
11. DrahtBot added the label GUI on Jul 27, 2019
12. DrahtBot added the label Tests on Jul 27, 2019
13. DrahtBot added the label Utils/log/libs on Jul 27, 2019
14. DrahtBot added the label Validation on Jul 27, 2019
15. DrahtBot added the label Wallet on Jul 27, 2019
16. 
    DrahtBot commented at 10:24 pm on July 27, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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)

    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.

17. 
    promag commented at 5:27 pm on July 28, 2019: member

    Concept ACK.
18. fanquake removed the label GUI on Jul 29, 2019
19. fanquake removed the label Validation on Jul 29, 2019
20. fanquake removed the label Wallet on Jul 29, 2019
21. fanquake removed the label Tests on Jul 29, 2019
22. 
    jonasschnelli commented at 8:42 am on July 29, 2019: contributor

    Concept ACK. Haven’t looked at the code though.
23. 
    practicalswift commented at 9:50 am on July 30, 2019: contributor

    Concept ACK
24. 
    ryanofsky commented at 6:41 pm on July 30, 2019: member

    I’d NACK this PR in it’s current form, but I think it could be fixed up without too much effort. Currently, commit “Use ArgsManager::ALLOW_STRING flag explicitly” (181f462dd983c0218618c1f64b65eefecb32ca4e) is changing too much behavior at the same time, and the current PR description doesn’t begin to describe all the changes, and there are no release notes. Regarding commit 181f462dd983c0218618c1f64b65eefecb32ca4e:

    • I think it’s good to trigger errors if someone specifies -noblocksdir or -nodatadir since these directories are mandatory and bitcoin can’t function without them. It looks like the commit is doing the right thing by forbidding these options.
    • It’s probably ok to trigger unconditional errors on -norpcport and -nowalletdir like this commit is doing, but I think more ideally conservative configurations like -noserver -norpcport or -nowallet -nowalletdir would continue to work instead of being forbidden now.
    • I think options like -noalertnotify, -noassumevalid, -noconf, -nominimumchainwork, -noaddnode, and others should be continue to allowed since they could be useful for command line overrides and because they have straightforward meanings. It looks like the PR is forbidding these options without a justification. If these flags aren’t currently being interpreted correctly, it would seem better to make simple bugfixes here than forbid options which should work.
    • The -noincludeconf case might be an exception, or there may be other options like it, but it’s explicitly handled in config parsing code to disable inclusion, but now the option itself is now forbidden, so this is a bug.

    Overall, I think commit 181f462dd983c0218618c1f64b65eefecb32ca4e changes too much and is too restrictive. I think some parts of it could be rolled back to limit scope of this PR and preserve more current behavior, while allowing more targeted improvements to be done in the future. Otherwise, I would just like to see it split up into smaller commits that:

    • Change individual options, or small groups of options that are similar to each other.
    • Document user-visible changes in release notes, so users can be forewarned about them. You can see instructions at https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes for adding a doc/release-notes-16476.md file, to document previously accepted options that are now errors and previously misinterpreted options that are now interpreted correctly.
25. Revamp option negating policy b70cc5d733
26. test: Make tests arg type specific e6f649cb2c
27. Use ArgsManager::ALLOW_BOOL flag explicitly 1bb9e33f58
28. Allow double negation for pure boolean args only 98ebe68704
29. test: Add tests for option negating policy ad3938065b
30. Use ArgsManager::ALLOW_STRING flag in utilities 95c452fc59
31. Use ArgsManager::ALLOW_STRING flag explicitly b886a80e66
32. test: Add tests for negated string args d8c154d134
33. doc: Add release notes for 16476 38eadd7577
34. hebasto force-pushed on Jul 31, 2019
35. 
    hebasto commented at 8:29 pm on July 31, 2019: member

    @ryanofsky

    I think some parts of it could be rolled back to limit scope of this PR and preserve more current behavior, while allowing more targeted improvements to be done in the future.

    Done. Only simple cases are in this PR. Other parts have been rolled back.

    I would just like to see it split up into smaller commits…

    Done. A big commit has been split in two smaller ones.

    Document user-visible changes in release notes, so users can be forewarned about them.

    Done.

    Also some tests have been added.

36. hebasto renamed this:
    Use ArgsManager::ALLOW_STRING flag explicitly
    Use ArgsManager::ALLOW_STRING flag explicitly in simple cases
    on Jul 31, 2019
37. 
    hebasto commented at 8:39 pm on July 31, 2019: member

    The PR title and description have been updated.
38. hebasto closed this on Aug 1, 2019

    ---

39. hebasto deleted the branch on Jun 13, 2020
40. DrahtBot locked this on Feb 15, 2022

Contributors
hebasto DrahtBot promag jonasschnelli practicalswift ryanofsky

Labels
Utils/log/libs