util: Forbid ambiguous multiple assignments in config file

ryanofsky commented at 10:11 pm on November 15, 2019: contributor

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

1c3d5be2ad34 util: Forbid ambiguous multiple assignments in config file
c65e3b3b7d1e test: Extend util_ArgsMerge test to check for “Multiple values specified” errors

Enable error “Multiple values specified for -setting in same section of config file.”, for ALLOW_ANY settings that don’t specify ALLOW_LIST.

Instead of silently ignoring settings, this change makes it an error to provide an ambiguous config file that provides assigns multiple values to a single-value setting. Change includes release notes.

Part of the motivation for this change is to improve usability and prevent settings that look valid from being silently ignored. Another motivation is to be able to remove confusing “reverse precedence” logic in #17581

fanquake added the label Utils/log/libs on Nov 15, 2019

fanquake added the label Refactoring on Nov 15, 2019

DrahtBot commented at 0:31 am on November 16, 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.

Type	Reviewers
Concept NACK	ajtowns
Concept ACK	JeremyRubin

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.

in src/util/system.cpp:969 in e81c6ea28d outdated

808+        bool negated = !InterpretKey(section, key, &option.second);
809+        Optional<unsigned int> flags = GetArgFlags('-' + key);
810         if (flags) {
811-            if (!CheckValid(key, value, flags, error)) {
812+            if (!(*flags & ALLOW_LIST) && m_settings.ro_config[section].count(key)) {
813+                error = strprintf("Multiple values specified for -%s in same section of config file.", key);

JeremyRubin commented at 9:18 pm on November 18, 2019:

Perhaps only err if the value differs?

JeremyRubin commented at 9:23 pm on November 18, 2019: contributor

Concept ACK.

Curious if you think this has a high likelihood of breaking a lot of configs in the wild – but I suppose it’s worth it, as anything which could be intentionally repeated gets the new flag.

One interesting point: if something is not allow list, but receives a new param that is identical to the last set one, we could allow it? Something like this could be useful for certain bool settings if config files are being merged.

Noting for reference tracking that #12763 will require an update after this is merged.

ryanofsky force-pushed on Nov 21, 2019

ryanofsky force-pushed on Nov 24, 2019

ryanofsky referenced this in commit 8c93069558 on Nov 24, 2019

ryanofsky renamed this:
~~util: Forbid ambiguous repeated assignments in config file~~
util: Forbid ambiguous multiple assignments in config file
on Nov 24, 2019

Khailand69 approved

DrahtBot added the label Needs rebase on Dec 19, 2019

ryanofsky force-pushed on Dec 20, 2019

ryanofsky referenced this in commit 2dab31be62 on Dec 20, 2019

DrahtBot removed the label Needs rebase on Dec 20, 2019

ajtowns commented at 5:16 am on January 9, 2020: contributor

Weak concept NACK on this, precedence in config files might be confusing but it’s not ambiguous and specifying setting=1 \n setting=2 is something you might plausibly see in the wild, so this change has the potential to cause systems to not restart correctly… Maybe having a warning rather than an error for a release would be better? Still, better to make the change once for everything than gradually as things get switched away from ALLOW_ANY to something more specific.

ryanofsky commented at 8:18 pm on January 9, 2020: contributor

so this change has the potential to cause systems to not restart correctly…

This description sounds a little misleading to me. It is true that if someone had an ambiguous config file, and they updated to a new bitcoin version and then restarted without testing, they would see a “Multiple values specified for -foo in same section of config file” init error. But there wouldn’t be a question of correctness, or risk that the new version would start up using different settings than the old version.

Personally, I don’t think a debug log warning would be as good as an error here, given experience with issues like #15629 where warnings have seemed pretty easy to miss. I understand your concern about the case where someone upgrades their version of bitcoin and sees an error about an insignificant ambiguity in their config file. But I wouldn’t want to forget about cases where someone could upgrade bitcoin be prompted to fix config errors that actually affected their privacy or security.

I wonder what you think about Jeremy’s suggestion to prevent errors in more spurious cases where a setting is repeated but the value doesn’t change: #17493#pullrequestreview-318636254. It’d be pretty easy to tweak the PR to avoid triggering errors in these cases.

ajtowns commented at 5:52 am on January 23, 2020: contributor

This description sounds a little misleading to me.

Sure; I meant “restart correctly” as in “keep working the way things are supposed to rather than gratuitously break and cause the sysadmin headaches” :) But yeah, I agree a warning everyone will miss isn’t that helpful, and there aren’t any better approaches coming to mind; really that’s what release notes and being careful about upgrades are for.

I’d probably wait to see someone using it in real life before adding special cases for duplicate settings with the same param/value.

DrahtBot added the label Needs rebase on Jan 30, 2020

ryanofsky force-pushed on Sep 28, 2020

DrahtBot removed the label Needs rebase on Sep 28, 2020

ryanofsky referenced this in commit 6285d00931 on Sep 28, 2020

ryanofsky referenced this in commit 4c30ac5006 on Sep 28, 2020

DrahtBot added the label Needs rebase on Sep 29, 2020

maflcko referenced this in commit 11cbd4bb54 on Jan 21, 2021

ryanofsky force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

ryanofsky referenced this in commit a9bc4b2151 on Apr 13, 2021

ryanofsky referenced this in commit 6c5a7d55e9 on Apr 13, 2021

ryanofsky referenced this in commit 58efdc25a2 on Apr 14, 2021

ryanofsky referenced this in commit 33efdead6e on Apr 14, 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

ryanofsky referenced this in commit 4b35d9dd2f on Jun 17, 2021

ryanofsky referenced this in commit 2d7e4aab89 on Jun 17, 2021

DrahtBot added the label Needs rebase on Aug 22, 2021

ryanofsky force-pushed on Dec 30, 2021

ryanofsky referenced this in commit 3376e470a0 on Dec 30, 2021

ryanofsky referenced this in commit 0eddb582bf 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

ryanofsky referenced this in commit bf8d006a74 on Sep 27, 2022

ryanofsky referenced this in commit 9f9c377780 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 30, 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:25 am on February 8, 2023: member

Moved this to draft for now, given it’s based on multiple other PRs, some of which are also drafts.

ryanofsky force-pushed on Feb 15, 2023

ryanofsky referenced this in commit a057e0c38f on Feb 15, 2023

ryanofsky referenced this in commit 44a9c4353d on Feb 15, 2023

DrahtBot removed the label Needs rebase on Feb 15, 2023

DrahtBot added the label Needs rebase on Mar 20, 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.

e2b9de219b

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

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

870160359e

Always reject empty -blockfilterindex="" arguments

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.

7e70f985ea

Merge remote-tracking branch 'origin/pull/17580/head' ba2c86c86a

util: Forbid ambiguous multiple assignments in config file

Enable error "Multiple values specified for -setting in same section of config
file.", for ALLOW_ANY settings that don't specify ALLOW_LIST.

Instead of silently ignoring settings, this change makes it an error to provide
an ambiguous config file that provides assigns multiple values to a
single-value setting. Change include release notes.

1c3d5be2ad

test: Extend util_ArgsMerge test to check for "Multiple values specified" errors

Also skip calling TestArgString in these cases since exact behavior retrieving
values is irrelevant when parsing values fails.

Also s/TestArgString/GetArg/ to be more efficient and direct, now that it's no
longer necessary to call with separation of ALLOW_LIST and non ALLOW_LIST
cases.

c65e3b3b7d

ryanofsky force-pushed on May 3, 2023

ryanofsky referenced this in commit 55b31d8a92 on May 3, 2023

ryanofsky referenced this in commit 40476a8b5d 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.

vijaydasmp referenced this in commit 7c9205be08 on Nov 7, 2023

vijaydasmp referenced this in commit 9c834f5546 on Nov 8, 2023

vijaydasmp referenced this in commit 087ebdb8f5 on Nov 9, 2023

vijaydasmp referenced this in commit b6de87d245 on Nov 9, 2023

vijaydasmp referenced this in commit 93d3f1e777 on Nov 25, 2023

vijaydasmp referenced this in commit 4a2d0ebd24 on Nov 25, 2023

vijaydasmp referenced this in commit 38e6e86035 on Nov 26, 2023

vijaydasmp referenced this in commit 3fa8e6127f on Nov 27, 2023

vijaydasmp referenced this in commit 73d0cceff7 on Dec 2, 2023

vijaydasmp referenced this in commit dd7aeee838 on Dec 4, 2023

PastaPastaPasta referenced this in commit ac7a1cd2b7 on Dec 6, 2023

UdjinM6 referenced this in commit b8267bb5a2 on Dec 8, 2023

DrahtBot commented at 0:42 am on February 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.

DrahtBot commented at 1:56 am on May 2, 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.

util: Forbid ambiguous multiple assignments in config file #17493

Reviews

Conflicts