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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: Pass ArgsManager into functions that register args #19561 (refactor: Pass ArgsManager into functions that register args by S3RK)
• util: Make default arg values more specific #19471 (util: Make default arg values more specific by hebasto)
• multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
• util: Set safe permissions for data directory and wallets/ subdir #17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
• Add loadwallet and createwallet load_on_startup options #15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
• interfaces: Expose settings.json methods to GUI #15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
• Add new bitcoin_rw.conf file that is used for settings modified by this software itself #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

^{Updated at: 2020-07-22T10:37:55.716811.}

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

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 0ab41dd from #15936 how this works in the GUI or in 04c80c4 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, Add new bitcoin_rw.conf file that is used for settings modified by this software itself #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.

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.

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).

Concept ACK

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.

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).

@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

@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?

# bitcoin_rw.conf: Dynamic settings for Bitcoin Core data directory
#
# Warning: DO NOT EDIT this file. Settings in this file can be overridden at
# runtime and comments will be lost. Instead, to statically configure the
# bitcoin node and wallet, create a bitcoin.conf file with the desired settings.

proxy=host:123
prune=2861
wallet=wallet1
wallet=wallet2

instead of the current settings.json file:

{
    "proxy": "host:123",
    "prune": 2861,
    "wallet": ["wallet1", "wallet2"]
}

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: 70675c3

@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...

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).

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).

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.

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.

utACK b25ed47

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

re-ACK b25ed47

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

Approach re-ACK 9c69cfe 🌾

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Approach re-ACK 9c69cfe4c54e38edd2f54303be2f8a53dcf5bad8 🌾
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUib/Av/Xc5gqDLuaziBgKQFpjDzCaVlaBH+BI5UjULzIL6ql9F0kCXzwHU1l/2P
4yFXMCzjjUUR2w6Qb/NJzVIYJYFoyU4kDu5E01nuMu/pvtfqnQ20/Zc2GTo5Sqd3
9X5DN5xFGU6nUZmmWfsTriQzugrjaYkYPONzk4LyaIpd8i+iZnPf6Tfyu46XnTv6
r+vyxGMhRdOJpl0lzQPnVAnBpHGnxKfv268edwgYmBJxRXKsMCYzk0nrIO8bBbwm
hDMhMqf1pvwvehM6OURESXk6o+TeGqJ37sXxJBHO8TrXUzOfmmvmET4z3JNV5qCx
4LqTkKhPSWQ/DjR7h8VCFkijfGadVbIPGztqeBpLnVORdaBvWwv6oRvLWLxl0f7Y
Vv9LD6bW6jTFum+ALVO4mUW9E0CzpN7zTQ0OERTXZXWw8fHW5+le+CFNmIRjVFOO
p4dvMwlFOW0ECMx06LuLOg00D7Eec59MRk30rG6yposECwthJd3eDX7in/tUWI1A
e4MKLdVZ
=dJtQ
-----END PGP SIGNATURE-----

utACK 9c69cfe

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?

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

re: #15935 (review)

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.

Addressed my feedback in #19624