From 966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Jan 2024 09:54:49 -0300 Subject: [PATCH 1/2] init: improve corrupted/empty settings file error msg The preceding "Unable to parse settings file" message lacked the necessary detail and guidance for users on what steps to take next in order to resolve the startup error. Co-authored-by: Ryan Ofsky --- src/common/settings.cpp | 4 +++- src/test/settings_tests.cpp | 4 +++- test/functional/feature_settings.py | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/common/settings.cpp b/src/common/settings.cpp index 5761e8b321e..cbf794a7c62 100644 --- a/src/common/settings.cpp +++ b/src/common/settings.cpp @@ -81,7 +81,9 @@ bool ReadSettings(const fs::path& path, std::map& va SettingsValue in; if (!in.read(std::string{std::istreambuf_iterator(file), std::istreambuf_iterator()})) { - 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; } diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index fa8ceb8dd6b..41190b35798 100644 --- a/src/test/settings_tests.cpp +++ b/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 fail_parse = {strprintf("Unable to parse settings file %s", fs::PathToString(path))}; + std::vector 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()); } diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 1bacdd447a2..4175699152e 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -53,7 +53,7 @@ class SettingsTest(BitcoinTestFramework): # 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: From e9014042a6bed8c16cc9a31fc35cb709d4b3c766 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Jan 2024 10:25:08 -0300 Subject: [PATCH 2/2] settings: add auto-generated warning msg for editing the file manually Hopefully, refraining users from modifying the file unless they are certain about the potential consequences. Co-authored-by: Ryan Ofsky --- src/common/settings.cpp | 11 +++++++++++ src/qt/test/optiontests.cpp | 4 ++++ test/functional/feature_settings.py | 7 ++++--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/common/settings.cpp b/src/common/settings.cpp index cbf794a7c62..913ad6d76a9 100644 --- a/src/common/settings.cpp +++ b/src/common/settings.cpp @@ -4,6 +4,10 @@ #include +#if defined(HAVE_CONFIG_H) +#include +#endif + #include #include #include @@ -116,6 +120,13 @@ bool WriteSettings(const fs::path& path, std::vector& 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); } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index e5a5179d879..7100603616c 100644 --- a/src/qt/test/optiontests.cpp +++ b/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(file), std::istreambuf_iterator()).c_str(), "{\n" + " \"_warning_\": \""+ default_warning+"\",\n" " \"dbcache\": \"600\",\n" " \"listen\": false,\n" " \"onion\": \"onion:234\",\n" diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 4175699152e..0214e781de0 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -23,10 +23,11 @@ class SettingsTest(BitcoinTestFramework): 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,7 +49,7 @@ class SettingsTest(BitcoinTestFramework): # 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: