Make negating list options act the same as not specifying list options for -norpcallowip, -norpcbind, -nobind, -nowhitebind, -noconnect, -noexternalip, -noonlynet, -nosignetchallenge, -nosignetseednode, -norpcwallet. Instead of using IsArgSet() use !GetArgs().empty() so negation does not produce unexpected side effects, which are detailed in commit messages.
fanquake added the label
Utils/log/libs
on Dec 20, 2019
DrahtBot
commented at 5:56 pm on December 20, 2019:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
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)
#27071 (Handle CJDNS from LookupSubNet() by vasild)
#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
commented at 11:53 pm on December 22, 2019:
member
Concept ACK.
DrahtBot added the label
Needs rebase
on Jan 30, 2020
ryanofsky force-pushed
on Sep 3, 2020
DrahtBot removed the label
Needs rebase
on Sep 3, 2020
DrahtBot added the label
Needs rebase
on Sep 15, 2020
ryanofsky force-pushed
on Sep 28, 2020
ryanofsky
commented at 11:48 am on September 28, 2020:
contributor
DrahtBot removed the label
Needs rebase
on Sep 28, 2020
DrahtBot added the label
Needs rebase
on Sep 29, 2020
ryanofsky force-pushed
on Apr 12, 2021
DrahtBot removed the label
Needs rebase
on Apr 12, 2021
ryanofsky force-pushed
on Apr 12, 2021
ryanofsky force-pushed
on Apr 12, 2021
DrahtBot added the label
Needs rebase
on Apr 15, 2021
ryanofsky force-pushed
on Jun 16, 2021
DrahtBot removed the label
Needs rebase
on Jun 16, 2021
DrahtBot added the label
Needs rebase
on Jul 21, 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 27, 2022
DrahtBot removed the label
Needs rebase
on Sep 27, 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
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 16, 2023
achow101 marked this as a draft
on Apr 25, 2023
Add ArgsManager flags to parse and validate settings on startup
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.
68347d6950
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. 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.
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-
76d30ef525
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.
49e25e1fa7
Always reject empty -rpcauth="" arguments
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.
d97b30d33c
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.
Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior
This change fixes some corner cases handling negated list options:
-norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind,
-noconnect, -noexternalip, -noonlynet, -nosignetchallenge, and
-nosignetseednode. Negating these options is now the same as not
specifying them at all. This is useful for being able to override config
file options on the command line without seeing parameter interaction
side effects from the otherwise ignored config options.
The code change here is just avoid calling the IsArgSet() function on
ALLOW_LIST options, and to disallow such calls in the future. Code that uses
IsArgSet() with list options is confusing and leads to mistakes due to the easy
to overlook case where an argument is negated and IsArgSet() returns true, but
GetArgs() returns an empty list.
This change includes release notes, but the release notes don't go into details
about specific options. For reference this change:
- Treats specifying -norpcwhitelist exactly the same as not specifying any
-rpcwhitelist, instead of behaving almost the same but flipping the default
-rpcwhitelistdefault value.
- Treats specifying -norpcallowip and -norpcbind exactly the same as not
specifying -rpcallowip or -rpcbind, instead of failing to bind to localhost
and failing to show warnings when one value is set without the other.
- Treats specifying -nobind, -nowhitebind, and -noconnect exactly the same as
not specifying -bind, -whitebind, or -connect values instead of treating them
almost the same but causing parameter interactions with -dnsseed, -listen,
and m_use_addrman_outgoing values.
- Treats specifying -noexternalip exactly the same as not specifying any
-externalip, instead of treating it almost the same but interacting with the
-discover value.
- Treats specifying -noonlynet exactly the same as not specifying -onlynet
instead of marking all networks unreachable.
- Treats specifying -nosignetchallenge exactly the same as not specifying
-signetchallenge instead of throwing strange error "-signetchallenge cannot
be multiple values"
- Clarifies -vbparams and -debug handling code and fixes misleading comments
without changing behavior.
a746ae0520
Fix nonsensical bitcoin-cli -norpcwallet behavior
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.
This restores previous behavior before 743077544b5420246ef29e0b708c90e3a8dfeeb6
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
f59bfafcc8
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
DrahtBot
commented at 4:37 pm on May 9, 2023:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot
commented at 2:00 am on August 7, 2023:
contributor
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
commented at 1:27 am on November 5, 2023:
contributor
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
commented at 1:27 am on February 4, 2024:
contributor
⌛ 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
commented at 1:26 am on May 3, 2024:
contributor
⌛ 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.
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-07-03 10:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
