init: handle empty settings file gracefully

furszy commented at 10:52 pm on December 26, 2023: member

Small and simple issue reported here.

Improving a confusing situation reported by users who did not understand why a settings parsing error occurred when the file was empty and did not know how to solve it.

Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content. In both scenarios, the ‘Unable to parse settings file’ error does not help the user move forward.

DrahtBot commented at 10:52 pm on December 26, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, ryanofsky, shaavan, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

0xB10C commented at 11:43 am on December 27, 2023: contributor

see previous discussion in #23096, #22591, and #21340 (comment)

furszy commented at 1:40 pm on December 27, 2023: member

see previous discussion in #23096, #22591, and #21340 (comment)

Hmm, okay, agree. Thanks. It would probably be useful to introduce support for comments. This way, we can write something at the beginning of the file, ensuring that users and other software developers don’t clean it up manually, thinking that it will be regenerated automatically. Do you know if something like this has been proposed before? @0xB10C.

shaavan commented at 12:58 pm on December 28, 2023: contributor

I agree with the reasoning ryanofsky has given here.

A zero-size settings file is a corrupt settings file.

But going through the issue mentioned in the description. I don’t think Unable to parse settings file /data/.bitcoin/settings.json is a very helpful message when it comes to bitcoind detecting an empty settings.json file.

I think we should still handle the case of empty file separately. But instead of passing the file as valid, we should rather print a very clear error message, pointing the user to the reason for failure and possible remedy. This will definitely better the User experience for the noderunners.

furszy commented at 1:48 pm on December 28, 2023: member

I think we should still handle the case of empty file separately. But instead of passing the file as valid, we should rather print a very clear error message, pointing the user to the reason for failure and possible remedy. This will definitely better the User experience for the noderunners.

For GUI interactive users, this isn’t an issue. We show a dialog that allows continuing when a parsing error is found (still, we might want to be clearer about the fact that they could be disabling Tor and/or other important configurations by allowing this).

For non-interactive ones, people or other softwares, I’m inclined to solve this situation by adding a comment at the top of the file. Something like “This is an auto-generated JSON-formatted file. Please refrain from modifying it unless you are certain about the potential consequences.”

in src/common/settings.cpp:86 in 725a1fc7a7 outdated

79@@ -80,7 +80,9 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
80     }
81 
82     SettingsValue in;
83-    if (!in.read(std::string{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()})) {
84+    std::string content{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()};
85+    if (content.empty()) return true; // Ok for empty file
86+    if (!in.read(content)) {
87         errors.emplace_back(strprintf("Unable to parse settings file %s", fs::PathToString(path)));

anibilthare commented at 7:42 am on December 29, 2023:

I agree with @shaavan here . What if we simply modify the error message here and keep rest of the code as is to be more user friendly. Referring to #23096, #22591, and #21340 (comment) as mentioned earlier, following change can be added to the error message

0        errors.emplace_back(strprintf("Unable to parse settings file %s, the file may be corrupt or empty(Consider deleting the file if it is empty). If the settings file is getting truncated automatically, check if there is suffiecient disk space available.", fs::PathToString(path)));

shaavan commented at 11:39 am on December 29, 2023:

I see your point here. But wording like this provides a very unclear reason for failure. Don’t you think we would be throwing users in too many directions here?

anibilthare commented at 12:24 pm on December 29, 2023:

Can you please point out exactly what part of these statements might confuse the user? Although we’ll be asking the user to check for 2 things in the comment but I believe they are pretty straight forward, right?

Is there any space left on the disk.
Is settings.json empty or not?

If none of the above conditions hold true and user is still seeing the issue then we are indicating the possibility of corruption of the settings file itself.

furszy commented at 1:52 pm on December 29, 2023:

Core does not works for umbrel @1440000bytes, I did not know about umbrel existence until you pointed it out.

The PR tries to improve a confusing situation reported by users who did not understand why a settings parsing error occurred when the file was empty and did not know how to solve it. Still, I’m no longer in favor of the current approach. And proposed a different path #29144#pullrequestreview-1798109497.

anibilthare changes_requested

shaavan commented at 11:28 am on December 29, 2023: contributor

I get your point, @furszy, and I can see your approach helping with smoother handling of the “file empty” situation.

However, pointing back again to ryanofsky comment

A zero-size settings file is a corrupt settings file.

The interpretation I draw from this is if a file is empty (and hence corrupted once) there is no telling what could be the reason for that happening or whether such corruption could happen again. So, I believe being very critical and clear with the handling will be better for the following reasons:

For crucial backend software, such as bitcoind, it seems negligent to not properly inform the user that their settings file has been lost to corruption.
It seems to me that a regular user won’t tinker with their settings file regularly, so any kind of auto-generated message in the settings.json file loses its relevance there.
Finally, say a user sets some custom settings once and later loses the settings to corruption (=> file now empty); it seems an improper approach to me for bitcoind to return to default settings without any kind of proper log or error to the user.

Also, a gentle ping to @ryanofsky to confirm if my interpretation of the situation is correct, and also to get his take on this PR.

kristapsk commented at 11:59 am on December 29, 2023: contributor

It would probably be useful to introduce support for comments. This way, we can write something at the beginning of the file, ensuring that users and other software developers don’t clean it up manually, thinking that it will be regenerated automatically.

This will not help if cause was full disk. #21340 (comment)

furszy commented at 12:53 pm on December 29, 2023: member

I get your point, @furszy, and I can see your approach helping with smoother handling of the “file empty” situation.

However, pointing back again to ryanofsky comment

A zero-size settings file is a corrupt settings file.

The interpretation I draw from this is if a file is empty (and hence corrupted once) there is no telling what could be the reason for that happening or whether such corruption could happen again. So, I believe being very critical and clear with the handling will be better for the following reasons:

For crucial backend software, such as bitcoind, it seems negligent to not properly inform the user that their settings file has been lost to corruption.

It seems to me that a regular user won’t tinker with their settings file regularly, so any kind of auto-generated message in the settings.json file loses its relevance there.

Finally, say a user sets some custom settings once and later loses the settings to corruption (=> file now empty); it seems an improper approach to me for bitcoind to return to default settings without any kind of proper log or error to the user.

Also, a gentle ping to @ryanofsky to confirm if my interpretation of the situation is correct, and also to get his take on this PR.

There is no discussion over that. I agreed with all previous arguments against the current implementation since my first comment on this PR. See #29144#pullrequestreview-1797199209.

Thus I proposed a different solution for non-interactive users who are manually modifying the file. See #29144#pullrequestreview-1798109497. Stating that the file is JSON-formatted and auto-generated gives bitcoind users another hint over what they can and can’t do.

It would probably be useful to introduce support for comments. This way, we can write something at the beginning of the file, ensuring that users and other software developers don’t clean it up manually, thinking that it will be regenerated automatically.

This will not help if cause was full disk. #21340 (comment)

Thats a different problematic @kristapsk. The comments support feature goal is to prevent users’, and/or other third party softwares, manual mistakes. Not OS level corruption issues.

shaavan commented at 8:44 am on January 2, 2024: contributor

Thus I proposed a different solution for non-interactive users who are manually modifying the file. See #29144#pullrequestreview-1798109497. Stating that the file is JSON-formatted and auto-generated gives bitcoind users another hint over what they can and can’t do.

I see your point, @furszy

But I think my points still apply to creating an “auto-generated” file.

A regular user might not tinker with their settings file regularly and hence the “this file is auto-generated” might go unnoticed by the user essentially making the message irrelevant.
If a user had a custom set of settings and later loses them to corruption, they might not know that this has happened for a while if they are not given an “on-the-face” alert. And for critical software such as bitcoind, this seems to me a non-optimal behavior.

I think it’s a great idea to help the user with an auto-generated file in case of setting loss. But in the absence of an “on-the-face” alert, that change loses much of its significance.

ryanofsky commented at 5:04 pm on January 8, 2024: contributor

re: 725a1fc7a7dbb1153bec188eda2f17a217560352 I think it is important to treat an empty settings file as an error, because the most likely cause of an empty file is some kind of crash or corruption or disk full state, and it is safer to alert the user about the situation so they can fix it, than proceed as if the error didn’t happen and lose important settings that could affect security or stability.

I think the error message “Unable to parse settings file %s” could definitely be improved though. Would suggest maybe “Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, and can be fixed by removing the file, which will reset settings to default values.” This would probably work OK for both command line and GUI (since the GUI already offers the option to reset the settings see https://github.com/bitcoin-core/gui/pull/379).

As far as adding a comments to the settings file, I’m not sure it would necessarily help in the reported case from https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144, but it probably depends on whatever caused the settings file to be truncated or created empty in that case, and it could be a good idea to add comments to the file regardless. JSON doesn’t have a syntax for comments, it but would be possible to add an extra field like:

0{
1  "_comment_": "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."
2}