refactor: Clarify and disable unused ArgsManager flags #22766

ryanofsky · 2021-08-21T20:20:04Z

This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.

Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).

An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.

Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.

None of the changes affect behavior in any way.

DrahtBot · 2021-08-21T21:51:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22834 (net: respect -onlynet= when making outbound connections by vasild)

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.

…Arg to GetIntArg 93b9800 scripted-diff: Rename overloaded int GetArg to GetIntArg (Russell Yanofsky) Pull request description: This is meant to improve readability of code and remove guesswork needed to determine argument types and migrate to [typed arguments (#22978)](bitcoin/bitcoin#22978) by having distinctly named `GetArg` `GetArgs` `GetBoolArg` and `GetIntArg` methods. --- This commit was originally part of #22766 and had some review discussion there. But it was [wisely suggested](bitcoin/bitcoin#22766 (comment)) to be split off to make that PR smaller. ACKs for top commit: hebasto: ACK 93b9800. MarcoFalke: re-ACK 93b9800 📨 Tree-SHA512: e034bd938b2c8fbadd90bcd52213a61161965dfddf18c2cb0d68816ecf2438cde8afee6fb7e3418f5c6b35c208338da9deb99e4e418dbf68ca34552e0916a625

ryanofsky · 2021-09-27T13:39:33Z

Rebased 0768cdc -> 5c04ca8 (pr/argscripts.5 -> pr/argscripts.6, compare) due to silent conflict with #23025 in scripted diff commit

promag

Concept ACK, code review on 1st commit. I need to understand the big picture from #16545 in order to review the remaining changes here.

promag · 2021-10-16T11:33:06Z

src/util/system.cpp

-            m_settings.ro_config[section][key].push_back(value);
+            std::optional<util::SettingsValue> value =
+                InterpretValue(key, option.second, keyinfo.negated, *flags, error);
+            if (!value) return false;


4be2e10

nit, rename value to parsed, here change to !parsed.has_value(), and below parsed.value().

re: #22766 (comment)

nit, rename value to parsed, here change to !parsed.has_value(), and below parsed.value().

Why these requests? Usage of has_value seems less idiomatic and the name parsed seems less descriptive. It's always helpful when you make suggestions to say what thinking is behind them so different preferences can be reconciled.

Just a nit, feel free to ignore. Personally prefer has_value(), especially if above was auto value = InterpretValue(...).

Edit:has_value() is used in some places. Maybe there should be some guidelines in developer notes or something?

src/util/system.cpp

ryanofsky · 2021-10-16T14:37:42Z

Thanks for review! Just wanted to quickly say you do not need to care anything about #16545 for this PR to make sense. This PR is disabling flags ALLOW_INT/STRING/BOOL which are broken, by commenting them out. #16545 is reenabling these flags by uncommenting them again and implementing them.

Even if #16545 is rejected, these flags are misleading now and should be disabled. The only effect these flags have now is to disallow negation, so this PR is replacing them with a clearer DISALLOW_NEGATION flag. I expect this flag to be rarely used in the future, and probably to be removed later. Defining it here is just useful to make current code clear and avoid changing behavior in any way with this PR.

(EDIT: The reason I think DISALLOW_NEGATION will be removed in the future is described #22766 (review). But basically I think the error message DISALLOW_NEGATION shows is vague and a little confusing, and in general you will produce better error messages by checking for allowed values than checking for disallowed values.)

promag

Code review ACK 5c04ca8.

Thanks for the clarification. Indeed, the existing flags are misleading when in practice they are used to infer if negation is accepted. I got distracted by ALLOW_LIST, just realized it's only added in #16545.

promag · 2021-10-16T15:15:05Z

src/util/system.h

-        ALLOW_INT = 0x04,         //!< unimplemented, draft implementation in #16545
-        ALLOW_STRING = 0x08,      //!< unimplemented, draft implementation in #16545
-        ALLOW_LIST = 0x10,        //!< unimplemented, draft implementation in #16545
+        // ALLOW_BOOL = 0x02,     //!< unimplemented, draft implementation in #16545


5c04ca8

For the record, I don't mind these commented out instead of removed.

re: #22766 (comment)

For the record, I don't mind these commented out instead of removed.

The reason these are commented out instead of removed is that flag name ALLOW_ANY doesn't make sense without seeing other ALLOW_ flags. I could remove or rename ALLOW_ANY, but I don't want to, because it would blow up the size of this PR and make it conflict with everything, and because as long as some validation is added later, the name ALLOW_ANY should be fine later.

kiminuo

Concept ACK

kiminuo · 2021-10-16T15:13:00Z

src/util/system.cpp

 * IsArgNegated() method. One use case for this is to have a way to disable
 * options that are not normally boolean (e.g. using -nodebuglogfile to request
 * that debug log output is not sent to any file at all).
 */
-
-static util::SettingsValue InterpretOption(std::string& section, std::string& key, const std::string& value)
+struct KeyInfo { std::string section; bool negated{false}; };


nit: As there are many "keys" in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.

re: #22766 (comment)

nit: As there are many "keys" in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.

I'm happy to use a different name, but this was the name suggested by another reviewer so I kept it. If reviewers want to decide on something different, I'm happy with any of these, since this is just a private struct local to system.cpp

src/util/system.cpp

src/util/system.h

kiminuo · 2021-10-16T15:42:51Z

src/util/system.cpp

@@ -227,7 +227,7 @@ static std::optional<util::SettingsValue> InterpretValue(const std::string& key,
 {
    // Return negated settings as false values.
    if (negated) {
-        if (!(flags & ArgsManager::ALLOW_BOOL)) {
+        if (flags & ArgsManager::DISALLOW_NEGATION) {
            error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);


6e613a0

nit: It seems to me that this message may simply say that negation is disallowed/forbidden as I'm not sure that it's guarranteed that negation is "meaningless".

re: #22766 (comment)

nit: It seems to me that this message may simply say that negation is disallowed/forbidden as I'm not sure that it's guarranteed that negation is "meaningless".

I agree this error message is not clear, and this is only kept to keep current behavior.

In the future, I think it would be good to remove the disallow negation flag entirely and use allowed value checks instead of disallowed values checks as described #22766 (comment).

src/util/system.h

ryanofsky

Thanks for review! Updated just making suggested changes.

Updated 5c04ca8 -> b8fd5d5 (pr/argscripts.6 -> pr/argscripts.7, compare) with suggested changes

src/util/system.cpp

ryanofsky · 2021-10-16T15:11:31Z

src/util/system.cpp

-            m_settings.ro_config[section][key].push_back(value);
+            std::optional<util::SettingsValue> value =
+                InterpretValue(key, option.second, keyinfo.negated, *flags, error);
+            if (!value) return false;


re: #22766 (comment)

nit, rename value to parsed, here change to !parsed.has_value(), and below parsed.value().

Why these requests? Usage of has_value seems less idiomatic and the name parsed seems less descriptive. It's always helpful when you make suggestions to say what thinking is behind them so different preferences can be reconciled.

ryanofsky

re: #22766 (comment)

nit: As there are many "keys" in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.

EDIT: moved reply to #22766 (comment)

src/util/system.cpp

src/util/system.h

promag

Code review ACK b8fd5d5.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

…flag usage Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is bitcoin#16545). An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed. Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in two commits, with this commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.

This commit does not change behavior in any way. See previous commit for complete rationale, but these flags are being disabled because they aren't implemented and will otherwise break backwards compatibility when they are implemented. -BEGIN VERIFY SCRIPT- sed -i 's:\(ALLOW_.*\) \(//!< unimplemented\):// \1\2:' src/util/system.h sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)' | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g' git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g' -END VERIFY SCRIPT-

ryanofsky · 2021-10-25T15:48:48Z

Rebased b8fd5d5 -> c5d7e34 (pr/argscripts.7 -> pr/argscripts.8, compare) due to conflict with #23002

ajtowns · 2021-10-26T09:55:55Z

utACK c5d7e34

ryanofsky · 2021-10-29T17:08:18Z

@promag do you think you could reack? Only change since your previous review was rebasing to avoid a minor wallet-tool conflict, #22766 (comment)

promag

Code review ACK c5d7e34, which as the new argument -legacy.

… flags

DrahtBot added Consensus GUI Mining P2P Refactoring RPC/REST/ZMQ Utils/log/libs UTXO Db and Indexes Validation Wallet labels Aug 21, 2021

maflcko removed GUI Wallet UTXO Db and Indexes RPC/REST/ZMQ P2P Mining Validation labels Aug 22, 2021

DrahtBot removed the Needs rebase label Sep 15, 2021

This was referenced Sep 18, 2021

Make descriptor wallets by default #23002

Merged

Full CJDNS support #23077

Merged

ryanofsky force-pushed the pr/argscripts branch from 0768cdc to 5c04ca8 Compare September 27, 2021 13:37

ryanofsky mentioned this pull request Oct 8, 2021

[RFC] util: Add suffix byte unit parser for ArgsManager #23224

Closed

dougEfresh mentioned this pull request Oct 11, 2021

util: ParseByteUnits - Parse a string with suffix unit #23249

Merged

promag reviewed Oct 16, 2021

View reviewed changes

kiminuo reviewed Oct 16, 2021

View reviewed changes

ryanofsky force-pushed the pr/argscripts branch from 5c04ca8 to b8fd5d5 Compare October 16, 2021 15:57

ryanofsky commented Oct 16, 2021

View reviewed changes

src/util/system.cpp Show resolved Hide resolved

src/util/system.cpp Show resolved Hide resolved

src/util/system.h Show resolved Hide resolved

promag reviewed Oct 22, 2021

View reviewed changes

DrahtBot added the Needs rebase label Oct 22, 2021

ryanofsky and others added 3 commits October 25, 2021 10:44

refactor: Split InterpretOption into Interpret{Key,Value} functions

26a50ab

Co-authored-by: Anthony Towns <aj@erisian.com.au>

ryanofsky force-pushed the pr/argscripts branch from b8fd5d5 to c5d7e34 Compare October 25, 2021 15:47

DrahtBot removed the Needs rebase label Oct 25, 2021

ryanofsky mentioned this pull request Oct 25, 2021

Win64 CI failure: wine: chdir to /tmp/wine-tOJTCJ/server-1000a1-778801 : No such file or directory #23357

Closed

promag reviewed Oct 29, 2021

View reviewed changes

fanquake merged commit 3fc3641 into bitcoin:master Nov 1, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 1, 2021

Merge bitcoin#22766: refactor: Clarify and disable unused ArgsManager…

958b1ca

… flags

bitcoin locked and limited conversation to collaborators Nov 2, 2022

refactor: Clarify and disable unused ArgsManager flags #22766

refactor: Clarify and disable unused ArgsManager flags #22766

Uh oh!

Conversation

ryanofsky commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Sep 27, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

promag Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo left a comment

Choose a reason for hiding this comment

Uh oh!

kiminuo Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kiminuo Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 21, 2021 •

edited

Loading

DrahtBot commented Aug 21, 2021 •

edited

Loading

promag Oct 17, 2021 •

edited

Loading

ryanofsky commented Oct 16, 2021 •

edited

Loading

ryanofsky Oct 16, 2021 •

edited

Loading

ryanofsky Oct 16, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading