Unify bitcoin-qt and bitcoind persistent settings #602

ryanofsky · 2022-05-19T20:14:16Z

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 bitcoin/bitcoin#15936 so some review of these commits previously took place in that PR.

DrahtBot · 2022-05-20T08:46:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#149 (Intro: Have user choose assumevalid 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.

ryanofsky · 2022-05-20T10:14:26Z

Rebased db64d21 -> 60a9db7 (pr/qtsopt.1 -> pr/qtsopt.2, compare) after bitcoin/bitcoin#24830 with no other changes.

bitcoin/bitcoin#24830 doesn't interact with this PR directly, but @Rspigler mentioned retesting this in bitcoin/bitcoin#15936 (comment), and testing this PR is annoying without bitcoin/bitcoin#24830

vasild

ACK 60a9db7

I feel strongly that the vanishing disabled values after restarting should be resolved, but that can be done as a followup, before 24.x.

src/qt/optionsmodel.cpp

ryanofsky

Thanks for reviewing!

Rebased 60a9db7 -> 9ae2f6f (pr/qtsopt.2 -> pr/qtsopt.3, compare) with suggested fixes on top of updated base PRs

src/qt/optionsmodel.cpp

ryanofsky · 2022-05-20T18:51:11Z

re: #602 (review)

I feel strongly that the vanishing disabled values after restarting should be resolved, but that can be done as a followup, before 24.x.

Since we talked about multiple solutions for this in #596, I posted #603 as one possible solution. I really do like simplicity of not keeping unused values across restarts. If I set a temporary ssh proxy value one day while I'm using bad wifi, I don't want that value sticking forever after I disabled the proxy, and confusing me months later or making it more difficult to set up tor. But #596 discusses this issue, #603 is one possible solution, and others solutions should be possible too.

Rspigler · 2022-05-21T06:13:56Z

tACK 9ae2f6f

Now the GUI changes settings.json (instead of just settings.json affecting the GUI; it's a two way street).

One thing I noticed is, setting prune=x,000 always results in x+1,000. For example, prune=2 -> prune=3 GB.

bitcoin.conf does also now change the GUI, with settings.json taking precedence. bitcoin.conf is never written to by the GUI or settings.json, as discussed here.

Also, main settings dialog has changed appropriately: "Options set in this dialog are overridden by the command line". (It used to say ..."or also in the configuration file"). (It is not overridden by the config file, since it should be matching the config file).

One thing I noticed:

If settings.json is empty, and bitcoin.conf is filled with settings; I open bitcoin-qt and the settings are reflected there. I disable and re-enable those settings, close and open bitcoin-qt, shouldn't those settings be written in settings.json? They are not.

promag

Concept ACK.

Rspigler · 2022-05-22T02:10:47Z

Was having issues with tests on my end, but now everything is passing. (tACK 9ae2f6f)

a63b60f refactor: Add OptionsModel getOption/setOption methods (Ryan Ofsky) Pull request description: This is a trivial change which is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from #602 because it causes a lot of rebase conflicts (any time there is a GUI options change). This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1 ACKs for top commit: vasild: ACK a63b60f furszy: Code review ACK a63b60f promag: Code review ACK a63b60f. Tree-SHA512: 1d99a1ce435b4055c1a38d2490702cf5b89bacc7d168c9968a60550bfd9f6dace649d5e98699de68d6305f02d5d1e3eb7e177ab578b98b996dd873b1df0ed236

ryanofsky · 2022-05-23T19:04:59Z

re: #602 (comment)

One thing I noticed is, setting prune=x,000 always results in x+1,000. For example, prune=2 -> prune=3 GB.

This can happen because -prune setting is specified in MiB (2^20 bytes) while GUI setting is specified in GB (10^9 bytes). So in various cases rounded values will be displayed. Like if you have prune=600 in bitcoin.conf, the gui will display pruning size of 1GB. But as soon as you change settings in the GUI, it will add "prune": 953 (1GB) or 1907 (2GB) or 2861 (3GB), etc to the settings.json file and whatever value is in bitcoin.conf will be ignored. If you edit the settings.json file manually, whatever value you specify will be used, but the GUI options dialog will convert it to GB for display. You should let me know if you see anything more strange than this, but hopefully behavior should be sensible in every case.

If settings.json is empty, and bitcoin.conf is filled with settings; I open bitcoin-qt and the settings are reflected there. I disable and re-enable those settings, close and open bitcoin-qt, shouldn't those settings be written in settings.json? They are not.

This is intentional. Only settings that a user actually changes will be written to settings.json. If all settings were copied to settings.json, then bitcoin.conf would stop having any effect after settings.json was written. Intention is for bitcoin.conf to be a source of settings from a packager or sysadmin, and settings.json to contain customizations from the user. User settings have precedence, but system settings take effect otherwise.

ryanofsky · 2022-05-23T19:07:04Z

Rebased 9ae2f6f -> 37682ee (pr/qtsopt.3 -> pr/qtsopt.4, compare) after #600 merge

…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 37682ee

This is just the first of multiple settings that will be stored in the bitcoin persistent setting file and shared with bitcoind, instead of being stored as Qt settings backed by the windows registry or platform specific config files which are ignored by bitcoind. Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>

…ettings.json

vasild

ACK 689dd65

hebasto · 2022-05-26T15:55:54Z

Concept ACK.

src/qt/bitcoin.cpp

ryanofsky

Updated 689dd65 -> ac5fb01 (pr/qtsopt.5 -> pr/qtsopt.6, compare) removing _() from qt code as suggested #602 (comment). Maybe will change it back depending how discussion goes in bitcoin/bitcoin#22764 (comment)

Changes look like:

--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -270,7 +270,9 @@ bool BitcoinApplication::createOptionsModel(bool resetSettings)
         fs::path settings_path;
         if (gArgs.GetSettingsPath(&settings_path)) {
             error += Untranslated("\n");
-            error += strprintf(_("Settings file %s might be corrupt or invalid."), fs::quoted(fs::PathToString(settings_path)));
+            std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
+            error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
+            error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
         }
         InitError(error);
         QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));
diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
index 2c15bfd45e3..d8d39abe6bd 100644
--- a/src/qt/optionsmodel.cpp
+++ b/src/qt/optionsmodel.cpp
@@ -200,7 +200,8 @@ bool OptionsModel::Init(bilingual_str& error)
         } catch (const std::exception& e) {
             // This handles exceptions thrown by univalue that can happen if
             // settings in settings.json don't have the expected types.
-            error = strprintf(_("Could not read setting \"%s\", %s."), setting, e.what());
+            error.original = strprintf("Could not read setting \"%s\", %s.", setting, e.what());
+            error.translated = tr("Could not read setting \"%1\", %2.").arg(QString::fromStdString(setting), e.what()).toStdString();
             return false;
         }
     }

vasild

ACK ac5fb01

hebasto · 2022-06-04T10:41:55Z

src/qt/bitcoin.cpp

+        if (gArgs.GetSettingsPath(&settings_path)) {
+            error += Untranslated("\n");
+            std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
+            error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
+            error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
+        }


What are expected scenarios which run this code?

Asking because settings.json file I/O errors are handled in the InitSettings() function.

re: #602 (comment)

What are expected scenarios which run this code?

Asking because settings.json file I/O errors are handled in the InitSettings() function.

This is triggered if invalid data was written to the setting.json file (from a bug or from the user editing the file). SettingTo{Bool,Int,String} functions can throw exceptions, OptionsModel::Init function will catch them and turn them into an error string, and this code log the error and show an error dialog. OptionsModel::Init right now basically just returns false if an array or object value was found where a normal value was expected, but it could return other errors in other cases as code changes.

hebasto · 2022-06-04T10:48:57Z

src/qt/optionsmodel.cpp

+            // This handles exceptions thrown by univalue that can happen if
+            // settings in settings.json don't have the expected types.


Should the following settings.json

{ "dbcache": "qwerty" }

throw an exception?

re: #602 (comment)

Should the following settings.json

{ "dbcache": "qwerty" }

throw an exception?

Probably not here, but maybe in the future it should raise an init error. bitcoin/bitcoin#16545 tries to add validation flags to make this easier. It is better to handle validation in shared bitcoind code not GUI code specifically, I think. This PR is only trying to add more settings to settings.json file, not change the way the file is interpreted.

src/qt/bitcoin.h

ryanofsky

Updated ac5fb01 -> bc6a562 (pr/qtsopt.6 -> pr/qtsopt.7, compare), just adding nodiscard

src/qt/bitcoin.h

ryanofsky · 2022-06-06T15:34:54Z

src/qt/bitcoin.cpp

+        if (gArgs.GetSettingsPath(&settings_path)) {
+            error += Untranslated("\n");
+            std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
+            error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
+            error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();
+        }


re: #602 (comment)

What are expected scenarios which run this code?

Asking because settings.json file I/O errors are handled in the InitSettings() function.

This is triggered if invalid data was written to the setting.json file (from a bug or from the user editing the file). SettingTo{Bool,Int,String} functions can throw exceptions, OptionsModel::Init function will catch them and turn them into an error string, and this code log the error and show an error dialog. OptionsModel::Init right now basically just returns false if an array or object value was found where a normal value was expected, but it could return other errors in other cases as code changes.

ryanofsky · 2022-06-06T15:45:53Z

src/qt/optionsmodel.cpp

+            // This handles exceptions thrown by univalue that can happen if
+            // settings in settings.json don't have the expected types.


re: #602 (comment)

Should the following settings.json

{ "dbcache": "qwerty" }

throw an exception?

Probably not here, but maybe in the future it should raise an init error. bitcoin/bitcoin#16545 tries to add validation flags to make this easier. It is better to handle validation in shared bitcoind code not GUI code specifically, I think. This PR is only trying to add more settings to settings.json file, not change the way the file is interpreted.

vasild

ACK bc6a562

furszy · 2022-06-08T19:08:54Z

src/qt/bitcoin.cpp

+        InitError(error);
+        QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));


In 6afa080:

This is going to open two dialogs, one after the other. Is that intended?

First one:
InitError signals ThreadSafeMessageBox which is captured by BitcoinGUI::message function, which opens an error dialog.

Second one:
QMessageBox::critical opens a critical message box with the given text in a standalone window.

re: #602 (comment)

This wouldn't be intended, but I don't think this problem happens in practice, or at least I can't reproduce it. I can trigger the error with settings.json file:

{ "dbcache": [ ] }

In this case, InitError just logs an error without creating a message box because app.createWindow has not been called yet. After that the QMessageBox::critical call actually shows a message box.

There is a bunch of other in code the src/qt/bitcoin.cpp file following the pattern of calling InitError then QMessageBox::critical. It might be something that can be simplified in the future, but I think it is all correct and not buggy.

ah ok, that was a bit misleading when I read it. Make sense, thanks 👌🏼. Agree about the possible future simplification.

furszy

Code reviewed bc6a562 (except the prune commit).
Left few comments, nothing big, not blocking.

Plus a small point about 01f5142:
The commit message talks about node.args = nullptr removal but that is not happening there.

furszy · 2022-06-09T13:16:51Z

src/qt/test/optiontests.cpp

+void OptionTests::resetArgs()
+{
+    gArgs.LockSettings([&](util::Settings& s) { s = m_previous_settings; });
+    gArgs.ClearPathCache();
+}


In 6afa080:

nit: could use Qt framework's function init() for this.
It's a private slot that Qt calls before running each function (example: furszy/bitcoin-core@5b63a5c)

re: #602 (comment)

nit: could use Qt framework's function init() for this

Great suggestion! Added your patch

furszy · 2022-06-09T13:29:50Z

src/qt/optionsmodel.cpp

 #include <validation.h> // For DEFAULT_SCRIPTCHECK_THREADS
+#include <wallet/wallet.h> // For DEFAULT_SPEND_ZEROCONF_CHANGE


Not for this PR but better to make the options model call getter functions that return these default values from the node and wallet interfaces instead of accessing them through direct includes.

I think that it's going against the multi-processes separation goal.

re: #602 (comment)

Not for this PR but I would rather make the options model call getter functions that return these default values from the node and wallet interfaces instead of accessing them through direct includes.

I think that it's going against the multi-processes separation goal.

I think the PR helps with this goal by at least getting node and gui to read settings from the same sources, but reading settings still requires dealing with unset settings and knowing default values. If settings API were better it would know about default values, but at least having a set of shared constants prevents values from getting out of sync.

yeah for sure, this PR is great as is. I was thinking more broadly.

GUI architecture wise, I think that only the interfaces should have access to validation.h and wallet.h. And going further, ideally only wallet model (one per active wallet) should have access to the wallet interface.

furszy · 2022-06-09T13:47:01Z

src/qt/optionsmodel.cpp

+        if (changed()) {
+            update(value.toBool());
            setRestartRequired(true);
        }
        break;
    case Server:
-        if (settings.value("server") != value) {
-            settings.setValue("server", value);
+        if (changed()) {
+            update(value.toBool());
            setRestartRequired(true);
        }


in 01f5142:

nit: maybe unify it?

Suggested change

if (changed()) {

update(value.toBool());

setRestartRequired(true);

}

break;

case Server:

if (settings.value("server") != value) {

settings.setValue("server", value);

if (changed()) {

update(value.toBool());

setRestartRequired(true);

}

case Listen:

case Server:

if (changed()) {

update(value.toBool());

setRestartRequired(true);

}

re: #602 (comment)

nit: maybe unify it?

Made this consolidation. Some other consolidations are possible too, but it looks like those require changing settings organization, or cause more complicated diffs, or interfere with followup #603, so in these cases I might put off deduping for a followup instead of trying to do it at the same time as other changes.

yep, sounds good

ryanofsky

Thanks for the review!

Updated bc6a562 -> e47c6c7 (pr/qtsopt.7 -> pr/qtsopt.8, compare) with suggested changes

re: #602 (review)

The commit message talks about node.args = nullptr removal but that is not happening there.

Thanks, this was obsolete, dropped it

ryanofsky · 2022-06-09T18:58:30Z

src/qt/test/optiontests.cpp

+void OptionTests::resetArgs()
+{
+    gArgs.LockSettings([&](util::Settings& s) { s = m_previous_settings; });
+    gArgs.ClearPathCache();
+}


re: #602 (comment)

nit: could use Qt framework's function init() for this

Great suggestion! Added your patch

ryanofsky · 2022-06-09T18:59:45Z

src/qt/optionsmodel.cpp

+        if (changed()) {
+            update(value.toBool());
            setRestartRequired(true);
        }
        break;
    case Server:
-        if (settings.value("server") != value) {
-            settings.setValue("server", value);
+        if (changed()) {
+            update(value.toBool());
            setRestartRequired(true);
        }


re: #602 (comment)

nit: maybe unify it?

Made this consolidation. Some other consolidations are possible too, but it looks like those require changing settings organization, or cause more complicated diffs, or interfere with followup #603, so in these cases I might put off deduping for a followup instead of trying to do it at the same time as other changes.

ryanofsky · 2022-06-09T19:00:35Z

src/qt/optionsmodel.cpp

 #include <validation.h> // For DEFAULT_SCRIPTCHECK_THREADS
+#include <wallet/wallet.h> // For DEFAULT_SPEND_ZEROCONF_CHANGE


re: #602 (comment)

Not for this PR but I would rather make the options model call getter functions that return these default values from the node and wallet interfaces instead of accessing them through direct includes.

I think that it's going against the multi-processes separation goal.

I think the PR helps with this goal by at least getting node and gui to read settings from the same sources, but reading settings still requires dealing with unset settings and knowing default values. If settings API were better it would know about default values, but at least having a set of shared constants prevents values from getting out of sync.

furszy

Code review ACK e47c6c7

With two, non-blocking, points for e47c6c7:

When the user presses "Reset Options", the triggered dialog is not telling the user that a backup file will be created.
Maybe the backup file should have a different name: settings1.json.bak, settings2.json.bak, settings3.json.bak etc. If there is an existing backup? (otherwise the previous file is being overwritten)

furszy · 2022-06-10T13:34:24Z

src/qt/optionsmodel.cpp

-    if (prune) {
-        QSettings settings;
-        settings.setValue("nPruneSize", prune_target_gb);
+    const util::SettingsValue cur_value = node().getPersistentSetting("prune");


In 9a016a3:

tiny nit:
Better to try to not use hardcoded strings, we could now do:
auto prune_name = SettingName(Prune);

(same for line 331)

hebasto

ACK e47c6c7

Going to merge this PR and leave nice @furszy's suggestions for follow ups.

Pull request description: Sync with the main repo up to the latest bitcoin/bitcoin@37633d2, which includes bitcoin-core/gui#602. No conflicts :) #### Guix builds on `arm64`: ``` # find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum fef5b12d92485e5bf3344a3e8f2271422151c58cb181ba6965f51b3406515175 guix-build-708323f6023d/output/arm-linux-gnueabihf/SHA256SUMS.part 0460a13c7687f7f7ff582d305a74ca5fe9fa472fc1fdbcd8e027ec14be9da8f3 guix-build-708323f6023d/output/arm-linux-gnueabihf/bitcoin-708323f6023d-arm-linux-gnueabihf-debug.tar.gz 8db3f95f873e73ff8fffa05e2f7cd6a0b19a7c2b577e96e674edb85605489beb guix-build-708323f6023d/output/arm-linux-gnueabihf/bitcoin-708323f6023d-arm-linux-gnueabihf.tar.gz 4d17e2b98fcbb945c8f251eb24c9496f6ad0df3a6ade7a1e61ed240e9979465c guix-build-708323f6023d/output/arm64-apple-darwin/SHA256SUMS.part 6c3c6b0cec8021063f1d553ccdf43946139ffb0dd4eaaf132be9058d65e9c474 guix-build-708323f6023d/output/arm64-apple-darwin/bitcoin-708323f6023d-arm64-apple-darwin-unsigned.dmg cabb7747ac6ec060eb71a092a43056dd6430ed21554af36ba20de551118b796d guix-build-708323f6023d/output/arm64-apple-darwin/bitcoin-708323f6023d-arm64-apple-darwin-unsigned.tar.gz 826c4d7f7edb220a7973b6260905f4fb3e58dd9aaf2567fe7cffd46bfc71f662 guix-build-708323f6023d/output/arm64-apple-darwin/bitcoin-708323f6023d-arm64-apple-darwin.tar.gz 3014169f0610e14e3feb547e49d2ccb7a027852321151b7f00abd79d0a09bc9e guix-build-708323f6023d/output/dist-archive/bitcoin-708323f6023d.tar.gz 1642a1509809e97cc677ca5c4b369a9cadcb419da2d0af58020dc5aec9a7ac24 guix-build-708323f6023d/output/powerpc64-linux-gnu/SHA256SUMS.part 31a5d1690dbb79d538f0f58e62d697654b632a0b84fee4d946ebf9f0d1a6a518 guix-build-708323f6023d/output/powerpc64-linux-gnu/bitcoin-708323f6023d-powerpc64-linux-gnu-debug.tar.gz 4be716159f7559e85a12e014b9b01ed120c4b3af3ae54724ba44193fc0b47da5 guix-build-708323f6023d/output/powerpc64-linux-gnu/bitcoin-708323f6023d-powerpc64-linux-gnu.tar.gz 7b9fbe3238e1a6408d7adfac90e39f97b18fd35a2be9898702eb5eb45b476773 guix-build-708323f6023d/output/powerpc64le-linux-gnu/SHA256SUMS.part 0625815bd3f46224c475e43a033d0cda505c8eb7a8dfc0b6024283f4c4ce1fea guix-build-708323f6023d/output/powerpc64le-linux-gnu/bitcoin-708323f6023d-powerpc64le-linux-gnu-debug.tar.gz a97627fdbbb943543abbf603dfb73f728fb2a0d7e7e06cce7d169dbbc74fe6ea guix-build-708323f6023d/output/powerpc64le-linux-gnu/bitcoin-708323f6023d-powerpc64le-linux-gnu.tar.gz ac4f48f933308b7ad6cb9c6cf280eff13d4de1770ab184535e0149e40d23c98b guix-build-708323f6023d/output/riscv64-linux-gnu/SHA256SUMS.part 1a864557c76def163ca54b398ea7eef66af65ee40287d15aa079f98fb55ac89c guix-build-708323f6023d/output/riscv64-linux-gnu/bitcoin-708323f6023d-riscv64-linux-gnu-debug.tar.gz d02afae96e21cd857e79dd3a90d2dec37a5000a3b13078f4f9e9021c05e13256 guix-build-708323f6023d/output/riscv64-linux-gnu/bitcoin-708323f6023d-riscv64-linux-gnu.tar.gz e674990b30dfc6c16fe4b80336aedc330e4a4843bda603cd29799b039525acfe guix-build-708323f6023d/output/x86_64-apple-darwin/SHA256SUMS.part 0507bad74eb3126d1afa31feb914dcb5da0dd74f0b7a4dfca1c957f9a8a9df4c guix-build-708323f6023d/output/x86_64-apple-darwin/bitcoin-708323f6023d-x86_64-apple-darwin-unsigned.dmg 8547a4fe6fb716049d6eb7a0c726ac04402f25356ebeb9bf00ed83d0d0715c80 guix-build-708323f6023d/output/x86_64-apple-darwin/bitcoin-708323f6023d-x86_64-apple-darwin-unsigned.tar.gz 33e566ae6da0a983e76cbbf4c78706bc9bd4175fecd94ed4144ae3e4a8c9f58b guix-build-708323f6023d/output/x86_64-apple-darwin/bitcoin-708323f6023d-x86_64-apple-darwin.tar.gz afa8afc89d9fe15da66e45e579c89668f3fde5aa50d00b56e97375eda0902fa6 guix-build-708323f6023d/output/x86_64-linux-gnu/SHA256SUMS.part fe858930f4dee66f026ed536426328117e6dd8c50b1748cd9145e937ea16f7f7 guix-build-708323f6023d/output/x86_64-linux-gnu/bitcoin-708323f6023d-x86_64-linux-gnu-debug.tar.gz d84466c0a5a102127dba8eef2f78fa24d80280362d7fb30ba9cec8ab407797cb guix-build-708323f6023d/output/x86_64-linux-gnu/bitcoin-708323f6023d-x86_64-linux-gnu.tar.gz 05e378131c684d18279a9db0382a05601fd41c3bf9efcf4e5bb0c7b964afa72e guix-build-708323f6023d/output/x86_64-w64-mingw32/SHA256SUMS.part a007d0e4588f04afb5c8ed5e5d9585b865f343debcace61707acc8545b64d699 guix-build-708323f6023d/output/x86_64-w64-mingw32/bitcoin-708323f6023d-win64-debug.zip ad5164a5a9de501d8238e7aff0c1a597c6f99cc3be8f514f3c52bdb50a182c3c guix-build-708323f6023d/output/x86_64-w64-mingw32/bitcoin-708323f6023d-win64-setup-unsigned.exe abcc4f45b45c6590e8d08ee5a044d1537b635c61b8febf2938f3b489ebbf1140 guix-build-708323f6023d/output/x86_64-w64-mingw32/bitcoin-708323f6023d-win64-unsigned.tar.gz 0b0d0ad32ef19a9fbb9106e87b0b20aa029164245c23158cceb30b1dfbbff344 guix-build-708323f6023d/output/x86_64-w64-mingw32/bitcoin-708323f6023d-win64.zip ``` [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/131) [![macOS](https://img.shields.io/badge/OS-macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/131) [![Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/131) ACKs for top commit: jarolrod: ACK 708323f Tree-SHA512: 57bbb812bb31daca6fa569765a4d525efdffeb34b8fcb6540c83cbacd20df8c56979e1626c15bc37e01086f83c13b9e73723cf6a40b0e5143466487fe174d553

…reation ac4fb3b gui: reset options, notify user about the backup creation (furszy) Pull request description: Quick follow-up to first point of bitcoin-core/gui#602 (review) ACKs for top commit: ryanofsky: Code review ACK ac4fb3b, just fixing displayed backup directory since last review jarolrod: tACK ac4fb3b Tree-SHA512: cfeca5cd6d6d3d69bbd81211cf1bfd490de13ac96bf53be081a5ceb88611afa57dff2be35f8e0a41b1088b7b892f75a21a9abf47f2e1d77e9e316467eb7c12be

…ings 9d3127b Add settings.json prune-prev, proxy-prev, onion-prev settings (Ryan Ofsky) Pull request description: With #602, if proxy and pruning settings are disabled in the GUI and the GUI is restarted, proxy and prune values are not stored anywhere. So if these settings are enabled in the future, default values will be shown, not previous values. This PR stores previous values so they will preserved across restarts. I'm not sure I like this behavior because showing default values seems simpler and safer to me. Previous values may just have been set temporarily and may have never actually worked, and it adds some code complexity to store them. This PR is one way of resolving #596. Other solutions are possible and could be implemented as alternatives. ACKs for top commit: hebasto: ACK 9d3127b, tested on Ubuntu 22.04. vasild: ACK 9d3127b jarolrod: tACK 9d3127b Tree-SHA512: 1778d1819443490c880cfd5c1711d9c5ac75ea3ee8440e2f0ced81d293247163a78ae8aba6027215110aec6533bd7dc6472aeead6796bfbd51bf2354e28f24a9

Currently if a you choose a non-default datadir in the GUI intro screen, the datadir is ignored by CLI tools. This means `bitcoin-cli` and `bitcoin-wallet` will try to use the wrong datadir and show errors if they are called without a -datadir arguments, and `bitcoind` will appear to work but use a different datadir, not loading the same wallets and settings, and downloading blocks into the wrong place. There are also more subtle inconsistencies between GUI and CLI selection of datadirs such as bitcoin#27273 where GUI might ignore a datadir= line in a bitcoin.conf that CLI tools would apply. This PR gets rid of inconsistencies between GUI and CLI tools and makes them use the same datadir setting by default. It is followup to bitcoin-core/gui#602 which made GUI and CLI tools use the same `-dbcache`, `-par`, `-spendzeroconfchange`, `-signer`, `-upnp`, `-natpmp`, `-listen`, `-server`, `-prune`, `-proxy`, `-onion`, and `-lang` settings as long as they loaded the same datadir. The reason for GUI and CLI tools using inconsistent datadirs, is that GUI stores the datadir path in a `strDataDir` field in `.config/Bitcoin/Bitcoin-Qt.conf`[^1] which CLI tools ignore. This PR changes the GUI to instead store the datadir path at the default datadir location `~/.bitcoin`[^2] as a symlink that CLI tools will already follow, or as a text file if the filesystem does not support creating symlinks. If upgrading from a previous version of the GUI and there is only a GUI datadir, the `strDataDir` setting will be automatically migrated to a symlink so CLI tools will start using it as well. If CLI and GUI tools are currently using different default datadirs, the GUI will show a prompt allowing either of the datadirs to be loaded and optionally set as the common default going forward.

This was referenced May 19, 2022

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

Merged

refactor: Add OptionsModel getOption/setOption methods #600

Merged

ryanofsky changed the title ~~Unify bitcoin-qt and bitcoind persistent settings #15936~~ Unify bitcoin-qt and bitcoind persistent settings May 19, 2022

This was referenced May 20, 2022

Enable users to configure their monospace font specifically #497

Merged

Do not require restart if overridden option is modified #440

Closed

Peers window: Show direction in a new column, with clearer icon #363

Closed

ryanofsky force-pushed the pr/qtsopt branch from db64d21 to 60a9db7 Compare May 20, 2022 10:11

ryanofsky mentioned this pull request May 20, 2022

refactor: Pass interfaces::Node references to OptionsModel constructor #601

Merged

vasild approved these changes May 20, 2022

View reviewed changes

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/qtsopt branch from 60a9db7 to 9ae2f6f Compare May 20, 2022 18:06

ryanofsky commented May 20, 2022

View reviewed changes

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request May 20, 2022

Add settings.json prune-prev, proxy-prev, onion-prev settings #603

Merged

promag reviewed May 21, 2022

View reviewed changes

DrahtBot added the Needs rebase label May 22, 2022

ryanofsky force-pushed the pr/qtsopt branch from 9ae2f6f to 37682ee Compare May 23, 2022 19:05

DrahtBot removed the Needs rebase label May 23, 2022

DrahtBot added the Needs rebase label May 24, 2022

vasild approved these changes May 24, 2022

View reviewed changes

ryanofsky and others added 3 commits May 26, 2022 11:05

Migrate -par setting from QSettings to settings.json

a7ef6d5

Migrate -spendzeroconfchange and -signer settings from QSettings to s…

1dc4fc2

…ettings.json

DrahtBot removed the Needs rebase label May 26, 2022

vasild approved these changes May 26, 2022

View reviewed changes

hebasto reviewed May 26, 2022

View reviewed changes

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/qtsopt branch from 689dd65 to ac5fb01 Compare May 31, 2022 20:00

ryanofsky commented May 31, 2022

View reviewed changes

vasild approved these changes Jun 3, 2022

View reviewed changes

hebasto reviewed Jun 4, 2022

View reviewed changes

ryanofsky force-pushed the pr/qtsopt branch from ac5fb01 to bc6a562 Compare June 6, 2022 15:55

ryanofsky commented Jun 6, 2022

View reviewed changes

vasild approved these changes Jun 7, 2022

View reviewed changes

furszy reviewed Jun 8, 2022

View reviewed changes

furszy reviewed Jun 9, 2022

View reviewed changes

ryanofsky force-pushed the pr/qtsopt branch from bc6a562 to e47c6c7 Compare June 9, 2022 19:38

ryanofsky commented Jun 9, 2022

View reviewed changes

furszy approved these changes Jun 10, 2022

View reviewed changes

hebasto approved these changes Jun 12, 2022

View reviewed changes

hebasto merged commit 37633d2 into bitcoin-core:master Jun 12, 2022

hebasto mentioned this pull request Jun 12, 2022

Sync with the main repo bitcoin-core/gui-qml#131

Merged

furszy mentioned this pull request Jun 12, 2022

Reset options, notify user about backup creation #617

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 13, 2022

Merge bitcoin-core/gui#602: Unify bitcoin-qt and bitcoind persistent …

f5f15ad

ryanofsky mentioned this pull request Mar 15, 2023

Bitcoin ignores datadir and blocksdir parameter in .conf bitcoin/bitcoin#27246

Open

1 task

ryanofsky mentioned this pull request Apr 3, 2023

Make GUI and CLI tools use the same datadir bitcoin/bitcoin#27409

Draft

bitcoin-core locked and limited conversation to collaborators Jun 12, 2023

		// This handles exceptions thrown by univalue that can happen if
		// settings in settings.json don't have the expected types.

		InitError(error);
		QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));

		#include <validation.h> // For DEFAULT_SCRIPTCHECK_THREADS
		#include <wallet/wallet.h> // For DEFAULT_SPEND_ZEROCONF_CHANGE

Unify bitcoin-qt and bitcoind persistent settings #602

Unify bitcoin-qt and bitcoind persistent settings #602

Conversation

ryanofsky commented May 19, 2022 • edited Loading

DrahtBot commented May 20, 2022 • edited Loading

Conflicts

ryanofsky commented May 20, 2022 • edited Loading

vasild left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented May 20, 2022

Rspigler commented May 21, 2022

promag left a comment

Choose a reason for hiding this comment

Rspigler commented May 22, 2022

ryanofsky commented May 23, 2022

ryanofsky commented May 23, 2022

vasild left a comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

hebasto commented May 26, 2022

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

hebasto Jun 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

furszy Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

furszy left a comment • edited Loading

Choose a reason for hiding this comment

furszy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

ryanofsky commented May 19, 2022 •

edited

Loading

DrahtBot commented May 20, 2022 •

edited

Loading

ryanofsky commented May 20, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading

hebasto Jun 4, 2022 •

edited

Loading

furszy Jun 8, 2022 •

edited

Loading

furszy Jun 8, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

furszy Jun 9, 2022 •

edited

Loading

furszy Jun 9, 2022 •

edited

Loading