interfaces: Expose settings.json methods to GUI #15936

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

Add interfaces::Node updateSetting, forceSetting, resetSettings, isSettingIgnored, and getPersistentSetting methods so GUI is able to manipulate settings.json file and use and modify node settings.

(Originally this PR also contained GUI changes to unify bitcoin-qt and bitcoind persistent settings and call these methods, but the GUI commits have been dropped from this PR and moved to bitcoin-core/gui/pull/602)

DrahtBot · 2019-05-02T00:42:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg 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.

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.

jonasschnelli · 2019-05-18T10:23:34Z

Concept ACK

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.

ryanofsky · 2019-12-04T02:31:12Z

Travis compile error in https://travis-ci.org/bitcoin/bitcoin/jobs/620388014#L3988 seems to be due to a clang bug with macros and raw strings: https://reviews.llvm.org/D39279

qt/test/optiontests.cpp:56:100: warning: missing terminating '"' character [-Winvalid-pp-token]
)", "std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str()", "R\"({
                                                                                                   ^
qt/test/optiontests.cpp:56:100: error: expected expression

Maybe can work around by adding parentheses, otherwise could declare temp variable.

Other travis error https://travis-ci.org/bitcoin/bitcoin/jobs/620388019#L2929 seems like a qsettings int value gets stringified on mac (doesn't happen on linux):

Actual: "dbcache": "600",
Expected: "dbcache": 600,

It would probably be good to fix this by adding an explicit type argument to ToSetting instead of trying to guess the setting type from the variant type

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.

hebasto

re-ACK 3340286

Rspigler · 2022-05-20T03:32:26Z

I will re-test tomorrow. Thanks for keeping this up!

1d4122d init: Allow -proxy="" setting values (Ryan Ofsky) Pull request description: This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin/bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin/bitcoin#15936, reported in bitcoin/bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message. ACKs for top commit: hebasto: re-ACK 1d4122d, only rebased since my recent [review](bitcoin/bitcoin#24830 (review)). Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53

vasild

ACK 3340286

This provides a way for the GUI settings dialog box to retain previous pruning and proxy settings when they are disabled, as requested by vasild: bitcoin/bitcoin#15936 (review) bitcoin/bitcoin#15936 (comment) bitcoin/bitcoin#15936 (comment) Importantly, while this PR changes the settings.json format, it changes it in a fully backwards compatible way, so previous versious of bitcoind and bitcoin-qt will correctly interpret prune, proxy, and onion settins written by new versions of bitcoin-qt.

Rspigler · 2022-05-21T04:19:45Z

tACK 3340286

Tested with various changes to settings.json, bitcoin.conf, and the GUI. Everything behaved as it should.

Previous issues people reported are resolved (including #24457).

Only other issue is solved by bitcoin-core/gui#603

promag

Concept ACK, unifying persistent node settings across apps makes sense to me.

src/node/interfaces.cpp

Rspigler · 2022-05-22T01:11:03Z

tACK 3340286 all tests pass

This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin#15936, reported in bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.

ryanofsky

Updated 3340286 -> f9fdcec (pr/qtset.46 -> pr/qtset.47, compare) just renaming updateSetting to updateRwSetting;

src/node/interfaces.cpp

…l constructor 31122aa refactor: Pass interfaces::Node references to OptionsModel constructor (Ryan Ofsky) Pull request description: Giving OptionsModel access to the node interface is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It has been split off from #602 to simplify that PR. Previously these commits were part of bitcoin/bitcoin#15936 and also had some review discussion there. ACKs for top commit: promag: Code review ACK 31122aa. furszy: Code ACK 31122aa jarolrod: ACK 31122aa Tree-SHA512: b8529322fd7ba97e19864129e6cf5f9acc58c124f2e5a7c50aca15772c8549de3c24e8b0c27e8cf2c06fd26529e9cdb898b0788a1de3cbfdfbdd3f85c9f0fe69

vasild

ACK f9fdcec

…ettings.json 344537c qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky) Pull request description: Backport of #24498 to 22.x branch This was already backported to the 23.x branch in #24511, but vasild discovered the issue can affect 22.x as well #15936 (comment): > While testing this, 22.x crashes if `settings.json` contains `"prune": 1234` due to #24498 which was fixed in 23.0. So, if this PR is included in 24.x and a user upgrades to 24.x and then downgrades to 22.x his 22.x would crash at startup. It's not very important to backport this to 22.x because I can work around the issue other ways, but I do see 22.x is listed as an active branch https://github.com/bitcoin/bitcoin/branches, so if there is another 22.x release, it would be nice to have this fix and not have a wonky uncaught univalue exception waiting to be hit ACKs for top commit: laanwj: ACK 344537c Tree-SHA512: 479f7fb4b4b73ec85f28e97af9b21d54245b8ab4d246e50134b37f1726fe3c57cf388fe4973f8ccf1b9efa7663166d1fbeb631758b39d4490b5eab82d0c83d77

hebasto

re-ACK f9fdcec, only a function renamed since my recent review.

ryanofsky

This is probably ready for merge (sorry for the confusing history, it was originally a lot bigger PR before I removed the GUI parts)

…s to OptionsModel constructor 31122aa refactor: Pass interfaces::Node references to OptionsModel constructor (Ryan Ofsky) Pull request description: Giving OptionsModel access to the node interface is needed as part of bitcoin#602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It has been split off from bitcoin#602 to simplify that PR. Previously these commits were part of bitcoin#15936 and also had some review discussion there. ACKs for top commit: promag: Code review ACK 31122aa. furszy: Code ACK 31122aa jarolrod: ACK 31122aa Tree-SHA512: b8529322fd7ba97e19864129e6cf5f9acc58c124f2e5a7c50aca15772c8549de3c24e8b0c27e8cf2c06fd26529e9cdb898b0788a1de3cbfdfbdd3f85c9f0fe69

…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

…settings e47c6c7 Reset settings.json when GUI options are reset (Ryan Ofsky) 99ccc02 Add release notes about unified bitcoin-qt and bitcoind persistent settings (Ryan Ofsky) 504b06b Migrate -lang setting from QSettings to settings.json (Ryan Ofsky) 9a016a3 Migrate -prune setting from QSettings to settings.json (Ryan Ofsky) f067e19 Migrate -proxy and -onion settings from QSettings to settings.json (Ryan Ofsky) a09e3b7 Migrate -listen and -server settings from QSettings to settings.json (Ryan Ofsky) d2ada6e Migrate -upnp and -natpmp settings from QSettings to settings.json (Ryan Ofsky) 1dc4fc2 Migrate -spendzeroconfchange and -signer settings from QSettings to settings.json (Ryan Ofsky) a7ef6d5 Migrate -par setting from QSettings to settings.json (Ryan Ofsky) 284f339 Migrate -dbcache setting from QSettings to settings.json (Ryan Ofsky) Pull request description: If a setting like pruning, port mapping, or a network proxy is enabled in the GUI, it will now be stored in the bitcoin persistent setting file in the datadir and shared with bitcoind, instead of being stored as Qt settings which end up in the the windows registry or platform specific config files and are ignored by bitcoind. This PR has been split off from #15936 so some review of these commits previously took place in that PR. ACKs for top commit: furszy: Code review ACK e47c6c76 hebasto: ACK e47c6c7 Tree-SHA512: 076ea7c7efe67805b4a357113bfe1643dce364d0032774106de59566a0ed5771d57a5923920085e03d686beb34b98114bd278555dfdf8bb7af0b778b0f35b7d2

…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

This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases. The error was originally added in bitcoin/bitcoin#20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well. The motivation for this change is to prevent a GUI bug that happens with bitcoin/bitcoin#15936, reported in bitcoin/bitcoin#15936 (review) by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.

This provides a way for the GUI settings dialog box to retain previous pruning and proxy settings when they are disabled, as requested by vasild: bitcoin/bitcoin#15936 (review) bitcoin/bitcoin#15936 (comment) bitcoin/bitcoin#15936 (comment) Importantly, while this PR changes the settings.json format, it changes it in a fully backwards compatible way, so previous versious of bitcoind and bitcoin-qt will correctly interpret prune, proxy, and onion settins written by new versions of bitcoin-qt.

This provides a way for the GUI settings dialog box to retain previous pruning and proxy settings when they are disabled, as requested by vasild: bitcoin#15936 (review) bitcoin#15936 (comment) bitcoin#15936 (comment) Importantly, while this PR changes the settings.json format, it changes it in a fully backwards compatible way, so previous versious of bitcoind and bitcoin-qt will correctly interpret prune, proxy, and onion settins written by new versions of bitcoin-qt.

ryanofsky mentioned this pull request May 1, 2019

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

Merged

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

ryanofsky force-pushed the pr/qtset branch from e7bf93f to a50315a Compare May 3, 2019 21:18

ryanofsky force-pushed the pr/qtset branch from a50315a to f625d1a Compare May 7, 2019 21:41

ryanofsky force-pushed the pr/qtset branch from f625d1a to 0ab41dd Compare May 8, 2019 16:13

ryanofsky mentioned this pull request May 22, 2019

GUI: Actual graphical tests #16075

Closed

laanwj added the Feature label Sep 30, 2019

ryanofsky mentioned this pull request Sep 30, 2019

[qt] move QSettings to bitcoin_rw.conf where possible #12833

Closed

ryanofsky mentioned this pull request Oct 22, 2019

qt: Fix text display when state of prune button is changed #17035

Closed

ryanofsky mentioned this pull request Nov 12, 2019

gui: Fix intro dialog labels when the prune button is toggled #17453

Merged

jonasschnelli mentioned this pull request Dec 1, 2019

qt: Add -guisettingsdir option #17636

Closed

ryanofsky force-pushed the pr/qtset branch from 0ab41dd to 955f7c0 Compare December 3, 2019 23:55

cvengler mentioned this pull request Dec 8, 2019

Configuration file when settings are changed in GUI #17692

Closed

hebasto mentioned this pull request Jan 12, 2020

bitcoin-core snap issues #17912

Closed

ryanofsky changed the title ~~WIP: Unify bitcoin-qt and bitcoind persistent settings~~ Unify bitcoin-qt and bitcoind persistent settings Jun 4, 2020

ryanofsky force-pushed the pr/qtset branch from 955f7c0 to 821268f Compare June 4, 2020 20:12

ryanofsky marked this pull request as ready for review June 4, 2020 20:12

hebasto approved these changes May 19, 2022

View reviewed changes

vasild approved these changes May 20, 2022

View reviewed changes

ryanofsky mentioned this pull request May 20, 2022

[22.x] qt: Avoid crash on startup if int specified in settings.json #25180

Merged

promag reviewed May 21, 2022

View reviewed changes

src/node/interfaces.cpp Show resolved Hide resolved

ryanofsky force-pushed the pr/qtset branch from 3340286 to f9fdcec Compare May 23, 2022 15:31

ryanofsky commented May 23, 2022

View reviewed changes

src/node/interfaces.cpp Show resolved Hide resolved

vasild approved these changes May 24, 2022

View reviewed changes

hebasto approved these changes May 25, 2022

View reviewed changes

ryanofsky commented May 26, 2022

View reviewed changes

maflcko merged commit 2642dee into bitcoin:master May 26, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022

Merge bitcoin#15936: interfaces: Expose settings.json methods to GUI

e191780

bitcoin locked and limited conversation to collaborators May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

interfaces: Expose settings.json methods to GUI #15936

interfaces: Expose settings.json methods to GUI #15936

Uh oh!

ryanofsky commented May 1, 2019 •

edited

Loading

Uh oh!

DrahtBot commented May 2, 2019 •

edited

Loading

Uh oh!

jonasschnelli commented May 18, 2019

Uh oh!

ryanofsky commented Dec 4, 2019 •

edited

Loading

Uh oh!

hebasto left a comment

Uh oh!

Rspigler commented May 20, 2022

Uh oh!

vasild left a comment

Uh oh!

Rspigler commented May 21, 2022

Uh oh!

promag left a comment

Uh oh!

Uh oh!

Rspigler commented May 22, 2022

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

vasild left a comment

Uh oh!

hebasto left a comment •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

interfaces: Expose settings.json methods to GUI #15936

interfaces: Expose settings.json methods to GUI #15936

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented May 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Rspigler commented May 20, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Rspigler commented May 21, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rspigler commented May 22, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented May 1, 2019 •

edited

Loading

DrahtBot commented May 2, 2019 •

edited

Loading

ryanofsky commented Dec 4, 2019 •

edited

Loading

hebasto left a comment •

edited

Loading