Repository: bitcoin/bitcoin (language: cpp)

Issue:
Bug: ArgsManager::ReadSettingsFile can return false even when it does load settings
Credit to @luke-jr for helping uncovering this bug https://github.com/bitcoin-core/gui/pull/379#discussion_r680408927

This bug has no user-visible effects currently, but combined with PR [bitcoin-core/gui#379](https://github.com/bitcoin-core/gui/pull/379) and a corrupt `settings.json` file that is corrupted specifically by specifying the same JSON keys multiple times, it can cause some dialog text added in that PR to be misleading. This is a basically an impossible-to-hit rube goldberg bug, but is only fixable at the util level not the GUI level, so I'm tracking this outside the GUI repository.

Simplest fix would be to just change `ReadSettingsFile` to call `rw_settings.clear()` on error. It would also be nice to add a unit test for the regression.

Code path that triggers this is:

https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/util/system.cpp#L548
https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/util/settings.cpp#L89
https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/util/settings.cpp#L92


Discussion / hints:
@ryanofsky 
> Credit to @luke-jr for helping uncovering this bug [bitcoin-core/gui#379 (comment)](https://github.com/bitcoin-core/gui/pull/379#discussion_r683406901)

Looks like a link to @luke-jr's comment should be fixed :)
> Looks like a link to @luke-jr's comment should be fixed :)

Thanks, fixed the link!

Fix patch:
diff --git a/src/util/settings.cpp b/src/util/settings.cpp
index 2b25b7f0e1f60..a2e30098dc642 100644
--- a/src/util/settings.cpp
+++ b/src/util/settings.cpp
@@ -99,6 +99,8 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
         auto inserted = values.emplace(in_keys[i], in_values[i]);
         if (!inserted.second) {
             errors.emplace_back(strprintf("Found duplicate key %s in settings file %s", in_keys[i], fs::PathToString(path)));
+            values.clear();
+            break;
         }
     }
     return errors.empty();


Test patch:
diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp
index 0feb68b9b1f8b..ad12c46561064 100644
--- a/src/test/settings_tests.cpp
+++ b/src/test/settings_tests.cpp
@@ -80,7 +80,7 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
     BOOST_CHECK(values.empty());
     BOOST_CHECK(errors.empty());
 
-    // Check duplicate keys not allowed
+    // Check duplicate keys not allowed and that values returns empty if a duplicate is found.
     WriteText(path, R"({
         "dupe": "string",
         "dupe": "dupe"
@@ -88,6 +88,7 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
     BOOST_CHECK(!util::ReadSettings(path, values, errors));
     std::vector<std::string> dup_keys = {strprintf("Found duplicate key dupe in settings file %s", fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), dup_keys.begin(), dup_keys.end());
+    BOOST_CHECK(values.empty());
 
     // Check non-kv json files not allowed
     WriteText(path, R"("non-kv")");