Except for two rpcauth and blockfilterindex fixes (update: rpcauth fix was merged separately in #30401) 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.
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:
#31483 (kernel: Move kernel-related cache constants to kernel cache by TheCharlatan)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#31223 (net, init: derive default onion port if a user specified a -port by mzumsande)
#29365 (Extend signetchallenge to set target block spacing by starius)
#28792 (Embed default ASMap as binary dump header file by fjahr)
#26966 (index: initial sync speedup, parallelize process by furszy)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
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.
DrahtBot added the label
Needs rebase
on Dec 19, 2019
ryanofsky force-pushed
on Dec 20, 2019
DrahtBot removed the label
Needs rebase
on Dec 20, 2019
promag
commented at 10:10 pm on December 22, 2019:
contributor
I don’t think it makes sense to ever include any of the ALLOW_ANY types in forbid (if you don’t want a string, then in reality you probably require a boolean eg), so having:
0 constexpr unsigned int ALLOW_ANY_EQUIV = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING;
12 assert((forbid & ALLOW_ANY_EQUIV) ==0);
3if (*flags & ALLOW_ANY) require &=~ALLOW_ANY_EQUIV;
45..67return (*flags & ALLOW_ANY) ==0; // since ignore_types is gone now
might make sense?
ryanofsky
commented at 8:23 pm on January 9, 2020:
In commit “refactor: Always enforce ALLOW_LIST in CheckArgFlags” (4f3e5e9988280ac96b7979c352da218bbf939ed0):
I see your point that CheckArgFlags is never called forbidding bool/int/string flags, so no need to update forbid and probably no reason to even want to, so I could make a simplification along those lines. I think I will at least drop the forbid &= ~ignore_types line.
in
src/test/util_tests.cpp:344
in
4f3e5e9988outdated
(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)
ryanofsky
commented at 8:23 pm on January 9, 2020:
In commit “refactor: Always enforce ALLOW_LIST in CheckArgFlags” (4f3e5e9988280ac96b7979c352da218bbf939ed0)
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.
in
src/httprpc.cpp:254
in
4bd1c3b160outdated
259@@ -260,7 +260,7 @@ static bool InitRPCAuthentication()
260 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");
261 strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
262 }
263- if (gArgs.GetArg("-rpcauth","") != "")
264+ if (!gArgs.GetArgs("-rpcauth").empty())
ryanofsky
commented at 8:23 pm on January 9, 2020:
In commit “refactor: Fix more ALLOW_LIST arguments” (4bd1c3b1606021dff2b39a1927781b3c53428bbe)
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
ajtowns
commented at 5:05 am on January 9, 2020:
contributor
Concept ACK, shame all these checks are happening at runtime rather than compile time though
ryanofsky
commented at 9:10 pm on January 9, 2020:
contributor
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 force-pushed
on Jan 24, 2020
ryanofsky
commented at 9:23 pm on January 24, 2020:
contributor
DrahtBot added the label
Needs rebase
on Jan 30, 2020
ryanofsky force-pushed
on Sep 2, 2020
DrahtBot removed the label
Needs rebase
on Sep 2, 2020
DrahtBot added the label
Needs rebase
on Sep 15, 2020
ryanofsky force-pushed
on Sep 25, 2020
DrahtBot removed the label
Needs rebase
on Sep 25, 2020
ryanofsky force-pushed
on Sep 28, 2020
DrahtBot added the label
Needs rebase
on Sep 29, 2020
ryanofsky force-pushed
on Oct 2, 2020
DrahtBot removed the label
Needs rebase
on Oct 2, 2020
DrahtBot added the label
Needs rebase
on Oct 29, 2020
ryanofsky force-pushed
on Apr 10, 2021
DrahtBot removed the label
Needs rebase
on Apr 10, 2021
ryanofsky force-pushed
on Apr 11, 2021
DrahtBot added the label
Needs rebase
on Apr 15, 2021
ryanofsky force-pushed
on Jun 2, 2021
DrahtBot removed the label
Needs rebase
on Jun 2, 2021
DrahtBot added the label
Needs rebase
on Aug 22, 2021
ryanofsky force-pushed
on Aug 24, 2021
ryanofsky force-pushed
on Dec 30, 2021
DrahtBot removed the label
Needs rebase
on Dec 30, 2021
DrahtBot added the label
Needs rebase
on Feb 9, 2022
uvhw referenced this in commit
47d44ccc3e
on Feb 14, 2022
ryanofsky force-pushed
on Sep 22, 2022
DrahtBot removed the label
Needs rebase
on Sep 22, 2022
ryanofsky force-pushed
on Sep 24, 2022
DrahtBot added the label
Needs rebase
on Nov 15, 2022
ryanofsky force-pushed
on Nov 29, 2022
DrahtBot removed the label
Needs rebase
on Nov 29, 2022
DrahtBot added the label
Needs rebase
on Jan 11, 2023
fanquake marked this as a draft
on Feb 8, 2023
fanquake
commented at 11:24 am on February 8, 2023:
member
Moved this to draft for now, given it’s based on another PR, which itself is a draft.
ryanofsky force-pushed
on Feb 14, 2023
DrahtBot removed the label
Needs rebase
on Feb 15, 2023
DrahtBot added the label
Needs rebase
on Mar 20, 2023
ryanofsky force-pushed
on May 3, 2023
DrahtBot removed the label
Needs rebase
on May 3, 2023
DrahtBot added the label
Needs rebase
on May 9, 2023
ryanofsky force-pushed
on Jul 19, 2024
DrahtBot removed the label
Needs rebase
on Jul 20, 2024
DrahtBot added the label
Needs rebase
on Aug 5, 2024
ryanofsky force-pushed
on Aug 6, 2024
DrahtBot removed the label
Needs rebase
on Aug 6, 2024
DrahtBot added the label
Needs rebase
on Aug 7, 2024
common: Grammar / formatting tweaks
Implement cleanup suggestions from l0rinc:
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773221823
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773291395
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382
20a9986751
doc: Add detailed ArgsManager type flag documention
This commit just adds documentation for the type flags. The flags are actually
implemented in the following two commits.
30385f2976
common: Implement ArgsManager flags to parse and validate settings on startup
This commit implements support for new ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and
ALLOW_LIST flags by validating settings with these flags earlier on startup and
providing detailed error messages to users.
The new flags implement stricter error checking than ALLOW_ANY. 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. And if a non-list setting is assigned multiple times in
the same section of a configuration file, the later assignments trigger errors
instead of being silently ignored.
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 in this commit have no effect on current application behavior
because the new flags are only used in unit tests. The existing ALLOW_ANY
checks in the argsman_tests/CheckValueTest confirm that no behavior is changing
for current settings, which use ALLOW_ANY.
a7a35ed080
common: Update ArgManager GetArg helper methods to work better with ALLOW flags
Update GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods to work
conveniently with ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags.
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 can 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 the same as before for ALLOW_ANY flags (returning the
same values and throwing the same exceptions).
225ab2bf73
test: Add tests to demonstrate usage of ArgsManager flags
test: Add test for settings.json parsing with type flags
The type flags aren't currently used to validate or convert settings in the
settings.json file, but they should be in the future. Add test to check current
behavior that can be extended when flags are applied.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
common: Add support for combining ArgsManager flags
Let ALLOW_STRING and ALLOW_INT flags be combined with ALLOW_BOOL so string and
int options can be specified without explicit values. This is useful for
imperative settings that trigger new behavior when specified and can accept
optional string or integer values, but do not require them. (For examples, see
the example_options unit test modified in this commit.)
da3bdd5f00
scripted-diff: Add ALLOW_LIST flag to arguments retrieved with GetArgs
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-
603423e139
refactor: Fix more ALLOW_LIST arguments
- 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 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.
7a756355c1
refactor: Always enforce ALLOW_LIST in CheckArgFlags
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.
d2edef6b1a
ryanofsky force-pushed
on Dec 6, 2024
ryanofsky
commented at 4:21 pm on December 6, 2024:
contributor
Dropping commit 811670650471b352b5e0755e6f38b85154c2bfb3 which is no longer needed after #30401
Rebased a1d65066621a21ea35c4ec8363cfabb862fb5239 -> 8723b8ed56a03394663945bcdaa00039ccdff5f6 (pr/wdlist.19 -> pr/wdlist.20, compare) due to various conflicts
Updated 8723b8ed56a03394663945bcdaa00039ccdff5f6 -> d2edef6b1a0bde00a29290229a5964e413c49696 (pr/wdlist.20 -> pr/wdlist.21, compare) adding commit da3bdd5f00b3dcd380b7bd67d2d54ec7c4b9c2bb which previously was part of base PR #16545 but was dropped there to make review easier. This functionality in this commit is needed for the -blockfilterindex argument
ryanofsky force-pushed
on Dec 6, 2024
DrahtBot added the label
CI failed
on Dec 6, 2024
DrahtBot
commented at 4:29 pm on December 6, 2024:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
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.
DrahtBot removed the label
Needs rebase
on Dec 6, 2024
DrahtBot removed the label
CI failed
on Dec 6, 2024
DrahtBot added the label
Needs rebase
on Dec 14, 2024
DrahtBot
commented at 1:18 am on December 14, 2024:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
