Merge settings one place instead of five places #15934

ryanofsky · 2019-05-01T21:48:50Z

This is a refactoring-only change that makes it easier to add a new settings source.

This PR doesn't change behavior. The util_ArgsMerge and util_ChainMerge tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

This change:

Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from Add <datadir>/settings.json persistent settings storage #15935).
Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places (GetArg, GetNetBoolArg, GetArgs, IsArgNegated, GetUnsuitableSectionOnlyArgs) in inconsistent ways.
Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line ignored arguments and more ignored arguments, and config file reverse precedence, inconsistently applied top-level settings, and zombie values.

The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent -wallet setting are pretty straightforward and show how the new code can be extended:

70675c3 from Add <datadir>/settings.json persistent settings storage #15935 – Add <datadir>/settings.json persistent settings storage
04c80c4 from Add loadwallet and createwallet load_on_startup options #15937 – Add loadwallet and createwallet RPC load_on_startup options

DrahtBot · 2019-05-02T00:43:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17385 (WIP: refactor: Use our own integer parsing/formatting everywhere by laanwj)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

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

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

Concept ACK.

src/util/system.cpp

ryanofsky · 2019-05-08T20:45:53Z

re: #15934 (review) from promag

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

This change does make it easier to add new settings sources (with consistent handling of negated args and things), so it should be compatible with your idea and maybe helpful.

Depending on the situation, I think having chained or scoped settings could be a good idea or not. I do think that in wallet code and application code generally it's good to get away from using key-value storage classes like ArgsManager or UniValue as quickly as possible, and switch to more direct representations like CCoinControl that are type safe and can be accessed more simply.

fanquake · 2019-05-24T13:58:49Z

Concept ACK

jamesob

Tested ACK 1d543ad

Generated and reviewed the test output locally. Mucked around with various argument formulations using the following config file:

dbcache=100
[main]
dbcache=200
[test]
dbcache=300

and commandline invocations e.g.

./src/bitcoind -conf=$(pwd)/test.conf -dbcache=1000 -dbcache=500 | grep Using

to verify dbcache being set as expected.

This is a well-written change that cleans up a lot of gnarly, duplicated settings munging. It explicitly outlines surprising corner cases in existing behavior (with docs too), and makes reasoning about settings merge order easier. This change also introduces substantial test coverage to settings management (util::Settings).

After this is merged, adding a read-write settings file (whether it's JSON or something else) will be much easier.

src/util/settings.cpp

src/util/system.cpp

jamesob · 2019-05-28T15:44:35Z

src/util/system.cpp

+        // Weird behavior preserved for backwards compatibility: command line
+        // options with section prefixes are allowed but ignored. It would be
+        // better if these options triggered the IsArgKnown error below, or were
+        // actually used instead of silently ignored.


Thanks for the nice comment. Potentially out of scope: could we log warnings for this instead of silently ignoring?

re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

jamesob · 2019-05-28T18:32:51Z

src/test/settings_tests.cpp

+// test parses and merges settings, representing the results as strings that get
+// compared against an expected hash. To debug, the result strings can be dumped
+// to a file (see comments below).
+BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup)


Cool test! The formatted output is really helpful. Encourage other reviewers to run and inspect with

SETTINGS_MERGE_TEST_OUT=results.txt ./src/test/test_bitcoin --run_test=settings_tests/Merge

src/test/util.h

src/test/settings_tests.cpp

ryanofsky

Updated 1d543ad -> 2dfeff1 (pr/mergeset.6 -> pr/mergeset.7), compare) with suggested changes.
Rebased 2dfeff1 -> 955c782 (pr/mergeset.7 -> pr/mergeset.8) to share common code with #15988.

src/test/settings_tests.cpp

src/util/settings.cpp

src/util/system.cpp

ryanofsky · 2019-05-29T15:49:15Z

src/util/system.cpp

+        // Weird behavior preserved for backwards compatibility: command line
+        // options with section prefixes are allowed but ignored. It would be
+        // better if these options triggered the IsArgKnown error below, or were
+        // actually used instead of silently ignored.


re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

src/test/util.h

jamesob · 2019-05-29T18:25:42Z

re-tACK 955c782 based on the interdiff and running an abbreviated version of the testing above.

ryanofsky · 2019-06-27T20:49:15Z

Rebased 955c782 -> 14a6dfc (pr/mergeset.8 -> pr/mergeset.9) due to conflict with #16278

jamesob · 2019-06-27T21:29:17Z

reACK 14a6dfc based on interdiff. Only change since pr/mergeset.8 is a trivial LogPrintf fix.

Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

includeconf -> conf_file_names to_include -> conf_file_name include_config -> conf_file_stream Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

Easier to review ignoring whitespace Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Suggested by Antoine Riard <[email protected]> bitcoin#15934 (comment) and John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Rename suggested by João Barbosa <[email protected]> bitcoin#16545 (comment) 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 e03483f bitcoin#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 (bitcoin#16545) This commit does not change behavior.

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Excludes 7f40528

Co-authored-by: UdjinM6 <[email protected]>

backport: bitcoin#15934, bitcoin#15864, bitcoin#19188, bitcoin#18338, bitcoin#19413, bitcoin#18571, bitcoin#18575 (deglobalization part 3)

This was referenced May 1, 2019

Add <datadir>/settings.json persistent settings storage #15935

Merged

interfaces: Expose settings.json methods to GUI #15936

Merged

Add loadwallet and createwallet load_on_startup options #15937

Merged

DrahtBot added Build system Tests Utils/log/libs labels May 1, 2019

promag reviewed May 2, 2019

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/mergeset branch 5 times, most recently from 19e84b7 to 1d543ad Compare May 8, 2019 15:59

ryanofsky changed the title ~~WIP: Dedup settings merge code~~ Dedup settings merge code May 14, 2019

ryanofsky marked this pull request as ready for review May 14, 2019 14:53

ryanofsky mentioned this pull request May 28, 2019

Docs: Add note on precedence of options in bitcoin.conf #16076

Closed

jamesob approved these changes May 28, 2019

View reviewed changes

ryanofsky force-pushed the pr/mergeset branch from 1d543ad to 955c782 Compare May 29, 2019 17:35

ryanofsky changed the title ~~Dedup settings merge code~~ Separate settings merging from parsing May 29, 2019

ryanofsky commented May 29, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 27, 2019

ryanofsky force-pushed the pr/mergeset branch from 955c782 to 14a6dfc Compare June 27, 2019 18:02

DrahtBot removed the Needs rebase label Jun 27, 2019

DrahtBot added the Needs rebase label Jun 28, 2019

ryanofsky force-pushed the pr/mergeset branch from 14a6dfc to d074e43 Compare June 28, 2019 14:08

Sjors mentioned this pull request Sep 29, 2021

Deprecate main and test -chain params #23090

Closed

Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this pull request Feb 17, 2022

Clarify emptyIncludeConf logic

29b1f0a

Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this pull request Feb 17, 2022

Remove includeconf nested scope

94a272b

Easier to review ignoring whitespace Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this pull request Feb 17, 2022

refactor: Clean up long lines in settings code

3857614

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this pull request Feb 17, 2022

refactor: Get rid of ArgsManagerHelper class

deb9db2

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

kwvg added a commit to kwvg/dash that referenced this pull request May 13, 2022

partial bitcoin#15934: Merge settings one place instead of five places

cf92976

Excludes 7f40528

kwvg mentioned this pull request Jun 2, 2022

backport: bitcoin#15934, #15864, #19188, #18338, #19413, #18571, #18575 (deglobalization part 3) dashpay/dash#4844

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 3, 2022

merge bitcoin#15934: Merge settings one place instead of five places

b37a692

Co-authored-by: UdjinM6 <[email protected]>

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Merge settings one place instead of five places #15934

Merge settings one place instead of five places #15934

Uh oh!

Conversation

ryanofsky commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented May 8, 2019

Uh oh!

fanquake commented May 24, 2019

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob May 28, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 29, 2019

Choose a reason for hiding this comment

Uh oh!

jamesob May 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented May 29, 2019

Uh oh!

ryanofsky commented Jun 27, 2019

Uh oh!

jamesob commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

ryanofsky commented May 1, 2019 •

edited

Loading

DrahtBot commented May 2, 2019 •

edited

Loading