Prompt to reset settings when settings.json cannot be read #379

ryanofsky commented at 7:17 PM on July 8, 2021: contributor

Currently the GUI shows confusing error messages when settings.json can't be read or written on startup. This causes the unrecoverable read error described in bitcoin/bitcoin#21340 and write error described bitcoin/bitcoin#21974. Current error read message looks like:

current

This PR tries to clarify the error dialog, and adds an option to just clear the settings and reset them to default:

new-read-error new-read-details

Additionally the PR also shows a slightly better error message when there is an error trying to write the settings file. This error probably should occur less frequently, but it is easy to improve, and it should be good to make the write error consistent with the read error. The new write error dialog looks like:

new-write-error

new-write-details

gui: Prompt to reset settings when settings.json cannot be read

Fixes bitcoin/bitcoin#21340

Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>

1ee6d0b01a

ryanofsky commented at 7:30 PM on July 8, 2021: contributor

Note: Screenshots above are from linux and I think the button placement Qt chooses there is pretty strange. I think it's weird that the default button is on the left in the read error dialog, but the default button is on the right in the write error dialog. I also think it's weird that the show details toggle is between two other buttons that dismiss the dialog. None of this is very bad, but I'm hoping maybe it looks a little better on other platforms, since it makes probably doesn't make sense to hardcode a button order if conventions vary across platforms.

Also, the following patch is an easy way to test the PR, and it's what I used to make the screenshots:

--- a/src/util/settings.cpp
+++ b/src/util/settings.cpp
@@ -65,7 +65,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
     if (!file.is_open()) return true; // Ok for file not to exist.
 
     SettingsValue in;
-    if (!in.read(std::string{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()})) {
+    if (1 | !in.read(std::string{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()})) {
         errors.emplace_back(strprintf("Unable to parse settings file %s", path.string()));
         return false;
     }
@@ -102,7 +102,7 @@ bool WriteSettings(const fs::path& path,
     }
     fsbridge::ofstream file;
     file.open(path);
-    if (file.fail()) {
+    if (1 | file.fail()) {
         errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", path.string()));
         return false;
     }

ryanofsky force-pushed on Jul 13, 2021

ryanofsky commented at 7:24 PM on July 13, 2021: contributor

Updated cf472050f232dfac0f923ea99993fb7b607b955c -> ebfd619e3cfef148fd0e93ca00505d635636ac34 (pr/badset.1 -> pr/badset.2, compare) to fix appveyor error https://ci.appveyor.com/project/DrahtBot/bitcoin-core-gui/builds/39921403

hebasto added the label UX on Jul 14, 2021

in src/qt/bitcoin.cpp:166 in ebfd619e3c outdated

 159 | +    if (!gArgs.ReadSettingsFile(&errors)) {
 160 | +        bilingual_str error = _("Settings file could not be read");
 161 | +        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
 162 | +
 163 | +        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
 164 | +        messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));

jarolrod commented at 7:12 AM on July 23, 2021:

could add a translator comment:

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index c72a5ecdd..bc9da65ab 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -161,6 +161,8 @@ static bool InitSettings()
         InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
 
         QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
+        /*: Explanatory text shown on startup when the settings file cannot be read.
+            Prompts user to make a choice between resetting or aborting. */
         messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
         messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
         messagebox.setTextFormat(Qt::PlainText);

ryanofsky commented at 10:40 PM on July 29, 2021:

re: #379 (review)

could add a translator comment:

Added, thanks!

luke-jr commented at 8:52 PM on July 30, 2021:

I don't see the logic to actually reset the settings to defaults.

ryanofsky commented at 10:23 PM on July 30, 2021:

I don't see the logic to actually reset the settings to defaults.

It happens implicitly because if reading the settings file fails, the file that is written on line 181 will be empty.

luke-jr commented at 8:58 PM on July 31, 2021:

But if it fails part way into reading the settings, won't the ones it already read remain?

ryanofsky commented at 7:18 PM on August 5, 2021:

re: #379 (review)

But if it fails part way into reading the settings, won't the ones it already read remain?

To my surprise, it is actually technically possible for this to happen! And it needs to be fixed outside the GUI code so I filed a bug in the main repo https://github.com/bitcoin/bitcoin/issues/22638. Practically speaking, the scenario where this would happen is nearly impossible and if it did happen, the effect would just be settings kept instead of discarded. So I think there's no need for this PR to be held up by the bug, but it is a good catch!

in src/qt/bitcoin.cpp:189 in ebfd619e3c outdated

 179 | +    if (!gArgs.WriteSettingsFile(&errors)) {
 180 | +        bilingual_str error = _("Settings file could not be written");
 181 | +        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
 182 | +
 183 | +        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
 184 | +        messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings."));

jarolrod commented at 7:18 AM on July 23, 2021:

could add translator comment:

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index c72a5ecdd..b29fb5dde 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -181,6 +181,9 @@ static bool InitSettings()
         InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
 
         QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
+        /*: Explanatory text shown on startup when the settings file could not be written.
+            Prompts user to check that we have the ability to write to the file.
+            Explains that the user has the option of running without a settings file.*/
         messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings."));
         messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
         messagebox.setTextFormat(Qt::PlainText);

ryanofsky commented at 10:42 PM on July 29, 2021:

re: #379 (review)

could add translator comment:

Added, thanks!

jarolrod commented at 7:37 AM on July 23, 2021: member

ACK ebfd619e3cfef148fd0e93ca00505d635636ac34

Tested functionality on macOS 11.3, Arch Linux and cross compiled to Windows 10. This is a nice change and makes these errors seem less scary. The nit on translator comments can be addressed in a follow-up, non-blocker.

Tested the error message would show up by applying this patch: #379 (comment)

Tested that I could abort and reset manually changing the settings file with the implemented buttons. Presentation of the read error was done by passing in a data directory that already had a settings file. The presentation of the write error was tested by passing in a new and empty data directory.

macOS 11.3

Read Error

Presentation	Expand Details

Write Error:

Presentation	Expand Details

Arch Linux

Read Error

Presentation	Expand Details

Write Error:

Presentation	Expand Details

Windows 10

Read Error

Presentation	Expand Details

Write Error:

Presentation	Expand Details

shaavan commented at 3:49 PM on July 27, 2021: contributor

tACK ebfd619e3cfef148fd0e93ca00505d635636ac34

Successfully compiled and tested on Ubuntu 20.04 I like the idea of elaborating the problem, and it is an improvement compared to the error message of the master branch. Because:

Read Error: Now, the user is aware of the issue and can either continue to GUI or abort and close the program. Write Error: The error message had essential details to understand the problem and do something about it.

But I would like to suggest some elaborations to make the user’s work more straightforward if they face this issue.

Some recommended further elaborations:

In case of an error in writing the file or failure in overwriting, then.
- If settings.json file:
  - does not exist: create a new file with default values
  - exists: try deleting the file and make a new file with default values
- Abort
In case of not able to delete the file (above solution):
- Abort

Talkless commented at 6:45 PM on July 28, 2021: none

I've tried reproducing using permissions like this:

sudo chown root:root /tmp/datadir/regtest/settings.json
sudo chmod a-rwx  /tmp/datadir/regtest/settings.json

bitcoin-qt just silently (both master and PR) overwritten that file after experiencing EACCES (Permission denied):

$ strace -efile src/qt/bitcoin-qt -regtest -datadir=/tmp/datadir 2>&1 | fgrep settings.json
openat(AT_FDCWD, "/tmp/datadir/regtest/settings.json", O_RDONLY) = -1 EACCES (Permission denied)
openat(AT_FDCWD, "/tmp/datadir/regtest/settings.json.tmp", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
rename("/tmp/datadir/regtest/settings.json.tmp", "/tmp/datadir/regtest/settings.json") = 0

(it's probably allowed to rename because process is still owner of the directory).

Not saying it's this PR's issue, just interesting find...

Talkless commented at 7:09 PM on July 28, 2021: none

I've created AppArmor profile /etc/apparmor.d/bitcoin that attaches to the built binaries (master and pr) and allows (almost) everything, except access to the settings.json:

profile bitcoin /home/vincas/code/bitcoin/379-bitcoin-core-gui.git/_build_{master,pr}/src/qt/bitcoin-qt {

    /{,**} rwmklux, # allow reading/writing/locking/mmaping/linking/executing... everything

    # allow various other kernel api stuff:
    capability,
    network,
    dbus,
    ptrace,
    signal,
    unix,

    # ..except this one
    audit deny /tmp/datadir/regtest/settings.json rwmkl,
}

I do get dialog Failed renaming settings file... but the fact that reading has failed was "skipped":

$ strace -efile src/qt/bitcoin-qt -regtest -datadir=/tmp/datadir 2>&1 | fgrep settings.json
openat(AT_FDCWD, "/tmp/datadir/regtest/settings.json", O_RDONLY) = -1 EACCES (Permission denied)
openat(AT_FDCWD, "/tmp/datadir/regtest/settings.json.tmp", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
rename("/tmp/datadir/regtest/settings.json.tmp", "/tmp/datadir/regtest/settings.json") = -1 EACCES (Permission denied)
- Failed renaming settings file /tmp/datadir/regtest/settings.json.tmp to /tmp/datadir/regtest/settings.json

audit.log shows that AppArmor denied reading and then writing:

type=AVC msg=audit(1627499280.531:631): apparmor="DENIED" operation="open" profile="bitcoin" name="/tmp/datadir/regtest/settings.json" pid=39084 comm="bitcoin-qt" requested_mask="r" denied_mask="r" fsuid=1000 ouid=1000FSUID="vincas" OUID="vincas"
type=AVC msg=audit(1627499280.531:632): apparmor="DENIED" operation="rename_dest" profile="bitcoin" name="/tmp/datadir/regtest/settings.json" pid=39084 comm="bitcoin-qt" requested_mask="wc" denied_mask="wc" fsuid=1000 ouid=1000FSUID="vincas" OUID="vincas"

ryanofsky force-pushed on Jul 29, 2021

ryanofsky commented at 11:11 PM on July 29, 2021: contributor

Thanks for the reviews and testing!

Updated ebfd619e3cfef148fd0e93ca00505d635636ac34 -> 1ee6d0b01a517893967379677029fb5417978247 (pr/badset.2 -> pr/badset.3, compare) just adding jarolrod's suggested translator comments verbatim.

This PR may be close to ready for merge.

re: #379#pullrequestreview-716093347

Some recommended further elaborations:

Thanks, I think I basically agree with the suggestions. Would encouraging filing new issues if there are specific problems that aren't being addressed. This PR clarifies the write error somewhat, but after this PR there are two more issues that need to be resolved on settings write errors:

bitcoin/bitcoin#21974 - when settings file doesn't exist and writing a new one fails, it can be indicate a problem with the datadir, like maybe it was set to an external location that is no longer accessible, so a more convenient option to choose a new datadir could be provided
bitcoin/bitcoin#22571 - valid but inaccessible settings.json files could be overwritten in some cases due to read access errors

re: #379 (comment)

I've tried reproducing using permissions like this: [...]

re: #379 (comment)

I've created AppArmor profile /etc/apparmor.d/bitcoin that attaches to the built binaries (master and pr) and allows (almost) everything, except access to the settings.json: [...]

Great find! Both of these posts are describing the same bug and I filed a new issue https://github.com/bitcoin/bitcoin/issues/22571. The fix needs to stop ignoring EACCES (Permission denied) and other read errors, instead of treating them the same as ENOENT (No such file or directory)

luke-jr changes_requested

jarolrod commented at 1:09 AM on August 3, 2021: member

ACK 1ee6d0b01a517893967379677029fb5417978247

Changes since my last review are only translator comments, thanks for adding them 🥃

hebasto commented at 11:42 AM on August 5, 2021: member

Concept ACK.

in src/qt/bitcoin.cpp:167 in 1ee6d0b01a

 162 | +
 163 | +        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
 164 | +        /*: Explanatory text shown on startup when the settings file cannot be read.
 165 | +            Prompts user to make a choice between resetting or aborting. */
 166 | +        messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
 167 | +        messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));

hebasto commented at 12:30 PM on August 5, 2021:

Usually, we use a detailed text to show untranslated error messages to users for convenient googling and referencing (see https://github.com/bitcoin/bitcoin/pull/16224). Now it only contains a list of errors. Could an untranslated error message be added here as well?

ryanofsky commented at 6:47 PM on August 5, 2021:

re: #379 (review)

Usually, we use a detailed text to show untranslated error messages to users for convenient googling and referencing (see bitcoin/bitcoin#16224). Now it only contains a list of errors. Could an untranslated error message be added here as well?

Just to clarify what's being discussed, errors is untranslated, and the error messages are self contained, so this dialog is consistent with other dialogs in using translated text in the main section, and untranslated text in the details section. The inconsistency is just in the prefixing.

On the prefixing, I do think it would be reasonable for a followup PR to append BitcoinGUI::tr("Original message:") + "\n" + QString::fromStdString(error.original) if error.original != error.translated to be a consistent with ThreadSafeMessageBox but I think it would be better to do it in a separate PR because:

I think this logic should go in a common util function so it is not duplicated. Trying to do this and refactor the ThreadSafeMessageBox code would expand the scope of this PR and complicate it when I think this PR is already a strict improvement and it's been reviewed and tested by people.
In this specific place, just because of the fact that ReadSettings errors are fully self-describing, I think adding the extra text would actually make the UX slightly worse (more repetitive and confusing) so more discussion in a followup PR about how to have consistency and clarity in these dialogs at same time could be a good idea.

ryanofsky commented at 7:40 PM on August 5, 2021: contributor

Thanks for reviews! There's good discussion from luke and hebasto about issues that would be good to follow up on (https://github.com/bitcoin-core/gui/pull/379#discussion_r683406901, #379 (review)), but to me it looks like with the testing and review this PR has had so far, it could be ready for merge.

I'd like this PR to remain simple and focused on the basic UX improvement of displaying settings.json read errors. I'd like future PRs to address settings write errors, settings read implementation bugs, and better distinctions between untranslated "details" and untranslated "original messages" as other improvements that can build on top of this improvement.

ghost commented at 8:39 PM on August 5, 2021: none

Concept ACK. Will test this weekend.

Zero-1729 commented at 11:01 PM on August 5, 2021: contributor

ACK 1ee6d0b01a517893967379677029fb5417978247

hebasto approved

hebasto commented at 3:37 PM on August 6, 2021: member

ACK 1ee6d0b01a517893967379677029fb5417978247, tested on Linux Mint 20.2 (Qt 5.12.8).

hebasto merged this on Aug 6, 2021

hebasto closed this on Aug 6, 2021

in src/qt/bitcoin.cpp:189 in 1ee6d0b01a

 184 | +
 185 | +        QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
 186 | +        /*: Explanatory text shown on startup when the settings file could not be written.
 187 | +            Prompts user to check that we have the ability to write to the file.
 188 | +            Explains that the user has the option of running without a settings file.*/
 189 | +        messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings."));

hebasto commented at 7:13 PM on August 6, 2021:

typo: occured ==> occurred

Addressed in https://github.com/bitcoin/bitcoin/pull/22653.

sidhujag referenced this in commit 1f3f166be5 on Aug 6, 2021

ghost commented at 8:38 PM on August 7, 2021: none

Tested on Windows with src/util/settings.cpp from https://github.com/bitcoin/bitcoin/pull/22591/

Post merge tACK https://github.com/bitcoin-core/gui/pull/379/commits/1ee6d0b01a517893967379677029fb5417978247

nit:

-Settings file could not be read
+Failed to read settings file

-Do you want to reset settings to default values, or to abort without making any changes?
+Do you want to reset settings to default values, or abort without making any changes?

nit:

DetailedText can be better if it was something like this:

File: E:\regtest\settings.json
Error: Please check permissions

If DetailedText is changed above, can follow the same format for write error as well.

fanquake referenced this in commit c3545a7396 on Aug 11, 2021

MarcoFalke referenced this in commit 91e07cc50d on Sep 5, 2021

sidhujag referenced this in commit 64186eccd8 on Sep 7, 2021

PastaPastaPasta referenced this in commit 8d07123fa7 on Jul 17, 2022

PastaPastaPasta referenced this in commit befba8f2c3 on Aug 30, 2022

PastaPastaPasta referenced this in commit 657bfaebcd on Aug 30, 2022

PastaPastaPasta referenced this in commit ab4405258e on Sep 3, 2022

bitcoin-core locked this on Sep 4, 2022