Add <datadir>/settings.json persistent settings storage

ryanofsky commented at 9:49 pm on May 1, 2019: member

Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.

DrahtBot added the label Build system on May 1, 2019

DrahtBot added the label GUI on May 1, 2019

DrahtBot added the label Tests on May 1, 2019

DrahtBot added the label Utils/log/libs on May 1, 2019

DrahtBot commented at 0:43 am on May 2, 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:

#19561 (refactor: Pass ArgsManager into functions that register args by S3RK)
#19471 (util: Make default arg values more specific by hebasto)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
#15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#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.

Coverage

Coverage	Change (pull 15935, 8932b6eca8c024905823f7c21f424a3da024bc72)	Reference (master, 597d2f905ecab4fe2c64bba13f8e76e0075b2f9f)
Lines	-0.0402 %	90.8980 %
Functions	-0.0155 %	85.7548 %
Branches	-0.0263 %	52.0202 %

Updated at: 2020-07-22T10:37:55.716811.

luke-jr commented at 12:09 pm on May 2, 2019: member

NACK, this is redundant with #11082 which is a better solution.

ryanofsky force-pushed on May 2, 2019

ryanofsky referenced this in commit 87c923c7ca on May 3, 2019

ryanofsky force-pushed on May 3, 2019

ryanofsky referenced this in commit 43d6998991 on May 7, 2019

ryanofsky force-pushed on May 7, 2019

ryanofsky referenced this in commit 83a4467441 on May 8, 2019

ryanofsky force-pushed on May 8, 2019

ryanofsky commented at 8:35 pm on May 8, 2019: member

re: #15935 (comment)

Just as background, the settings file is not supposed to replace the config file. The settings file is supposed to replace some QSettings uses in bitcoin-qt, implementing type-safe persistent storage, and sharing this functionality with bitcoind, so settings modified at runtime will be stored in a common place regardless of whether you’re using the GUI or a plain RPC interface. (You can see in 0ab41dd770c1f13983df528b111cfc8a51fe016a from #15936 how this works in the GUI or in 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 how this can be used in an RPC.)

this is redundant with #11082 which is a better solution.

Every solution has some tradeoffs, and I’m sure you can elaborate more on advantages of #11082 compared to this. But here are what I see as nice things about this approach:

Standard format. The bitcoin.conf format works pretty well as a manual configuration mechanism. But the format is unspecified and unstable, and less appealing for use as persistent key/value store compared to JSON which is well understood and supported by many tools and libraries.
Uniformity of representation. Since bitcoin.conf files are meant to be written by people and parsed by software, it’s reasonable for them to provide many ways to do things like represent true/false values (foo=1 foo= foo=23 foo=-1, nofoo=0 for true / foo=0 nofoo=1 foo=false for false), or to strip whitespace. But for a file that is meant to be round-trip read and written by software, only accepting plain true false keywords as bools and not mangling strings is a better way to avoid complexity, configuration errors, and software bugs.
Flexibility of representation. bitcoin.conf format can’t currently represent strings with pound (#) or newline characters or strings with various types of whitespace. It can also only awkwardly represent lists by repeating settings (so there is no distinction between a non-list value and a single-element list). I think these things are likely to cause bugs, unnecessary limitations, headaches, and extra boilerplate in application-level code that reads and writes settings.
Maintainability. JSON for all that’s good and bad about it is stable and usable enough and isn’t going to change. Even writing this PR didn’t require adding new parsing or formatting code. By contrast, #11082 not only adds new code right now to parse and generate bitcoin_rw.conf files, but will also require more work on an ongoing basis whenever the bitcoin.conf format is extended, because any new change will now require roundtrip support. (For example if we added an escaping mechanism it would require new code to escape values in addition to new code parsing escapes.)

This PR is also not mutually exclusive with #11082. I don’t know exactly what features you wanted to implement on top of #11082, but if you are thinking of new features different from #15936 and #13937 that call for injecting settings into a human-edited config file, nothing in this PR should get in the way of that.

There were some other issues I had with #11082 when I reviewed it (adding a new bitcoin.conf parser instead of extending the existing one, and concerns about init order and inconsistent interpretation of network sections in bitcoin.conf and bitcoin_rw.conf), but those issues were specific to the implementation and not inherent to the approach.

jonasschnelli commented at 10:58 am on May 18, 2019: contributor

Concept ACK.

Standard format. The bitcoin.conf format works pretty well as a manual configuration mechanism. But the format is unspecified and unstable, and less appealing for use as persistent key/value store compared to JSON which is well understood and supported by many tools and libraries.

I strongly agree with that.

luke-jr commented at 7:52 pm on May 18, 2019: member

INI is a standard format, supported by at least as many tools and libraries for longer. It has been working fine for bitcoin.conf and rwconf for years now.

JSON, on the other hand, is fragile, doesn’t allow users to add comments, and is currently supposed to only be used for the JSON-RPC/REST interfaces of Core.

The only thing to gain over the current INI format would be to avoid rewriting the entire file for changes. JSON doesn’t get us that (nor anything else).

jnewbery commented at 6:36 pm on July 2, 2019: member

Concept ACK

ajtowns commented at 3:45 am on July 3, 2019: member

Concept NACK from me too for much the same reasons @luke-jr gives. JSON’s fine for a human-readable wire format where programs in different languages need to exchange data (ie, RPC), but it’s not a great configuration language at all. I don’t think “the settings file is not supposed to replace the config file” will hold up – @jnewbery’s already suggesting this be used for user configuration in 16248, eg.

ryanofsky commented at 10:34 am on July 3, 2019: member

re: #15935 (comment)

I can make a more complete response later but I think the quote “the settings file is not supposed to replace the config file” is not being understood correctly. I’m using “config” there to refer to static configuration and “settings” to refer to dynamic configuration (based on bitcoin.conf, which is static, and QSettings, which is dynamic, in current code).

My thought is that more advanced command-line / linux type users will want to sit down and write out their configurations in text files with comments, but less advanced users will not be editing text files and will be happy if preferences they set through gui or rpc interfaces are just persisted in a simple format without support for comments. I can write more but @ajtowns, it would be helpful to me if you could say how you would want GUI and runtime settings to be stored ideally (for example, the list of wallets to load on startup).

ajtowns commented at 1:07 pm on July 3, 2019: member

@ryanofsky I think it would make sense to do any of:

just keep doing what we’re already doing
switch to a different machine-readable format that can do key-value store but isn’t tied to the gui (eg bdb, serialize.h, sqlite)
have a limited machine-updatable version of our human-readable config format (luke’s PR roughly)
switch to a new format (eg toml, yaml, json5/hjson/hocon, etc) for our human-readable static config, and use the same format (or a restricted version of it) for the machine-modifiable dynamic/persistent config

If we stuck with a primarily machine-readable format, having a “dumpsettings” / “loadsettings” RPC pair that translated that to and from the bitcoin.conf ini-format could be workable for people who want to play with persistent settings outside of the GUI (eg, playing with settings in the gui to get something that seems to work right, then wanting to copy some of them to bitcoind on a different computer).

I don’t think it’s realistic to treat json as a “machine-readable format” for case 2 because it’s both human readable and more powerful than our ini-format so it’s an obvious hammer to pick up for static configuration cases where the ini-format is awkward, cf #16248 (comment) and #16248 (comment) . Maybe we want to make bitcoin.conf json (or a supserset of json), but that ought to be a separate discussion.

Hope that makes sense

ryanofsky commented at 2:39 pm on July 3, 2019: member

@ajtowns, it sounds like your opinion is basically the same as Luke’s, and a concrete thing maybe we all would be happy with would be a PR that stores persistent GUI and application settings in an INI file instead of a JSON file.

If I am understanding correctly, you are not any citing direct advantages to the INI format for storing dynamic settings, but instead you’re making more of a slippery slope precautionary argument: that if we add a way to represent new settings in JSON form, it will encourage creation of ever more complex settings that can be conveniently represented as JSON lists and objects that will be easy to manipulate with GUIs and dynamic configuration, but will be impossible or awkward to represent in INI format. And therefore as a result, even though the settings.json file is intended to be only machine edited, users will end up modifying it anyway and wind up putting settings in a read-write file without comments, that would be more appropriately stored in a read-only file with comments. Let me know if I have this wrong, or if you do think INI format is a better format for dynamic configuration than the JSON format for other reasons that I have not understood.

But to follow up, would you be happy with a PR like this one puts settings in a bitcoin_rw.conf file with a big warning on top?

0# bitcoin_rw.conf: Dynamic settings for Bitcoin Core data directory
1#
2# Warning: DO NOT EDIT this file. Settings in this file can be overridden at
3# runtime and comments will be lost. Instead, to statically configure the
4# bitcoin node and wallet, create a bitcoin.conf file with the desired settings.
5
6proxy=host:123
7prune=2861
8wallet=wallet1
9wallet=wallet2

instead of the current settings.json file:

0{
1    "proxy": "host:123",
2    "prune": 2861,
3    "wallet": ["wallet1", "wallet2"]
4}

I will respond to other alternatives you suggested, but I think they are all have deficiencies

just keep doing what we’re already doing

The status quo is that we’re storing dynamic settings in the registry on windows, in in $HOME/.config/Bitcoin/Bitcoin-Qt.conf files on linux and $HOME/Library/Preferences plist files on Mac OS, and that dynamic settings are not tied to the datadir, and not shared between bitcoind and bitcoin-qt. I think this is unfortunate and want to fix this problem before adding support for more dynamic settings.

switch to a different machine-readable format that can do key-value store but isn’t tied to the gui (eg bdb, serialize.h, sqlite)

I’m not sure what advantages you see in this approach compared to the current PR, other than that the settings files would be less accessible, so advanced users would be more encouraged to create static configurations where appropriate. I don’t mean to be unfair, but I think we could get the same benefit with less code by just tweaking the current PR and having it save settings.json.gz or settings.json.rot13 files that would also be a pain in the ass to edit.

have a limited machine-updatable version of our human-readable config format (luke’s PR roughly)

I do think our current format is pretty terrible (stupid about bool, and lists, can’t represent strings containing #, etc, see previous comments) and I have other specific issues with Luke’s PR (again see previous comments). But I could live with this approach and I will attempt to implement it if this PR is dead or going nowhere.

switch to a new format (eg toml, yaml, json5/hjson/hocon, etc) for our human-readable static config, and use the same format (or a restricted version of it) for the machine-modifiable dynamic/persistent config

As far as I can see (please correct me if I’m mistaken) all of these formats have 0 advantages over plain JSON for dynamic configuration, and very few advantages over current INI format for static configuration, and would trigger lots of bikeshedding, and require new parsing code. I think the major advantage of this over PR all your suggested alternatives but especially this one, is that it’s ridiculously simple and requires no new parsing code at all: 70675c3e4975203ad6222ba2b00c83b4e4213793

ajtowns commented at 4:06 pm on July 3, 2019: member

@ajtowns, it sounds like your opinion is basically the same as Luke’s, and a concrete thing maybe we all would be happy with would be a PR that stores persistent GUI and application settings in an INI file instead of a JSON file.

Yeah; the benefit of that approach that the json one doesn’t have is that you can just cut and paste lines that the gui has created into your permanent bitcoin.conf directly.

But to follow up, would you be happy with a PR like this one puts settings in a bitcoin_rw.conf file with a big warning on top?

I think a warning makes sense.

The status quo is that we’re storing dynamic settings in the registry on windows, in in $HOME/.config/Bitcoin/Bitcoin-Qt.conf files on linux and $HOME/Library/Preferences plist files on Mac OS, and that dynamic settings are not tied to the datadir, and not shared between bitcoind and bitcoin-qt. I think this is unfortunate and want to fix this problem before adding support for more dynamic settings.

A different approach would be to say “bitcoin-qt has dynamic settings – it has a GUI that lets you do dynamic things” and “bitcoind does not have dynamic settings”, and decide that’s the way it’s meant to be. Not having dynamic settings means that if you “shutdown and restart” you’re back to a clean slate which can be good, and could be appropriate for a gui-less daemon. If so, it could just be enough to have some way of viewing/exporting the current dynamic settings from the gui, so you can paste them into your bitcoin.conf. Not really advocating this, but I think it’s actually workable.

switch to a different machine-readable format that can do key-value store but isn’t tied to the gui (eg bdb, serialize.h, sqlite)

I’m not sure what advantages you see in this approach compared to the current PR, other than that the settings files would be less accessible, so advanced users would be more encouraged to create static configurations where appropriate.

It’s more that if you’ve got “settings.json” or “bitcoin_rw.conf” sitting around, people are going to go “ah, great, I see what’s going on” and edit it. If they see an sqlite file, they might fire up sqlite, but it’ll have a schema so they’ll only be able to touch it in fairly sensible ways. If they see a serialize.h or bdb file, they probably won’t touch it, but if they do it’ll be using tools that will probably get it right and likely won’t be surprised if things break. With a .json (or .json.gz or .ini) they’re going to go “oh cool! look what i found!” and fire up notepad and mess around, and you need to give detailed error messages to help them find their mistakes and manage expectations so they’re not terribly surprised when their formatting/comments/ordering/additions disappear or otherwise get mangled.

Using sqlite would get the “avoid rewriting the entire file for changes” benefit for whatever that’s worth.

have a limited machine-updatable version of our human-readable config format (luke’s PR roughly)

I do think our current format is pretty terrible (stupid about bool, and lists, can’t represent strings containing #, etc, see previous comments)

I don’t disagree, but if that’s the problem, let’s actually work on it and improve the actual config format. But:

As far as I can see (please correct me if I’m mistaken) all of these formats [toml, yaml, json5/hjson/hocon, etc] have 0 advantages over plain JSON for dynamic configuration, and very few advantages over current INI format for static configuration, and would trigger lots of bikeshedding, and require new parsing code.

They’d all also require some special handling for upgrading from today’s bitcoin.conf to whatever the future one is, which seems like a pain too. But if the conclusion there is “our custom ini stuff is what’s best for bitcoin.conf” then I don’t think it makes sense to complain about how terrible our current format is, if it’s still the best we can do.

and I have other specific issues with Luke’s PR (again see previous comments)

Sure. I suppose I’m obliged to review that PR now too…

I think the major advantage of this over PR all your suggested alternatives but especially this one, is that it’s ridiculously simple and requires no new parsing code at all

If it’s the wrong thing, it doesn’t really matter how easy it is…

jnewbery commented at 9:11 am on July 10, 2019: member

But I could live with this approach and I will attempt to implement it if this PR is dead or going nowhere.

Note that this PR was discussed in an IRC meeting here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-01-10-19.00.log.html#l-175

Several contributors there were in favour of this approach because it’s less code (@jonasschnelli , @jimpo , @jamesob , @laanwj ). As far as I’m concerned that means this approach still has far more concept ACKs than NACKs (but any of those contributors should speak up if they’ve changed their minds).

ryanofsky commented at 11:24 am on July 10, 2019: member

Note this PR didn’t exist yet at the time of the IRC discussion. In any case, I tend to be more interested in reasoning behind NACKs, and how specific concerns can be addressed, than just their gut level responses. For example, Luke seemed to be concerned with comments and comment preservation in the dynamic configuration file, which could be addressed with code to read/write the dynamic configuration in ini format instead of json format. AJ seemed skeptical of dynamic configuration for bitcoind in general, and I definitely think there should be a way to turn off dynamic configuration (gray out dynamic configuration in gui, return errors from RPCs that would try to save a persistent setting).

luke-jr commented at 11:30 am on July 10, 2019: member

Several contributors there were in favour of this approach because it’s less code

But it’s not really. UniValue is more code than updating the INI file. And until now, we have had a policy (that IMO we should keep) to only use UniValue in the JSON-RPC code.

ryanofsky commented at 12:02 pm on July 10, 2019: member

we have had a policy (that IMO we should keep) to only use UniValue in the JSON-RPC code @luke-jr, I think I’ve heard of this policy before, but maybe didn’t take it seriously. Does it come from an objection to the JSON format, or problems with the univalue implementation, or something else? I wouldn’t want univalue to used in application code (in wallet code or consensus code) because string lookups and variant typing lead to c++ code that’s verbose, messy, and fragile. But at an API layer, or for accessing config data, univalue seems pretty well-suited, so I wonder what the concerns about it are or were.

practicalswift commented at 3:58 pm on July 10, 2019: contributor

Concept ACK

Rationale:

Standard format. The bitcoin.conf format works pretty well as a manual configuration mechanism. But the format is unspecified and unstable, and less appealing for use as persistent key/value store compared to JSON which is well understood and supported by many tools and libraries.

NicolasDorier commented at 3:55 am on July 26, 2019: contributor

I think bitcoin.conf is easier to use than a json file by scripting languages. It is easy to add one line to bitcoin.conf, but adding one node to a json file? It requires additional dependencies.

ryanofsky commented at 4:42 am on July 26, 2019: member

@NicolasDorier, can you clarify your use case? settings.json does not replace bitcoin.conf and should not be used for static configuration. It replaces QSettings (see #15936) and is used for dynamic configuration. It’s database of settings updated dynamically while bitcoin is running, similar to peers.dat or fee_estimates.dat, only it happens to be in a human readable format.

NicolasDorier commented at 7:56 am on July 26, 2019: contributor

@ryanofsky ah ok then this make sense. I was worried that some configuration thing start becoming spread in different files in different format, but this is not the case here.

luke-jr commented at 12:43 pm on July 26, 2019: member

Software should be modifying bitcoin_rw.conf/settings.json, not bitcoin.conf

ryanofsky commented at 5:23 pm on July 26, 2019: member

Software should be modifying bitcoin_rw.conf/settings.json, not bitcoin.conf

It makes sense for deployment software to generate and modify bitcoin.conf files. Specifically configuration software like ansible, package managers like nix or guix, build scripts like docker files, should all be setting up their deployments through bitcoin.conf. Managing static software configurations is a big part of what these tools do and why they exist.

For bitcoin_rw.conf/settings.json, there are some exceptions I could think of, but mainly I think the only tool that will want to write these files is bitcoin itself. Partly this is due to safety, because with features like #15936 and #15937, settings can change any time while bitcoin is running, so external programs trying to modify these files could see their updates lost. But more importantly, a major motivation for wanting to add dynamic settings is to provide new, less cumbersome ways of updating settings than the raw file system. A user is less likely to screw up something like pruning when the way to enable it is to type a command like bitcoin-cli config prune 3gb, and get instant error checking and feedback, than to have to locate their datadir, open a text editor, make changes without running afoul of ini sections syntax and precedence rules, stop bitcoin, restart bitcoin, and check the logs to verify their changes picked up.

Bitcoin isn’t pure server software that only needs static configuration, and it isn’t pure application software that only needs dynamic configuration. It’s P2P software that should support both static and dynamic configuration for different use cases. My expectation is that server admins and advanced users will use bitcoin.conf just like now and always, and for a lot of them to disable dynamic configuration. I would expect more typical users to have no bitcoin.conf file, or only a minimal one from their distro or package manager, and to manage settings through GUI and command interfaces without editing text files.

laanwj added the label Feature on Sep 30, 2019

laanwj referenced this in commit a7aec7ad97 on Nov 8, 2019

sidhujag referenced this in commit 16a52029e7 on Nov 9, 2019

jnewbery commented at 11:06 pm on November 11, 2019: member

The base PR #15934 is merged. Looking forward to reviewing this once it’s rebased!

ryanofsky force-pushed on Dec 3, 2019

hebasto commented at 9:14 am on January 13, 2020: member

Concept ACK.

Standard format. The bitcoin.conf format works pretty well as a manual configuration mechanism. But the format is unspecified and unstable, and less appealing for use as persistent key/value store compared to JSON which is well understood and supported by many tools and libraries.

There are many JSON standards (e.g., ISO/IEC 21778, ECMA-404). Could we point the one which UniValue complies to?

jonasschnelli commented at 7:28 am on May 29, 2020: contributor

I think this PR goes into the right direction and it has a few concept ACKs. @ryanofsky: can you take it out of the draft mode to get final reviews?

ryanofsky renamed this:
~~WIP: Add <datadir>/settings.json persistent settings storage~~
Add <datadir>/settings.json persistent settings storage
on Jun 4, 2020

ryanofsky force-pushed on Jun 4, 2020

ryanofsky marked this as ready for review on Jun 4, 2020

in src/util/system.cpp:401 in 6c6846cd58 outdated

396+    file.open(filepath);
397+    if (!file.is_open()) return true; // Ok for file not to exist.
398+
399+    util::SettingsValue in;
400+    if (!in.read(std::string{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()})) {
401+        LogPrintf("Error: Unable to parse settings file %s\n", filepath.string());

jonasschnelli commented at 7:40 am on June 5, 2020:

These logprints are currently going nowhere because InitLogging() is called after ReadSettingsFile(). Not saying it is a problem.

ryanofsky commented at 10:21 pm on June 5, 2020:

re: #15935 (review)

These logprints are currently going nowhere because InitLogging() is called after ReadSettingsFile(). Not saying it is a problem.

Good catch, errors are returned now

in src/util/system.cpp:425 in 6c6846cd58 outdated

420+        m_settings.rw_settings.emplace(keys[i], values[i]);
421+    }
422+    return true;
423+}
424+
425+bool ArgsManager::WriteSettingsFile() const

jonasschnelli commented at 7:41 am on June 5, 2020:

Without further PRs, this part is dead code,… would it at least make sense to add a test for it?

ryanofsky commented at 10:21 pm on June 5, 2020:

re: #15935 (review)

Without further PRs, this part is dead code,… would it at least make sense to add a test for it?

Added a write call and tests

jonasschnelli commented at 7:43 am on June 5, 2020: contributor

Reviewed the code and it looks good. To merge this on its own, I think it would require some tests and, because this is read only currently, it should be mentioned in the init.cpp help as well as in the files.md (where the read-only note can be removed once something like #15936 is merged).

ryanofsky force-pushed on Jun 5, 2020

ryanofsky commented at 10:25 pm on June 5, 2020: member

re: jonasschnelli #15935 (comment)

Added some tests and a write call

Rebased 70675c3e4975203ad6222ba2b00c83b4e4213793 -> 5873a9c6ce1212d351de48ba1bdce68e8ee143ca (pr/rwset.5 -> pr/rwset.6, compare) after #15934 merge Rebased 5873a9c6ce1212d351de48ba1bdce68e8ee143ca -> 6c6846cd58e33108887cea4e2a1127a56e8c7d67 (pr/rwset.6 -> pr/rwset.7, compare) due to conflicts with #18391 and #17473 Updated 6c6846cd58e33108887cea4e2a1127a56e8c7d67 -> 14849d8b245f47bb844e3173dc7ce209e8010ebf (pr/rwset.7 -> pr/rwset.8, compare) with suggested changes Updated 14849d8b245f47bb844e3173dc7ce209e8010ebf -> eb966f421c9365e232627209687e6506089deb34 (pr/rwset.8 -> pr/rwset.9, compare) to fix python lint errors https://travis-ci.org/github/bitcoin/bitcoin/jobs/695231597 Updated eb966f421c9365e232627209687e6506089deb34 -> 8f099645ac8308bdb8f9f5d86d25358e6422c638 (pr/rwset.9 -> pr/rwset.10, compare) with various cleanups Rebased 8f099645ac8308bdb8f9f5d86d25358e6422c638 -> 81c2e91ca90ba11bdd15f7fca871683a39636f35 (pr/rwset.10 -> pr/rwset.11, compare) due to silent conflict with #19176 and with some code style cleanups

ryanofsky force-pushed on Jun 5, 2020

ryanofsky force-pushed on Jun 11, 2020

in src/init.cpp:417 in 81c2e91ca9 outdated

413@@ -414,6 +414,7 @@ void SetupServerArgs(NodeContext& node)
414             "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
415     gArgs.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
416     gArgs.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
417+    gArgs.AddArg("-settings=<file>", strprintf("Specify path to read-write settings file for storage of persistent settings. Can be disabled with -nosettings. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

achow101 commented at 4:53 pm on June 11, 2020:

I think this should mention that the file is not to be manually created or modified.

ryanofsky commented at 8:21 pm on June 16, 2020:

re: #15935 (review)

I think this should mention that the file is not to be manually created or modified.

Mentioned with pointer to bitcoin.conf instead

achow101 commented at 5:00 pm on June 11, 2020: member

ACK 81c2e91ca90ba11bdd15f7fca871683a39636f35

While this isn’t mutually exclusive with #11082, it’s much easier to read and review.

ryanofsky force-pushed on Jun 16, 2020

ryanofsky commented at 10:12 pm on June 16, 2020: member

Updated 81c2e91ca90ba11bdd15f7fca871683a39636f35 -> 15b93eeea3e7077f3a8151ac7ec15c0bb07cb3a7 (pr/rwset.11 -> pr/rwset.12, compare) with suggested change

achow101 commented at 4:34 pm on June 18, 2020: member

ACK 15b93eeea3e7077f3a8151ac7ec15c0bb07cb3a7

DrahtBot added the label Needs rebase on Jun 21, 2020

jonatack commented at 5:15 pm on June 21, 2020: member

Concept ACK

ryanofsky force-pushed on Jun 25, 2020

ryanofsky commented at 9:23 pm on June 25, 2020: member

Rebased 15b93eeea3e7077f3a8151ac7ec15c0bb07cb3a7 -> 6ec69d5e6573b620a7c7a5a9ab39a71061a063f1 (pr/rwset.12 -> pr/rwset.13, compare) due to conflict with #19200

DrahtBot removed the label Needs rebase on Jun 25, 2020

achow101 commented at 3:46 pm on June 29, 2020: member

ACK 6ec69d5e6573b620a7c7a5a9ab39a71061a063f1

DrahtBot added the label Needs rebase on Jul 11, 2020

meshcollider commented at 8:48 am on July 11, 2020: contributor

Concept & utACK 6ec69d5e6573b620a7c7a5a9ab39a71061a063f1

It would be good to get this in soon, it does seem that there are many more concept ACKs than arguments against.

util: Add ReadSettings and WriteSettings functions

Currently unused, but includes tests.

eb682c5700

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

Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to
the loadwallet RPC and maintain a dynamic list of wallets that should be loaded
on startup that also can be shared between bitcoind and bitcoin-qt.

9c69cfe4c5

ryanofsky force-pushed on Jul 11, 2020

ryanofsky commented at 10:21 am on July 11, 2020: member

Rebased 6ec69d5e6573b620a7c7a5a9ab39a71061a063f1 -> dfe59647232a53011d6831d246bd3a01fffa96b8 (pr/rwset.13 -> pr/rwset.14, compare) due to conflict with #19474

DrahtBot removed the label Needs rebase on Jul 11, 2020

luke-jr commented at 10:28 pm on July 11, 2020: member

The many reasons for rejecting it remain…

Adds a THIRD config system for no reason (Note that QSettings can never be fully removed)
Difficult for users to edit the new config file (JSON doesn’t support comments, even if prettified)
Impossible to edit externally while the node is running (any changes would just get written over)
Adds complexity for developers (where to put each new setting? multiple systems??)
Better alternative already exists and is well-tested (in Knots) for years (#11082, rwconf)
Introduces UniValue into non-RPC code modules

in doc/files.md:53 in dfe5964723 outdated

49@@ -50,14 +50,15 @@ Subdirectory       | File(s)               | Description
50 `indexes/blockfilter/basic/`    | `fltrNNNNN.dat`<sup>[\[2\]](#note2)</sup> | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic`
51 `wallets/`         |                       | [Contains wallets](#multi-wallet-environment); can be specified by `-walletdir` option; if `wallets/` subdirectory does not exist, a wallet resides in the data directory
52 `./`               | `banlist.dat`         | Stores the IPs/subnets of banned nodes
53-`./`               | `bitcoin.conf`        | Contains [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`; can be specified by `-conf` option
54+`./`               | `bitcoin.conf`        | User-defined [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`. File is read-only and must be created manually. Path can be specified by `-conf` option

jnewbery commented at 9:18 am on July 12, 2020:

nit: I think this would be clearer as “File is not written to by the software and must be created manually.

ryanofsky commented at 12:23 pm on July 13, 2020:

re: #15935 (review)

nit: I think this would be clearer as “File is not written to by the software and must be created manually.

Thanks, used your suggestion

in src/util/settings.cpp:37 in dfe5964723 outdated

33@@ -32,6 +34,10 @@ static void MergeSettings(const Settings& settings, const std::string& section,
34     if (auto* values = FindKey(settings.command_line_options, name)) {
35         fn(SettingsSpan(*values), Source::COMMAND_LINE);
36     }
37+    // Merge in the read-write settings

jnewbery commented at 9:19 am on July 12, 2020:

nit: the comment above (“Forced config > command line > …”) should be updated.

ryanofsky commented at 12:23 pm on July 13, 2020:

re: #15935 (review)

nit: the comment above (“Forced config > command line > …”) should be updated.

Thanks, updated

jnewbery commented at 9:27 am on July 12, 2020: member

Concept and code review ACK dfe59647232a53011d6831d246bd3a01fffa96b8

I’ve left a couple of nits on docs/comments, which can easily be done in a follow-up.

ryanofsky force-pushed on Jul 13, 2020

ryanofsky commented at 12:40 pm on July 13, 2020: member

Updated dfe59647232a53011d6831d246bd3a01fffa96b8 -> 7b129c849b4e3dae1e6f54c8d395f57a2c248388 (pr/rwset.14 -> pr/rwset.15, compare) with suggestions

jnewbery commented at 1:07 pm on July 13, 2020: member

reACK 7b129c849b4e3dae1e6f54c8d395f57a2c248388

ryanofsky commented at 2:02 pm on July 13, 2020: member

re: #15935 (comment)

Adds a THIRD config system for no reason

This PR allows #15936 to completely replace QSettings as a source of bitcoin node and wallet settings. There are three sources of node and wallet settings currently (runtime command line settings, stored QSettings, and static bitcoin.conf settings), and there will be 3 sources after this PR and #15936.

(Note that QSettings can never be fully removed)

Not sure why it couldn’t be removed, but I think QSettings is a reasonable place to store gui specific settings like window positions, and to have a datadir override. I think we all agree QSettings is not a good place to store node or wallet settings, though.

Difficult for users to edit the new config file (JSON doesn’t support comments, even if prettified)

I think this is an advantage, not a disadvantage, but I’d be curious if there are specific use-cases where it is a disadvantage.

I think current bitcoin.conf file is good place for static settings. These settings can be provided by distros and packages, and created by more advanced users who are comfortable creating text files, learning a syntax, writing comments, and may want to integrate with their bitcoin configs with version control systems like git, or configuration management systems like ansible.

The settings.json file is not intended to be another source of static settings. It is intended to be a storage location for dynamic settings. I don’t think users should have to edit text files and start and stop their nodes just so they they can save few settings. They should be able to update settings with real-time error checking using simple GUI and RPC interfaces. I think it’s good that the saved settings format doesn’t encourage comments or manual editing, because comments for dynamic settings will get outdated as settings are updated, and we don’t need an alternative to bitcoin.conf for manual editing.

Impossible to edit externally while the node is running (any changes would just get written over)

This is not the case in this PR which just loads and saves settings on startup. #15937 does overwrite settings, though. I think this is fine, but if a different behavior like merging settings is more appropriate, it’d be easy to implement there. Again I think it would be useful to know some specific use cases where this would be a good idea.

Adds complexity for developers (where to put each new setting? multiple systems??)

Settings from command line, bitcoin.conf, and settings.json are all merged so I don’t think there is a concern about where to put settings. Unclear what the specific concern or demand is here.

Better alternative already exists and is well-tested (in Knots) for years (#11082, rwconf)

I reviewed #11082 in detail (https://github.com/bitcoin/bitcoin/pull/11082#pullrequestreview-186204085) and wrote this PR to be a more robust alternative. I think knots can pretty easily stay backwards compatible by loading the ini file and calling WriteSettings, if that’s the specific concern.

Introduces UniValue into non-RPC code modules

Again I see this as an advantage not a disadvantage. If there is a new settings source, there has to be some new API for reading and writing settings. UniValue for all its quirks is a reasonable way of passing settings values around a simple API. It’s easier to use, higher level, and more flexible than the new api from #11082:

0  void ModifyRWConfigFile(const std::map<std::string, std::string>& settings_to_change);
1  void ModifyRWConfigFile(const std::string& setting_to_change, const std::string& new_value);
2  void EraseRWConfigFile();

UniValue is also familiar and already used in thousands of lines of bitcoin code.

Another nice side effect of using UniValue is that it avoids the need to add new parsing code, partially duplicating existing parsing code that will need to be kept in sync. I also think it’s possible that as well tested as the #11082 new parsing code may be, UniValue probably has better test coverage and has been through more testing for correctness and vulnerabilities at this point, like the test suite I added https://github.com/jgarzik/univalue/pull/30.

UniValue also has support for list settings, which the #11082 API does not support. And it does not mangle strings. It has no problem with # or \n characters.

in src/util/settings.cpp:80 in ced5588a78 outdated

75+        errors.emplace_back(strprintf("Found non-object value %s in settings file %s", in.write(), path.string()));
76+        return false;
77+    }
78+
79+    const std::vector<std::string>& in_keys = in.getKeys();
80+    const std::vector<UniValue>& in_values = in.getValues();

hebasto commented at 1:13 pm on July 19, 2020:

ced5588a7815de2d1b2e08ebd9979fa29ac0d3f2

0    const std::vector<SettingsValue>& in_values = in.getValues();

ryanofsky commented at 12:49 pm on July 21, 2020:

re: #15935 (review)

ced5588

Thanks, updated

in doc/files.md:53 in 7b129c849b outdated

49@@ -50,14 +50,15 @@ Subdirectory       | File(s)               | Description
50 `indexes/blockfilter/basic/`    | `fltrNNNNN.dat`<sup>[\[2\]](#note2)</sup> | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic`
51 `wallets/`         |                       | [Contains wallets](#multi-wallet-environment); can be specified by `-walletdir` option; if `wallets/` subdirectory does not exist, a wallet resides in the data directory
52 `./`               | `banlist.dat`         | Stores the IPs/subnets of banned nodes
53-`./`               | `bitcoin.conf`        | Contains [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`; can be specified by `-conf` option
54+`./`               | `bitcoin.conf`        | User-defined [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`. File is not written to by the software and must be created manually. Path can be specified by `-conf` option

hebasto commented at 1:26 pm on July 19, 2020:

0`./`               | `bitcoin.conf`        | User-defined read-only [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`. File must be created manually. Path can be specified by `-conf` option

ryanofsky commented at 12:50 pm on July 21, 2020:

re: #15935 (review)

This is different than John’s suggestion #15935 (review), and maybe is stale. Can follow up with new suggestion or give a rationale if a change is needed here.

in doc/files.md:61 in 7b129c849b outdated

57 `./`               | `fee_estimates.dat`   | Stores statistics used to estimate minimum transaction fees and priorities required for confirmation
58 `./`               | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used
59 `./`               | `mempool.dat`         | Dump of the mempool's transactions
60 `./`               | `onion_private_key`   | Cached Tor hidden service private key for `-listenonion` option
61 `./`               | `peers.dat`           | Peer IP address database (custom format)
62+`./`               | `settings.json`        | Read-write settings set though GUI or RPC interfaces, augmenting manual settings from bitcoin.conf. File is created automatically if read-write setting storage is not disabled with `-nosettings` option. Path can be specified with `-settings` option

hebasto commented at 1:28 pm on July 19, 2020:

0`./`               | `settings.json`       | Read-write settings set through GUI or RPC interfaces, augmenting manual settings from [bitcoin.conf](bitcoin-conf.md). File is created automatically if read-write settings storage is not disabled with `-nosettings` option. Path can be specified with `-settings` option

ryanofsky commented at 12:50 pm on July 21, 2020:

re: #15935 (review)

Thanks, updated

in src/init.cpp:400 in 7b129c849b outdated

396@@ -397,7 +397,7 @@ void SetupServerArgs(NodeContext& node)
397 #endif
398     gArgs.AddArg("-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
399     gArgs.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
400-    gArgs.AddArg("-conf=<file>", strprintf("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
401+    gArgs.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

hebasto commented at 1:30 pm on July 19, 2020:

0    gArgs.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative path will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

ryanofsky commented at 12:50 pm on July 21, 2020:

re: #15935 (review)

Not changing “paths” to “path” because it would be an unrelated change and inconsistent with other entries here. I think it may also be less grammatical.

in src/init.cpp:421 in 7b129c849b outdated

417@@ -418,6 +418,7 @@ void SetupServerArgs(NodeContext& node)
418             "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
419     gArgs.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
420     gArgs.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
421+    gArgs.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

hebasto commented at 1:45 pm on July 19, 2020:

0    gArgs.AddArg("-settings=<file>", strprintf("Specify path to read-write settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by user (use %s instead for custom settings). Relative path will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

ryanofsky commented at 12:50 pm on July 21, 2020:

re: #15935 (review)

Could change “users” to “user” but this seems slightly worse

in src/util/system.h:369 in 7b129c849b outdated

364+    void LockSettings(Fn&& fn)
365+    {
366+        LOCK(cs_args);
367+        fn(m_settings);
368+    }
369+

hebasto commented at 1:55 pm on July 19, 2020:

This function template is not used in this PR.

ryanofsky commented at 12:51 pm on July 21, 2020:

re: #15935 (review)

This function template is not used in this PR.

Right, it is used by #15936 and #15937

hebasto approved

hebasto commented at 2:09 pm on July 19, 2020: member

ACK 7b129c849b4e3dae1e6f54c8d395f57a2c248388, tested on Linux Mint 20.

The bitcoin-conf.md should be also updated (possibly in a follow-up pr).

ryanofsky force-pushed on Jul 21, 2020

ryanofsky commented at 1:20 pm on July 21, 2020: member

Thanks for review!

Updated 7b129c849b4e3dae1e6f54c8d395f57a2c248388 -> b25ed47202a3c2f47f1c35629901ac9afc7453a1 (pr/rwset.15 -> pr/rwset.16, compare) with suggestions

jnewbery commented at 1:30 pm on July 21, 2020: member

utACK b25ed47202a3c2f47f1c35629901ac9afc7453a1

It feels like this is ready for merge if @hebasto @achow101 and @meshcollider re-ACKed

hebasto approved

hebasto commented at 1:41 pm on July 21, 2020: member

re-ACK b25ed47202a3c2f47f1c35629901ac9afc7453a1, only suggested changes since the previous review.

achow101 commented at 4:17 pm on July 21, 2020: member

re-ACK b25ed47202a3c2f47f1c35629901ac9afc7453a1

MarcoFalke removed the label Build system on Jul 22, 2020

MarcoFalke removed the label Feature on Jul 22, 2020

MarcoFalke removed the label GUI on Jul 22, 2020

MarcoFalke removed the label Tests on Jul 22, 2020

in src/util/settings.cpp:77 in b25ed47202 outdated

72+
73+    if (file.fail()) {
74+        errors.emplace_back(strprintf("Failed reading settings file %s", path.string()));
75+        return false;
76+    }
77+    file.close();

MarcoFalke commented at 8:20 am on July 22, 2020:

why? Either this is redundant because this is already called when file goes out of scope, or it should be called before file.fail() (in cause you want to check for some kind of failure)

ryanofsky commented at 1:12 pm on July 22, 2020:

re: #15935 (review)

why? Either this is redundant because this is already called when file goes out of scope, or it should be called before file.fail() (in cause you want to check for some kind of failure)

Just freeing a resource that’s no longer needed, added comment

in src/util/settings.h:29 in b25ed47202 outdated

25@@ -24,19 +26,31 @@ namespace util {
26 //!       https://github.com/bitcoin/bitcoin/pull/15934/files#r337691812)
27 using SettingsValue = UniValue;
28 
29-//! Stored bitcoin settings. This struct combines settings from the command line
30-//! and a read-only configuration file.
31+//! Stored bitcoin settings. This struct combines settings from the command line,

MarcoFalke commented at 8:21 am on July 22, 2020:

style-nit: Should say PACKAGE_NAME, but that seems a bit too verbose. Might as well remove

0//! Stored settings. This struct combines settings from the command line,

ryanofsky commented at 1:12 pm on July 22, 2020:

re: #15935 (review)

style-nit: Should say PACKAGE_NAME, but that seems a bit too verbose. Might as well remove

Thanks, updated

in src/util/system.cpp:412 in b25ed47202 outdated

407+{
408+    for (const auto& error : errors) {
409+        if (error_out) {
410+            error_out->emplace_back(error);
411+        } else {
412+            LogPrintf("%s\n", error);

MarcoFalke commented at 8:23 am on July 22, 2020:

any reason to add dead code?

MarcoFalke commented at 10:59 am on July 22, 2020:

See https://drahtbot.github.io/reports/coverage/bitcoin/bitcoin/15935/total.coverage/src/util/system.cpp.gcov.html#412

My preference would be to not pass the erros as ptr, but as reference

MarcoFalke commented at 11:03 am on July 22, 2020:

Also, as an idea to add test coverage in the future, the failure cases that are not logic errors could be tested in the two touched files. https://drahtbot.github.io/reports/coverage/bitcoin/bitcoin/15935/total.coverage/src/util/settings.cpp.gcov.html

ryanofsky commented at 1:12 pm on July 22, 2020:

re: #15935 (review)

any reason to add dead code?

This is used in #15936 and #15937. Added test to quell LCOV.

MarcoFalke approved

MarcoFalke commented at 8:33 am on July 22, 2020: member

Approach ACK b25ed47202a3c2f47f1c35629901ac9afc7453a1 (style nits only)

It looks like this file is append-only, with the effect that deprecated or renamed settings will stay there forever

ryanofsky force-pushed on Jul 22, 2020

ryanofsky commented at 1:21 pm on July 22, 2020: member

Thanks!

Updated b25ed47202a3c2f47f1c35629901ac9afc7453a1 -> 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 (pr/rwset.16 -> pr/rwset.17, compare) with suggested improvements

re: #15935#pullrequestreview-453097605

It looks like this file is append-only, with the effect that deprecated or renamed settings will stay there forever

That’s not the intention. If or when settings are deprecated, future versions of the software can remove them. This version of the software will not remove settings before they are deprecated or remove unrecognized settings. If there’s a use-case for removing unrecognized settings in this PR, I’d be curious to hear what it is.

MarcoFalke commented at 6:06 am on July 23, 2020: member

Sorry for causing another force push. Only changes were comment and test related.

Approach re-ACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 🌾

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Approach re-ACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 🌾
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUib/Av/Xc5gqDLuaziBgKQFpjDzCaVlaBH+BI5UjULzIL6ql9F0kCXzwHU1l/2P
 84yFXMCzjjUUR2w6Qb/NJzVIYJYFoyU4kDu5E01nuMu/pvtfqnQ20/Zc2GTo5Sqd3
 99X5DN5xFGU6nUZmmWfsTriQzugrjaYkYPONzk4LyaIpd8i+iZnPf6Tfyu46XnTv6
10r+vyxGMhRdOJpl0lzQPnVAnBpHGnxKfv268edwgYmBJxRXKsMCYzk0nrIO8bBbwm
11hDMhMqf1pvwvehM6OURESXk6o+TeGqJ37sXxJBHO8TrXUzOfmmvmET4z3JNV5qCx
124LqTkKhPSWQ/DjR7h8VCFkijfGadVbIPGztqeBpLnVORdaBvWwv6oRvLWLxl0f7Y
13Vv9LD6bW6jTFum+ALVO4mUW9E0CzpN7zTQ0OERTXZXWw8fHW5+le+CFNmIRjVFOO
14p4dvMwlFOW0ECMx06LuLOg00D7Eec59MRk30rG6yposECwthJd3eDX7in/tUWI1A
15e4MKLdVZ
16=dJtQ
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6253e88b8270f1f8ad8dbba3415a37830e223905d1d01c5eb3c11cee6b400cfb -

jnewbery commented at 10:17 am on July 23, 2020: member

utACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8

Only changes are comments and an additional test file.

Now that we have multiple ACKs, can we save style nits for a follow-up please?

MarcoFalke merged this on Jul 23, 2020

MarcoFalke closed this on Jul 23, 2020

MarcoFalke commented at 6:26 pm on July 23, 2020: member

If there’s a use-case for removing unrecognized settings in this PR, I’d be curious to hear what it is

The same reason that unrecognized settings from the config file or command line are rejected

ryanofsky commented at 2:27 pm on July 24, 2020: member

re: #15935#pullrequestreview-453097605

It looks like this file is append-only, with the effect that deprecated or renamed settings will stay there forever

re: #15935 (comment)

If there’s a use-case for removing unrecognized settings in this PR, I’d be curious to hear what it is

The same reason that unrecognized settings from the config file or command line are rejected

Thanks for clarifying. I thought you were asking to remove unrecognized settings, not reject them with a fatal error. I would like it to be possible to define settings that will be rejected as errors (see -require idea below), but I don’t think every unrecognized setting should be rejected as an error. Doing this would make any datadir with newer settings unloadable with older versions of bitcoin. There would be some justification for this kind of strictness with bitcoin.conf settings (we are less strict currently, just warning to debug.log), but it would be bad to be this strict with settings.json because unlike bitcoin.conf, settings.json isn’t created or meant to be edited by users, so it should be able to handle version upgrades and downgrades smoothly without manual intervention.

That said, I think there are some possible future improvments:

It would be good to log Warning: unrecognized setting 'foo' in settings.json is being ignored or similar debug messages to make it easier to notice unrecognized settings even if they will not be fatal errors.
It would be good to add a -require=<feature> command-line / bitcoin.conf / settings.json option to support cases where users or developers really do want to prevent a particular datadir or configuration being loaded by a bitcoind that is too old or was built with features disabled. Useful feature values could be -require=wallet to cause an init error if bitcoin was built without wallet support, -require=zmq to error if there’s no zmq support, -require=wallet-compatible-db to error if bitcoin doesn’t support writing bdb 4.8 wallets, -require=wallet-sqlite to error if it doesn’t support reading sqlite wallets, or various other feature strings. Other feature strings that might make sense (I don’t know) could be things like -require=addrv2 or -require=wtxid-relay or -require=taproot or -require=muhash, to refuse bitcoind startup if the binary doesn’t support these things.
If or when individual bitcoin settings are deprecated and removed in the future, it would probably be good write code to strip them from settings.json so they don’t stick around forever.

sidhujag referenced this in commit bcd2ed79e3 on Jul 24, 2020

MarcoFalke commented at 8:53 am on July 30, 2020: member

Addressed my feedback in #19624

jasonbcox referenced this in commit 34bd94378a on Oct 20, 2020

jasonbcox referenced this in commit 4089671a9e on Oct 21, 2020

sidhujag referenced this in commit c1677b430c on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

Add /settings.json persistent settings storage #15935

Conflicts

Coverage