init: handle empty settings file gracefully

[furszy]

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]

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]

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

[furszy]

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]

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]

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."

[shaavan]

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:

1. 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.
2. 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.
3. 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]

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]

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:

1. 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.
2. 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.
3. 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 (review).

Thus I proposed a different solution for non-interactive users who are manually modifying the file. See #29144 (review).
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]

Thus I proposed a different solution for non-interactive users who are manually modifying the file. See #29144 (review).
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.

1. 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.

2. 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]

re: 725a1fc 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 bitcoin-core/gui#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:

{
  "_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."
}

[furszy]

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 bitcoin-core/gui#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:

{
  "_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."
}

Sounds great. Thanks @ryanofsky!
Updated per feedback.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

[furszy]

Cool notification. The CI should be happy now.

[ryanofsky]

Code review ACK 6f3183e. This seems like a helpful change.

But It does seem unfortunate that both of the commits here make tests more verbose and repetitive. And the tests even before this PR were probably unnecessarily fragile. But I'm not sure I see a better way to write the tests, or a way to avoid the repetition of warning and error messages. If anybody has any ideas on how to improve the tests here, I'd be interested to know.

[shaavan]

Thanks for the update, @furszy
The new error message and the settings.json warnings are very precise in their details and I believe will be better in guiding the user in case of error, and refraining them from modifying settings without being cautious.

I just have one suggestion I want to add.

[furszy]

But It does seem unfortunate that both of the commits here make tests more verbose and repetitive. And the tests even before this PR were probably unnecessarily fragile. But I'm not sure I see a better way to write the tests, or a way to avoid the repetition of warning and error messages. If anybody has any ideas on how to improve the tests here, I'd be interested to know.

This is not a small change, but.. we could work on a Resources class that is auto-generated during the compile phase. It would store all static error and warning message strings in conjunction with an auto-generated ID for each of them. Allowing us to access messages via their IDs instead of using the strings directly. The ID names could follow a certain layering pattern; e.g. accessing a init error could be: R.getStr(R.errors.init.settings_invalid_json). (similar to how Android organizes app resources).

Then, to prevent code and string repetition inside functional and unit tests, the Resources class generation script could output C++ and Python compatible code.

@ryanofsky, just an idea. Not sure if it worth the efforts.

[furszy]

Rebased to cleanup spurious CI failure.

[hebasto]

ACK d664eab, tested on Ubuntu 23.10.

nano-nit: There are some complaints from clang-format-diff.py.

[furszy]

nano-nit: There are some complaints from clang-format-diff.py.

Done.

[hebasto]

re-ACK e901404.

[ryanofsky]

Code review ACK e901404. Just whitespace formatting changes and shortening a test string literal since last review

[shaavan]

Code review ACK e901404

After our chat in this thread, I believe this PR is good to go!

There are just a few whitespace tweaks since my last comment, and they look great too.

[luke-jr]

Should we perhaps add (and recommend, for non-GUI users), a -resetguisettings for non-GUI?

[achow101]

ACK e901404

[furszy]

Should we perhaps add (and recommend, for non-GUI users), a -resetguisettings for non-GUI?

Sure. We could introduce a -resetsettings in a follow-up.

[fanquake]

So now we will (always?) log this on startup:

2024-01-23T22:48:12.205684Z [init] Setting file arg: _warning_ = "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."

Which seems a bit pointless/noisy, and could maybe even be confusing to users?

[furszy]

So now we will (always?) log this on startup:

2024-01-23T22:48:12.205684Z [init] Setting file arg: _warning_ = "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."

Which seems a bit pointless/noisy, and could maybe even be confusing to users?

Agree. Good catch. Fixed in #29301.