Skip to content

Unify bitcoin-qt and bitcoind persistent settings #602

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

Merged
merged 10 commits into from
Jun 12, 2022
Merged
15 changes: 15 additions & 0 deletions doc/release-notes-15936.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
GUI changes
-----------

Configuration changes made in the bitcoin GUI (such as the pruning setting,
proxy settings, UPNP preferences) are now saved to `<datadir>/settings.json`
file rather than to the Qt settings backend (windows registry or unix desktop
config files), so these settings will now apply to bitcoind, instead of being
ignored.

Also, the interaction between GUI settings and `bitcoin.conf` settings is
simplified. Settings from `bitcoin.conf` are now displayed normally in the GUI
settings dialog, instead of in a separate warning message ("Options set in this
dialog are overridden by the configuration file: -setting=value"). And these
settings can now be edited because `settings.json` values take precedence over
`bitcoin.conf` values.
27 changes: 23 additions & 4 deletions src/qt/bitcoin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,26 @@ void BitcoinApplication::createPaymentServer()
}
#endif

void BitcoinApplication::createOptionsModel(bool resetSettings)
bool BitcoinApplication::createOptionsModel(bool resetSettings)
{
optionsModel = new OptionsModel(node(), this, resetSettings);
optionsModel = new OptionsModel(node(), this);
if (resetSettings) {
optionsModel->Reset();
}
bilingual_str error;
if (!optionsModel->Init(error)) {
fs::path settings_path;
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();
}
Comment on lines +271 to +276
Copy link
Member

@hebasto hebasto Jun 4, 2022

Choose a reason for hiding this comment

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

What are expected scenarios which run this code?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

InitError(error);
QMessageBox::critical(nullptr, PACKAGE_NAME, QString::fromStdString(error.translated));
Comment on lines +277 to +278
Copy link
Member

@furszy furszy Jun 8, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@furszy furszy Jun 8, 2022

Choose a reason for hiding this comment

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

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

return false;
}
return true;
}

void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
Expand Down Expand Up @@ -327,7 +344,7 @@ void BitcoinApplication::parameterSetup()

void BitcoinApplication::InitPruneSetting(int64_t prune_MiB)
{
optionsModel->SetPruneTargetGB(PruneMiBtoGB(prune_MiB), true);
optionsModel->SetPruneTargetGB(PruneMiBtoGB(prune_MiB));
}

void BitcoinApplication::requestInitialize()
Expand Down Expand Up @@ -641,7 +658,9 @@ int GuiMain(int argc, char* argv[])
app.createNode(*init);

// Load GUI settings from QSettings
app.createOptionsModel(gArgs.GetBoolArg("-resetguisettings", false));
if (!app.createOptionsModel(gArgs.GetBoolArg("-resetguisettings", false))) {
return EXIT_FAILURE;
}

if (did_show_intro) {
// Store intro dialog settings other than datadir (network specific)
Expand Down
2 changes: 1 addition & 1 deletion src/qt/bitcoin.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class BitcoinApplication: public QApplication
/// parameter interaction/setup based on rules
void parameterSetup();
/// Create options model
void createOptionsModel(bool resetSettings);
[[nodiscard]] bool createOptionsModel(bool resetSettings);
/// Initialize prune setting
void InitPruneSetting(int64_t prune_MiB);
/// Create main window
Expand Down
2 changes: 1 addition & 1 deletion src/qt/forms/optionsdialog.ui
Original file line number Diff line number Diff line change
Expand Up @@ -896,7 +896,7 @@
<item>
<widget class="QLabel" name="overriddenByCommandLineInfoLabel">
<property name="text">
<string>Options set in this dialog are overridden by the command line or in the configuration file:</string>
<string>Options set in this dialog are overridden by the command line:</string>
</property>
<property name="textFormat">
<enum>Qt::PlainText</enum>
Expand Down
5 changes: 0 additions & 5 deletions src/qt/optionsdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include <QIntValidator>
#include <QLocale>
#include <QMessageBox>
#include <QSettings>
#include <QSystemTrayIcon>
#include <QTimer>

Expand Down Expand Up @@ -56,10 +55,6 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
#ifndef USE_NATPMP
ui->mapPortNatpmp->setEnabled(false);
#endif
connect(this, &QDialog::accepted, [this](){
QSettings settings;
model->node().mapPort(settings.value("fUseUPnP").toBool(), settings.value("fUseNatpmp").toBool());
});

ui->proxyIp->setEnabled(false);
ui->proxyPort->setEnabled(false);
Expand Down
Loading