Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jul 23, 2020

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented May 1, 2019

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
Copy link
Contributor

@DrahtBot DrahtBot commented May 2, 2019

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, 8932b6e) Reference (master, 597d2f9)
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
Copy link
Member

@luke-jr luke-jr commented May 2, 2019

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

ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 3, 2019
This change makes it easier to add a new persistent settings source in followup
PR bitcoin#15935 without introducing new bugs and inconsistencies.

This PR does not change any behavior and has good test coverage through the
util_SettingsMerge test added in bitcoin#15869.

Right now settings from different sources are partially merged when settings
are parsed, and partially merged when settings are retrieved. The logic
implemented merging during retrieval is repeated 5 places:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

which makes it cumbersome introduce a new settings source. Inconsistencies
between the 5 implementations are also responsible for some confusing and
counterintuitive behaviors (documented in this PR but not changed for now):

  - Ignored command line arguments (see ArgsManager::ParseParameters)
  - Inconsistently ignored network-specific arguments (see GetSetting)
  - Reappearing negated config arguments (see GetListSetting)

After this change, merging of settings happens in one place: a new
settings.h/settings.cpp file in code that has more comments and is less
duplicative. It separates merging from parsing, using a data structure that
holds complete representation of parsed config files and command line options,
instead of the current more abstract data structure that has negated values
represented by placeholder map entries.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 7, 2019
This change makes it easier to add a new persistent settings source in followup
PR bitcoin#15935 without introducing new bugs and inconsistencies.

This PR does not change any behavior and has good test coverage through the
util_SettingsMerge test added in bitcoin#15869.

Right now settings from different sources are partially merged when settings
are parsed, and partially merged when settings are retrieved. The logic
implemented merging during retrieval is repeated 5 places:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

which makes it cumbersome introduce a new settings source. Inconsistencies
between the 5 implementations are also responsible for some confusing and
counterintuitive behaviors (documented in this PR but not changed for now):

  - Ignored command line arguments (see ArgsManager::ParseParameters)
  - Inconsistently ignored network-specific arguments (see GetSetting)
  - Reappearing negated config arguments (see GetListSetting)

After this change, merging of settings happens in one place: a new
settings.h/settings.cpp file in code that has more comments and is less
duplicative. It separates merging from parsing, using a data structure that
holds complete representation of parsed config files and command line options,
instead of the current more abstract data structure that has negated values
represented by placeholder map entries.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 8, 2019
This change makes it easier to add a new persistent settings source in followup
PR bitcoin#15935 without introducing new bugs and inconsistencies.

This PR does not change any behavior and has good test coverage through the
util_SettingsMerge test added in bitcoin#15869.

Right now settings from different sources are partially merged when settings
are parsed, and partially merged when settings are retrieved. The logic
implemented merging during retrieval is repeated 5 places:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

which makes it cumbersome introduce a new settings source. Inconsistencies
between the 5 implementations are also responsible for some confusing and
counterintuitive behaviors (documented in this PR but not changed for now):

  - Ignored command line arguments (see ArgsManager::ParseParameters)
  - Inconsistently ignored network-specific arguments (see GetSetting)
  - Reappearing negated config arguments (see GetListSetting)

After this change, merging of settings happens in one place: a new
settings.h/settings.cpp file in code that has more comments and is less
duplicative. It separates merging from parsing, using a data structure that
holds complete representation of parsed config files and command line options,
instead of the current more abstract data structure that has negated values
represented by placeholder map entries.
@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented May 8, 2019

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, #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
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 18, 2019

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
Copy link
Member

@luke-jr luke-jr commented May 18, 2019

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
Copy link
Member

@jnewbery jnewbery commented Jul 2, 2019

Concept ACK

@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Jul 3, 2019

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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jul 3, 2019

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
Copy link
Contributor

@ajtowns ajtowns commented Jul 3, 2019

@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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jul 3, 2019

@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
Copy link
Contributor

@ajtowns ajtowns commented Jul 3, 2019

@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
Copy link
Member

@jnewbery jnewbery commented Jul 10, 2019

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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jul 10, 2019

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
Copy link
Member

@luke-jr luke-jr commented Jul 10, 2019

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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jul 10, 2019

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.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Thanks for review!

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

}

const std::vector<std::string>& in_keys = in.getKeys();
const std::vector<UniValue>& in_values = in.getValues();
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

ced5588

Thanks, updated

@@ -50,14 +50,15 @@ Subdirectory | File(s) | Description
`indexes/blockfilter/basic/` | `fltrNNNNN.dat`<sup>[\[2\]](#note2)</sup> | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic`
`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
`./` | `banlist.dat` | Stores the IPs/subnets of banned nodes
`./` | `bitcoin.conf` | Contains [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`; can be specified by `-conf` option
`./` | `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
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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

doc/files.md Outdated
`./` | `bitcoind.pid` | Stores the process ID (PID) of `bitcoind` or `bitcoin-qt` while running; created at start and deleted on shutdown; can be specified by `-pid` option
`./` | `debug.log` | Contains debug information and general logging generated by `bitcoind` or `bitcoin-qt`; can be specified by `-debuglogfile` option
`./` | `fee_estimates.dat` | Stores statistics used to estimate minimum transaction fees and priorities required for confirmation
`./` | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used
`./` | `mempool.dat` | Dump of the mempool's transactions
`./` | `onion_private_key` | Cached Tor hidden service private key for `-listenonion` option
`./` | `peers.dat` | Peer IP address database (custom format)
`./` | `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
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

Thanks, updated

@@ -397,7 +397,7 @@ void SetupServerArgs(NodeContext& node)
#endif
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);
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);
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);
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);
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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.

@@ -418,6 +418,7 @@ void SetupServerArgs(NodeContext& node)
"(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);
gArgs.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
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);
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);
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

Could change "users" to "user" but this seems slightly worse

/**
* Access settings with lock held.
*/
template <typename Fn>
void LockSettings(Fn&& fn)
{
LOCK(cs_args);
fn(m_settings);
}

Copy link
Contributor Author

@ryanofsky ryanofsky Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

This function template is not used in this PR.

Right, it is used by #15936 and #15937

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jul 21, 2020

utACK b25ed47

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

Copy link
Member

@hebasto hebasto left a comment

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

@achow101
Copy link
Member

@achow101 achow101 commented Jul 21, 2020

re-ACK b25ed47

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Approach ACK b25ed47 (style nits only)

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

errors.emplace_back(strprintf("Failed reading settings file %s", path.string()));
return false;
}
file.close();
Copy link
Member

@MarcoFalke MarcoFalke Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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

@@ -24,19 +26,31 @@ namespace util {
//! https://github.com/bitcoin/bitcoin/pull/15934/files#r337691812)
using SettingsValue = UniValue;

//! Stored bitcoin settings. This struct combines settings from the command line
//! and a read-only configuration file.
//! Stored bitcoin settings. This struct combines settings from the command line,
Copy link
Member

@MarcoFalke MarcoFalke Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
//! Stored bitcoin settings. This struct combines settings from the command line,
//! Stored settings. This struct combines settings from the command line,

Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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

Thanks, updated

if (error_out) {
error_out->emplace_back(error);
} else {
LogPrintf("%s\n", error);
Copy link
Member

@MarcoFalke MarcoFalke Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to add dead code?

Copy link
Member

@MarcoFalke MarcoFalke Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@MarcoFalke MarcoFalke Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

any reason to add dead code?

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

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Thanks!

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

re: #15935 (review)

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.

errors.emplace_back(strprintf("Failed reading settings file %s", path.string()));
return false;
}
file.close();
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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

@@ -24,19 +26,31 @@ namespace util {
//! https://github.com/bitcoin/bitcoin/pull/15934/files#r337691812)
using SettingsValue = UniValue;

//! Stored bitcoin settings. This struct combines settings from the command line
//! and a read-only configuration file.
//! Stored bitcoin settings. This struct combines settings from the command line,
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

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

Thanks, updated

if (error_out) {
error_out->emplace_back(error);
} else {
LogPrintf("%s\n", error);
Copy link
Contributor Author

@ryanofsky ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #15935 (comment)

any reason to add dead code?

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 23, 2020

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

Approach re-ACK 9c69cfe 🌾

Show signature and timestamp

Signature:

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

Timestamp of file with hash 6253e88b8270f1f8ad8dbba3415a37830e223905d1d01c5eb3c11cee6b400cfb -

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jul 23, 2020

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?

@MarcoFalke MarcoFalke merged commit f4cfa6d into bitcoin:master Jul 23, 2020
4 checks passed
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 23, 2020

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
Copy link
Contributor Author

@ryanofsky ryanofsky commented Jul 24, 2020

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.

sidhujag added a commit to syscoin/syscoin that referenced this issue Jul 24, 2020
…storage

9c69cfe Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)

Pull request description:

  Persistent settings are used in followup PRs bitcoin#15936 to unify gui settings between bitcoin-qt and bitcoind, and bitcoin#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.

ACKs for top commit:
  MarcoFalke:
    Approach re-ACK 9c69cfe 🌾
  jnewbery:
    utACK 9c69cfe

Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 30, 2020

Addressed my feedback in #19624

@jnewbery jnewbery mentioned this pull request Aug 17, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Oct 20, 2020
Summary:
Currently unused, but includes tests.

Backport of [[bitcoin/bitcoin#15935 | PR15935]] part [1/2] : bitcoin/bitcoin@eb682c5

Test Plan:
  ninja check

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8011
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Oct 21, 2020
Summary:
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.

Backport of [[bitcoin/bitcoin#15935 | PR15935]] part [2/2] : bitcoin/bitcoin@9c69cfe

Depends on D8009 and D8011

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Subscribers: jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8012
sidhujag added a commit to syscoin-core/syscoin that referenced this issue Nov 10, 2020
083c954 Add settings_tests (Russell Yanofsky)
7f40528 Deduplicate settings merge code (Russell Yanofsky)
9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cf Remove includeconf nested scope (Russell Yanofsky)
5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky)

Pull request description:

  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`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#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 bitcoin#15935).
  - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) 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](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108).

  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 bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_
  * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_

ACKs for top commit:
  ariard:
    ACK 083c954
  jnewbery:
    ACK 083c954
  jamesob:
    ACK 083c954

Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet