refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

ryanofsky · 2019-11-24T20:05:39Z

This is based on #16545. The non-base commits are:

Except for two rpcauth and blockfilterindex fixes, this PR does not change any behavior outside of tests. It is just supposed to enforce internal consistency and prevent bugs by ensuring that list arguments are always retrieved with GetArgs() and non-list arguments are always retrieved with GetArg(). Followup PRs could use the ALLOW_LIST flags for better documentation and error checking in the future. For example, #17493 builds on this to disallow conflicting config values.

This change was originally made as part of #17493

hebasto · 2019-11-24T20:14:02Z

Concept ACK

DrahtBot · 2019-11-24T23:52:57Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	hebasto, promag, ajtowns

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27491 (refactor: Move chain constants to the util library by TheCharlatan)
#27480 (doc: Improve documentation of rpcallowip by willcl-ark)
#27446 (Allow configuring target block time for a signet by benthecarman)
#27302 (init: Error if ignored bitcoin.conf file is found by ryanofsky)
#27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)
#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26938 ([WIP] p2p: asmap, avoid inbound connections from a specific AS by brunoerg)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

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.

promag · 2019-12-22T22:10:11Z

~~Wrong commit bcbbc48?~~ nevermind.

Concept ACK.

ajtowns

Concept ACK, shame all these checks are happening at runtime rather than compile time though

src/util/system.cpp

ajtowns · 2020-01-09T04:18:46Z

src/test/util_tests.cpp

+        Arg* arg;
+        unsigned int prev_flags = arg ? arg->m_flags : 0;
+    };
+    //! Call GetArgs(), temporarily enabling ALLOW_LIST so call can succeed.


I think it might be better to restructure the tests than have these special functions that tweak the flags. Maybe something like:

str_config = "a=\n" "b=1\n" "c=foo\n" "noaaa=1\n" "r=0\n" "r=1\n"

(with a section as well to check overrides/repeats etc work correctly) and then try setting up all the args as ALLOW_ANY and check for appropriate errors, then all the args as ALLOW_BOOL and check for appropriate results/errors, and all the args as ALLOW_LIST and check for appropriate errors?

(Might be nicer to separate out the test infrastructure changes first, so that it's easy to see all the changes to test results in the commit that changes the functionality, but YMMV)

In commit "refactor: Always enforce ALLOW_LIST in CheckArgFlags" (4f3e5e9)

I think it might be better to restructure the tests than have these special functions that tweak the flags.

I'm not too inclined to want to restructure the tests, though I'd be happy to review a PR that did. I also wouldn't want to do it in this PR because I'd like the test changes here to reflect only reflect behavior that's changing, and not have other differences.

ajtowns · 2020-01-09T04:57:40Z

src/httprpc.cpp

@@ -260,7 +260,7 @@ static bool InitRPCAuthentication()
        LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
    }
-    if (gArgs.GetArg("-rpcauth","") != "")
+    if (!gArgs.GetArgs("-rpcauth").empty())


gArgs.IsArgSet("-rpcauth") ?

In commit "refactor: Fix more ALLOW_LIST arguments" (4bd1c3b)

gArgs.IsArgSet("-rpcauth") ?

That would do the wrong thing if the argument is negated. IsArgSet is generally broken and misused for list settings (and a lot of non-list settings), see #17783

ryanofsky

Thanks for the review!

shame all these checks are happening at runtime rather than compile time though

Agree, but I think adding checks and types like this PR and #17783 are doing is really the hard part. Switching enforcement from runtime to compile time after constraints are in already place would be easier by comparison

ryanofsky · 2020-01-09T20:23:27Z

src/httprpc.cpp

@@ -260,7 +260,7 @@ static bool InitRPCAuthentication()
        LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
        strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
    }
-    if (gArgs.GetArg("-rpcauth","") != "")
+    if (!gArgs.GetArgs("-rpcauth").empty())


In commit "refactor: Fix more ALLOW_LIST arguments" (4bd1c3b)

gArgs.IsArgSet("-rpcauth") ?

That would do the wrong thing if the argument is negated. IsArgSet is generally broken and misused for list settings (and a lot of non-list settings), see #17783

ryanofsky · 2020-01-09T20:23:37Z

src/test/util_tests.cpp

+        Arg* arg;
+        unsigned int prev_flags = arg ? arg->m_flags : 0;
+    };
+    //! Call GetArgs(), temporarily enabling ALLOW_LIST so call can succeed.


In commit "refactor: Always enforce ALLOW_LIST in CheckArgFlags" (4f3e5e9)

I think it might be better to restructure the tests than have these special functions that tweak the flags.

I'm not too inclined to want to restructure the tests, though I'd be happy to review a PR that did. I also wouldn't want to do it in this PR because I'd like the test changes here to reflect only reflect behavior that's changing, and not have other differences.

src/util/system.cpp

ryanofsky

fanquake · 2023-02-08T11:24:34Z

Moved this to draft for now, given it's based on another PR, which itself is a draft.

This commit does not change current application behavior. It adds new ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and ALLOW_LIST flags which are available to be used by new settings, without affecting parsing of any existing settings. The new flags validate settings earlier on startup and provide more detailed error messages to users. The new flags also provide stricter error checking. For example, a double negated option like -nosetting=0 is treated like an error instead of true, and an unrecognized bool value like -setting=true is treated like an error instead of false. The new flags also trigger errors if a non-list setting is specified multiple times in the same section of a configuration file, instead of silently ignoring later values. The new flags also provide type information that allows ArgsManager GetSettings() and GetSettingsList() methods to return typed integer and boolean values instead of unparsed strings. The changes have no effect on current application behavior because the new flags are only used in unit tests. The existing ALLOW_ANY checks in the CheckValueTest unit test confirm that no current validation or parsing behavior is changing.

Update GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods to work conveniently with ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags. This commit does not change application behavior in any way because these flags are new and currently only used in unit tests. The GetArg methods are convenience wrappers around the GetSetting method. The GetSetting method returns the originally parsed settings values in their declared bool/int/string types, while the GetArg wrappers provide extra type-coercion and default-value fallback features as additional conveniences for callers. This commit makes two changes to GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods when BOOL/INT/STRING flags are used: 1. GetArg methods will now raise errors if they are called with inconsistent flags. For example, GetArgs will raise a logic_error if it is called on a non-LIST setting, GetIntArg will raise a logic_error if it is called on a non-INT setting. 2. GetArg methods will now avoid various type coersion footguns when they are called on new BOOL/INT/STRING settings. Existing ALLOW_ANY settings are unaffected. For example, negated settings will return "" empty strings instead of "0" strings (in the past the "0" strings caused strangeness like "-nowallet" options creating wallet files named "0"). The new behaviors are fully specified and checked by the `CheckValueTest` unit test. The ergonomics of the GetArg helper methods are subjective and the behaviors they implement can be nitpicked and debated endlessly. But behavior of these helper methods does not dictate application behavior, and they be bypassed by calling GetSetting and GetSettingList methods instead. If it's necessary, behavior of these helper methods can also be changed again in the future. The changes have no effect on current application behavior because the new flags are only used in unit tests. The `setting_args` unit test and ALLOW_ANY checks in the `CheckValueTest` unit test are unchanged and confirm that `GetArg` methods behave exactly the same (returning the same values and throwing the same exceptions) for ALLOW_ANY flags before and after this change.

This change has no effect on behavior, and is basically just a documentation change at this point. The ALLOW_LIST flag is currently ignored unless ALLOW_BOOL, ALLOW_INT, or ALLOW_STRING flags are also present, and these flags are not used yet. -BEGIN VERIFY SCRIPT- for f in `git grep -n 'GetArgs(' | grep -v _tests | sed -n 's/.*GetArgs("$[^"]\+$".*/\1/p' | sort -u`; do git grep -l -- "$f" | xargs sed -i "/AddArg(\"$f[=\"]/ s/ArgsManager::ALLOW_ANY/& | ArgsManager::ALLOW_LIST/g" done -END VERIFY SCRIPT-

- Remove ALLOW_LIST flag from bitcoin-wallet -wallet and -debug arguments. They are list arguments for bitcoind, but single arguments for bitcoin-wallet. - Add ALLOW_LIST flag to -includeconf arg (missed by scripted diff since it's not accessed through GetArgs) - Add ALLOW_LIST flag to -debug, -loglevel, -whitebind, and -whitelist args (missed by scripted diff due to line breaks in AddArgs calls) - Add ALLOW_LIST flag to -zmq args (missed by scripted diff due to programmatic GetArgs calls) This change has no effect on behavior, and is basically just a documentation change at this point. The ALLOW_LIST flag is currently ignored unless ALLOW_BOOL, ALLOW_INT, or ALLOW_STRING flags are also present, and these flags are not used yet.

Previous behavior was nonsensical: - If an empty -rpcauth="" argument was specified as the last command line argument, it would cause all other -rpcauth arguments to be ignored. - If an empty -rpcauth="" argument was specified on the command line followed by any nonempty -rpcauth argument, it would cause an error. - If an empty "rpcauth=" line was specified after non-empty rpcauth line it would cause an error. - If an empty "rpcauth=" line in a config file was the entry in the config file it would cause all rpcauth entries to be ignored, unless the last command line argument was a nonempty -rpcauth argument, in which case it would be ignored. New behavior is simple: - If an empty "rpcauth=" config line or -rpcauth="" command line argument is used it will cause an error

Previous behavior was inconsistent: if -blockfilterindex or -blockfilterindex="" arguments were specified they would normally enable all block filter indexes, but could also trigger "Unknown -blockfilterindex value" errors if followed by later -blockfilterindex arguments. It was confusing that the same -blockfilterindex options could sometime trigger errors and sometimes not depending on option position. It was also confusing that an empty -blockfilterindex="" setting could enable all indexes even though indexes are disabled by default. New behavior is more straightforward: - -blockfilterindex and -blockfilterindex=1 always enable indexes - -noblockfilterindex and -blockfilterindex=0 always disable indexes - -blockfilterindex="" is always an unknown value error The meaning of these options no longer changes based on option position.

Prevent GetArg() from being called on ALLOW_LIST arguments, and GetArgs() from being called on non-list arguments. This checking was previously skipped unless typed INT/BOOL/STRING flags were present, but now it's always done. This change has no effect on external behavior. It is just supposed to enforce internal consistency and prevent bugs caused by using the wrong GetArg method to retrieve settings.

DrahtBot · 2023-05-09T16:37:47Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2023-08-07T02:00:46Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2023-11-05T01:27:58Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-02-04T01:27:37Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-05-03T01:26:23Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

ryanofsky mentioned this pull request Nov 24, 2019

util: Forbid ambiguous multiple assignments in config file #17493

Draft

fanquake added the Refactoring label Nov 24, 2019

This was referenced Nov 24, 2019

refactor: Remove settings merge reverse precedence code #17581

Draft

Simpler settings interpretation #17508

Open

DrahtBot added the Needs rebase label Dec 19, 2019

ryanofsky force-pushed the pr/wdlist branch from a036aa9 to 4f3e5e9 Compare December 20, 2019 15:23

ryanofsky mentioned this pull request Dec 20, 2019

util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior #17783

Draft

DrahtBot removed the Needs rebase label Dec 20, 2019

ajtowns reviewed Jan 9, 2020

View reviewed changes

ryanofsky commented Jan 9, 2020

View reviewed changes

ryanofsky mentioned this pull request Jan 23, 2020

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

ryanofsky force-pushed the pr/wdlist branch from 4f3e5e9 to e42d223 Compare January 24, 2020 21:21

ryanofsky commented Jan 24, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jan 30, 2020

ryanofsky force-pushed the pr/wdlist branch from e42d223 to 9621fae Compare September 2, 2020 20:19

DrahtBot removed the Needs rebase label Sep 2, 2020

DrahtBot added the Needs rebase label Sep 15, 2020

ryanofsky force-pushed the pr/wdlist branch from 9621fae to 1fe816e Compare September 25, 2020 20:22

DrahtBot removed the Needs rebase label Sep 25, 2020

This was referenced Sep 26, 2020

doc: Mention signet in -help output #20014

Merged

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

Merged

net: Add CNode::ConnectedThroughNetwork member function #19998

Merged

ryanofsky force-pushed the pr/wdlist branch from 27685d5 to dd3a716 Compare February 14, 2023 21:55

DrahtBot removed the Needs rebase label Feb 15, 2023

DrahtBot mentioned this pull request Mar 11, 2023

Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs #27231

Closed

DrahtBot added the Needs rebase label Mar 20, 2023

ryanofsky added 8 commits May 2, 2023 09:03

Merge remote-tracking branch 'origin/pull/16545/head'

a3f2490

ryanofsky force-pushed the pr/wdlist branch from dd3a716 to 7e70f98 Compare May 3, 2023 15:58

DrahtBot removed the Needs rebase label May 3, 2023

DrahtBot added the Needs rebase label May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

ryanofsky commented Nov 24, 2019 •

edited

Loading

hebasto commented Nov 24, 2019 •

edited

Loading

DrahtBot commented Nov 24, 2019 •

edited

Loading

promag commented Dec 22, 2019 •

edited

Loading

ajtowns left a comment

ajtowns Jan 9, 2020

ryanofsky Jan 9, 2020

ajtowns Jan 9, 2020

ryanofsky Jan 9, 2020

ryanofsky left a comment

ryanofsky Jan 9, 2020

ryanofsky Jan 9, 2020

ryanofsky left a comment •

edited

Loading

fanquake commented Feb 8, 2023

DrahtBot commented May 9, 2023

DrahtBot commented Aug 7, 2023

DrahtBot commented Nov 5, 2023

DrahtBot commented Feb 4, 2024

DrahtBot commented May 3, 2024

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

Are you sure you want to change the base?

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

Conversation

ryanofsky commented Nov 24, 2019 • edited Loading

hebasto commented Nov 24, 2019 • edited Loading

DrahtBot commented Nov 24, 2019 • edited Loading

Reviews

Conflicts

promag commented Dec 22, 2019 • edited Loading

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Jan 9, 2020

Choose a reason for hiding this comment

ryanofsky Jan 9, 2020

Choose a reason for hiding this comment

ajtowns Jan 9, 2020

Choose a reason for hiding this comment

ryanofsky Jan 9, 2020

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jan 9, 2020

Choose a reason for hiding this comment

ryanofsky Jan 9, 2020

Choose a reason for hiding this comment

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

fanquake commented Feb 8, 2023

DrahtBot commented May 9, 2023

DrahtBot commented Aug 7, 2023

DrahtBot commented Nov 5, 2023

DrahtBot commented Feb 4, 2024

DrahtBot commented May 3, 2024

ryanofsky commented Nov 24, 2019 •

edited

Loading

hebasto commented Nov 24, 2019 •

edited

Loading

DrahtBot commented Nov 24, 2019 •

edited

Loading

promag commented Dec 22, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading