refactor: Settings code cleanups

ryanofsky commented at 4:37 am on November 14, 2019: member

This PR doesn’t change behavior. It just implements some suggestions from #15934 and #16545 and few other small cleanups.

refactor: Clean up long lines in settings code

Suggested by James O'Beirne <james.obeirne@gmail.com>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344366743

This commit does not change behavior.

3f7dc9b808

refactor: Clean up includeconf comments

Suggested by Antoine Riard <ariard@student.42.fr>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344291875

and John Newbery <john@johnnewbery.com>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344271224

This commit does not change behavior.

57e8b7a727

refactor: Replace FlagsOfKnownArg with GetArgFlags

Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-519048000

This also gets rid of ArgsManager::NONE constant, which was an implementation
detail not meant to be used by ArgsManager callers.

Finally this reverts a change from 7f40528cd50fc43ac0bd3e785de24d661adddb7a
https://github.com/bitcoin/bitcoin/pull/15934 adding "-" characters to argument
names. Better for GetArgFlags to require "-" prefixes for consistency with
other ArgsManager methods, and to be more efficient later when GetArg functions
need to call GetArgFlags (https://github.com/bitcoin/bitcoin/pull/16545)

This commit does not change behavior.

dc0f148074

refactor: Get rid of ArgsManagerHelper class

Suggested by John Newbery <john@johnnewbery.com>
https://github.com/bitcoin/bitcoin/pull/15934#issuecomment-551969778

This commit does not change behavior.

3e185522ac

refactor: Add ArgsManager::GetSettingsList method

Add for consistency with ArgsManager::GetSetting method and to make setting
types accessible to ArgsManager callers and tests (test added next commit).

This commit does not change behavior.

0fa54358b0

refactor: Add util_CheckValue test

Test GetSetting and GetArg type coercion, negation, and default value handling.
Test is expanded later to cover other flags besides ALLOW_ANY when they are
implemented in https://github.com/bitcoin/bitcoin/pull/16545

This commit does not change behavior.

425bb30725

scripted-diff: Remove unused ArgsManager type flags in tests

The bool/int/string flags were added speculatively in #16097 and trigger errors
when type checking is actually implemented in
https://github.com/bitcoin/bitcoin/pull/16545

-BEGIN VERIFY SCRIPT-
sed -i 's/ALLOW_\(BOOL\|INT\|STRING\)/ALLOW_ANY/g' src/test/util_tests.cpp src/test/getarg_tests.cpp
-END VERIFY SCRIPT-

This commit does not change behavior.

cba2710220

refactor: Remove null setting check in GetSetting()

Also rename the "result_complete" variable in GetSettingsList() to "done" to be
more consistent with GetSetting().

This change doesn't affect current behavior but could be useful in the future
to support dynamically changing settings at runtime and adding new settings
sources, because it lets high priority sources reset settings back to default
(see test).

By removing a special case for null, this change also helps merge code treat
settings values more like black boxes, and interfere less with settings parsing
and retrieval.

e9fd366044

fanquake added the label Refactoring on Nov 14, 2019

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

DrahtBot commented at 8:58 am on November 14, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#17482 (util: Disallow network-qualified command line options by ryanofsky)
#16673 (Relog configuration args on debug.log rotation by LarryRuane)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
#16115 (On bitcoind startup, write config args to debug.log by LarryRuane)
#15937 (WIP: Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15936 (WIP: Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#15935 (WIP: Add /settings.json persistent settings storage 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.

hebasto commented at 9:28 am on November 14, 2019: member

Concept ACK 6f018d367302fdf79174c0981e76fc32ec9d77b2

ryanofsky force-pushed on Nov 14, 2019

in src/util/system.h:278 in 9926e06fe9 outdated

269@@ -270,6 +270,27 @@ class ArgsManager
270      */
271     std::string GetChainName() const;
272 
273+    /**
274+     * Returns true if settings values from the default section should be used,
275+     * depending on the current network and whether the setting is
276+     * network-specific.
277+     */
278+    bool UseDefaultSection(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args);

promag commented at 8:02 am on November 15, 2019:

nit, could be private.

jnewbery commented at 7:10 pm on December 12, 2019:

👍

Same for GetSetting() below

ryanofsky commented at 10:00 pm on December 17, 2019:

re: #17473 (review)

nit, could be private.

Same for GetSetting() below

Made protected, since these are used in tests.

promag commented at 8:07 am on November 15, 2019: member

Concept ACK. Is bcf8060557d544fa881be9e74c2e108189010dca really necessary, not sure the motivation of it.

ryanofsky commented at 10:35 pm on November 15, 2019: member

Concept ACK. Is bcf8060 really necessary, not sure the motivation of it.

As the commit message says, some of the flags are incorrectly applied and cause tests to fail when type checking is actually implemented in #16545. At first I tried to selectively update the flags instead of using a scripted diff, but as I was iterating on #16545 more of these old tests kept failing and I kept going back and forth on the flags. I think bottom line is these tests were never written to interact with flags, and I have much better dedicated tests for the flags in #16545. I doubt #16545 will be the last PR changing these flags, and I want to avoid old tests becoming a maintenance burden for more flag changes in the future.

Reason for including the commit in this PR instead of delaying it for the future, is that right now the tests are misleading and make it look like unused unimplemented constants could actually have an effect.

in src/util/settings.cpp:88 in 9926e06fe9 outdated

84+
85         // Ignore settings in default config section if requested.
86-        if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && !never_ignore_negated_setting) return;
87+        if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION &&
88+            !never_ignore_negated_setting)
89+            return;

jnewbery commented at 6:49 pm on December 12, 2019:

style nit: use braces for single-line if block

ryanofsky commented at 10:00 pm on December 17, 2019:

re: #17473 (review)

style nit: use braces for single-line if block

Thanks, fixed

in src/util/system.h:320 in 9926e06fe9 outdated

318@@ -298,7 +319,7 @@ class ArgsManager
319      * Return Flags for known arg.
320      * Return ArgsManager::NONE for unknown arg.

jnewbery commented at 6:58 pm on December 12, 2019:

Comment is outdated

ryanofsky commented at 10:00 pm on December 17, 2019:

re: #17473 (review)

Comment is outdated

Thanks, fixed

jnewbery commented at 8:07 pm on December 12, 2019: member

utACK 9926e06fe9aa258ae06bf2b6e9c7f589f7c8a565 with just a few small style nits

ryanofsky force-pushed on Dec 18, 2019

in src/test/util_tests.cpp:303 in 425bb30725 outdated

298+    CheckValue(M::ALLOW_ANY, "-value", Expect{""}.String("").Int(0).Bool(true).List({""}));
299+    CheckValue(M::ALLOW_ANY, "-value=", Expect{""}.String("").Int(0).Bool(true).List({""}));
300+    CheckValue(M::ALLOW_ANY, "-value=0", Expect{"0"}.String("0").Int(0).Bool(false).List({"0"}));
301+    CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"}));
302+    CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"}));
303+    CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));

ryanofsky commented at 5:04 pm on December 18, 2019:

In commit “refactor: Add util_CheckValue test” (425bb307252cf4dec9b3ef6426e6548b2be7a303)

Note: this test is extended in 909bcca7971063f42de6d981111c628bf5a0ebfa from #16545 to check other flags besides ALLOW_ANY

ryanofsky commented at 5:04 pm on December 18, 2019: member

Updated 9926e06fe9aa258ae06bf2b6e9c7f589f7c8a565 -> e9fd366044e271632dc0e4f96e1c14f8e87213ae (pr/cleanset.2 -> pr/cleanset.3, compare) with suggestions from João and John

jnewbery commented at 7:30 pm on December 19, 2019: member

Code review ACK e9fd366044e271632dc0e4f96e1c14f8e87213ae

MarcoFalke commented at 8:05 pm on December 19, 2019: member

ACK e9fd366044 🚟

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e9fd366044 🚟
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjBJQwAgNgNoo3sHg6IyTw00ye/IHSr222mR3aGg6NhiAoVziJ7US66yyt/c85I
 8s6nxdJe/5b4rkmNNID7bAh/VJNYAz4gNgMARcYngShCbQTRADKzeauIY8ZzBZFgm
 9W+uay6TWLKQM55E7y5jBBYmUFy1D5efmLtKR9wktp22eeMXlJMhLkI40nSfq4Tkt
10aOSX6K7a5aaIImJs1SShl0nHsqF6z+al2MVuWpBxPZ34Wkn257p6G8BXwX6mNTNH
11lxzjuPj6aHuWI496G7zbNBA6IkSGZgBE4/HcPCyYz+i16LzaqyYX/pWwtoRnSjME
12BVCeqQEg3JFoNZtWsazzGpsh/Ki29X15jZwz1UrNYIXlf2v4QCC4Cj0CZe9P5QWc
13+Ib+OyLoecz3+/WbGBWO6Lv+LKnPpyMjNj7bRNzJHikq6nw1wklbPMnOfxrJNbQv
14xNTgZDLj6Z+Wp9TLD1jua7q2ciM+g7v3ADDYIh4UPJnLulAP8ML/pNKb0t1p5gne
15L8iVPKtM
16=p2lQ
17-----END PGP SIGNATURE-----

Timestamp of file with hash 31d53ff8d0571f29f549510cc92e83930bbab33e7884245727870371ceabaaad -

MarcoFalke referenced this in commit 6677be64f6 on Dec 19, 2019

MarcoFalke merged this on Dec 19, 2019

MarcoFalke closed this on Dec 19, 2019

fanquake deleted a comment on Dec 19, 2019

sidhujag referenced this in commit 33fe88ae5b on Dec 19, 2019

in src/test/util_tests.cpp:246 in e9fd366044

241+            BOOST_CHECK_EQUAL(settings_list[0].write(), expect.setting.write());
242+        }
243+
244+        if (expect.error) {
245+            BOOST_CHECK(!success);
246+            BOOST_CHECK_NE(error.find(expect.error), std::string::npos);

MarcoFalke commented at 2:07 pm on December 20, 2019:

Why not

 0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index 86ea56ff36..f2f8ebd59a 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -243,7 +243,7 @@ public:
 5 
 6         if (expect.error) {
 7             BOOST_CHECK(!success);
 8-            BOOST_CHECK_NE(error.find(expect.error), std::string::npos);
 9+            BOOST_CHECK_EQUAL(error, expect.error);
10         } else {
11             BOOST_CHECK(success);
12             BOOST_CHECK_EQUAL(error, "");

? I presume later tests require the less strict check?

ryanofsky commented at 3:18 pm on December 20, 2019:

I think it should be possible to make the change you’re suggesting, which would give more helpful debug info if the check fails. This was originally written as part of #16545 which at one point was doing partial checks for error messages, but now I think checks the whole error string.

ryanofsky commented at 9:12 pm on January 8, 2020:

Why not […]? I presume later tests require the less strict check?

Updated #16545 to use the suggestion and more strict checks

deadalnix referenced this in commit 619943a5c9 on Apr 30, 2020

sidhujag referenced this in commit 96245fcfb0 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Settings code cleanups #17473

Conflicts