Merge bitcoin/bitcoin#29144: init: handle empty settings file gracefully

e901404 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de init: improve corrupted/empty settings file error msg (furszy)

Pull request description:

  Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).

  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.

ACKs for top commit:
  achow101:
    ACK e901404
  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

Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec

Author: achow101

src/common/settings.cpp

@@ -4,6 +4,10 @@
 
 #include <common/settings.h>
 
+#if defined(HAVE_CONFIG_H)
+#include <config/bitcoin-config.h>
+#endif
+
 #include <tinyformat.h>
 #include <univalue.h>
 #include <util/fs.h>
@@ -81,7 +85,9 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
 
     SettingsValue in;
     if (!in.read(std::string{std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()})) {
-        errors.emplace_back(strprintf("Unable to parse settings file %s", fs::PathToString(path)));
+        errors.emplace_back(strprintf("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.",
+                                      fs::PathToString(path)));
         return false;
     }
 
@@ -114,6 +120,13 @@ bool WriteSettings(const fs::path& path,
     std::vector<std::string>& errors)
 {
     SettingsValue out(SettingsValue::VOBJ);
+    // Add auto-generated warning comment only if it does not exist
+    if (!values.contains("_warning_")) {
+        out.pushKV("_warning_", strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node "
+                                          "is running, as any changes might be ignored or overwritten.",
+                                          PACKAGE_NAME));
+    }
+    // Push settings values
     for (const auto& value : values) {
         out.pushKVEnd(value.first, value.second);
     }

src/qt/test/optiontests.cpp

@@ -61,7 +61,11 @@ void OptionTests::migrateSettings()
     QVERIFY(!settings.contains("addrSeparateProxyTor"));
 
     std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
+    std::string default_warning = strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node "
+                                            "is running, as any changes might be ignored or overwritten.",
+                                            PACKAGE_NAME);
     QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n"
+        "    \"_warning_\": \""+ default_warning+"\",\n"
         "    \"dbcache\": \"600\",\n"
         "    \"listen\": false,\n"
         "    \"onion\": \"onion:234\",\n"

src/test/settings_tests.cpp

@@ -99,7 +99,9 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
     // Check invalid json not allowed
     WriteText(path, R"(invalid json)");
     BOOST_CHECK(!common::ReadSettings(path, values, errors));
-    std::vector<std::string> fail_parse = {strprintf("Unable to parse settings file %s", fs::PathToString(path))};
+    std::vector<std::string> fail_parse = {strprintf("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.",
+                                                     fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), fail_parse.begin(), fail_parse.end());
 }
 

test/functional/feature_settings.py

@@ -23,10 +23,11 @@ def run_test(self):
         settings = node.chain_path / "settings.json"
         conf = node.datadir_path / "bitcoin.conf"
 
-        # Assert empty settings file was created
+        # Assert default settings file was created
         self.stop_node(0)
+        default_settings = {"_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."}
         with settings.open() as fp:
-            assert_equal(json.load(fp), {})
+            assert_equal(json.load(fp), default_settings)
 
         # Assert settings are parsed and logged
         with settings.open("w") as fp:
@@ -48,12 +49,12 @@ def run_test(self):
 
         # Assert settings are unchanged after shutdown
         with settings.open() as fp:
-            assert_equal(json.load(fp), {"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]})
+            assert_equal(json.load(fp), {**default_settings, **{"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]}})
 
         # Test invalid json
         with settings.open("w") as fp:
             fp.write("invalid json")
-        node.assert_start_raises_init_error(expected_msg='Unable to parse settings file', match=ErrorMatch.PARTIAL_REGEX)
+        node.assert_start_raises_init_error(expected_msg='does not contain valid JSON. This is probably caused by disk corruption or a crash', match=ErrorMatch.PARTIAL_REGEX)
 
         # Test invalid json object
         with settings.open("w") as fp: