Merge bitcoin/bitcoin#15936: interfaces: Expose settings.json methods to GUI

f9fdcec7e9 settings: Add resetSettings() method (Ryan Ofsky)
77fabffef4 init: Remove Shutdown() node.args reset (Ryan Ofsky)
0e55bc6e7f settings: Add update/getPersistent/isIgnored methods (Ryan Ofsky)

Pull request description:

  Add `interfaces::Node` `updateSetting`, `forceSetting`, `resetSettings`, `isSettingIgnored`, and `getPersistentSetting` methods so GUI is able to manipulate `settings.json` file and use and modify node settings.

  (Originally this PR also contained GUI changes to unify bitcoin-qt and bitcoind persistent settings and call these methods, but the GUI commits have been dropped from this PR and moved to bitcoin-core/gui/pull/602)

ACKs for top commit:
  vasild:
    ACK f9fdcec7e9
  hebasto:
    re-ACK f9fdcec7e9, only a function renamed since my recent [review](https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-979324357).

Tree-SHA512: 4cac853ee29be96d2ff38404165b9dfb7c622b2a9c99a15979596f3484ffde0da3d9c9c372677dff5119ca7cffa6383d81037fd9889a29cc9285882a8dc0c268

src/init.cpp

@@ -319,7 +319,6 @@ void Shutdown(NodeContext& node)
         LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
     }
 
-    node.args = nullptr;
     LogPrintf("%s: done\n", __func__);
 }
 

src/interfaces/node.h

@@ -5,12 +5,13 @@
 #ifndef BITCOIN_INTERFACES_NODE_H
 #define BITCOIN_INTERFACES_NODE_H
 
-#include <consensus/amount.h>
+#include <consensus/amount.h>          // For CAmount
-#include <net.h>        // For NodeId
+#include <net.h>                       // For NodeId
-#include <net_types.h>  // For banmap_t
+#include <net_types.h>                 // For banmap_t
-#include <netaddress.h> // For Network
+#include <netaddress.h>                // For Network
-#include <netbase.h>    // For ConnectionDirection
+#include <netbase.h>                   // For ConnectionDirection
 #include <support/allocators/secure.h> // For SecureString
+#include <util/settings.h>             // For util::SettingsValue
 #include <util/translation.h>
 
 #include <functional>
@@ -97,6 +98,24 @@ public:
     //! Return whether shutdown was requested.
     virtual bool shutdownRequested() = 0;
 
+    //! Return whether a particular setting in <datadir>/settings.json is or
+    //! would be ignored because it is also specified in the command line.
+    virtual bool isSettingIgnored(const std::string& name) = 0;
+
+    //! Return setting value from <datadir>/settings.json or bitcoin.conf.
+    virtual util::SettingsValue getPersistentSetting(const std::string& name) = 0;
+
+    //! Update a setting in <datadir>/settings.json.
+    virtual void updateRwSetting(const std::string& name, const util::SettingsValue& value) = 0;
+
+    //! Force a setting value to be applied, overriding any other configuration
+    //! source, but not being persisted.
+    virtual void forceSetting(const std::string& name, const util::SettingsValue& value) = 0;
+
+    //! Clear all settings in <datadir>/settings.json and store a backup of
+    //! previous settings in <datadir>/settings.json.bak.
+    virtual void resetSettings() = 0;
+
     //! Map port.
     virtual void mapPort(bool use_upnp, bool use_natpmp) = 0;
 

src/node/interfaces.cpp

@@ -112,6 +112,46 @@ public:
         }
     }
     bool shutdownRequested() override { return ShutdownRequested(); }
+    bool isSettingIgnored(const std::string& name) override
+    {
+        bool ignored = false;
+        gArgs.LockSettings([&](util::Settings& settings) {
+            if (auto* options = util::FindKey(settings.command_line_options, name)) {
+                ignored = !options->empty();
+            }
+        });
+        return ignored;
+    }
+    util::SettingsValue getPersistentSetting(const std::string& name) override { return gArgs.GetPersistentSetting(name); }
+    void updateRwSetting(const std::string& name, const util::SettingsValue& value) override
+    {
+        gArgs.LockSettings([&](util::Settings& settings) {
+            if (value.isNull()) {
+                settings.rw_settings.erase(name);
+            } else {
+                settings.rw_settings[name] = value;
+            }
+        });
+        gArgs.WriteSettingsFile();
+    }
+    void forceSetting(const std::string& name, const util::SettingsValue& value) override
+    {
+        gArgs.LockSettings([&](util::Settings& settings) {
+            if (value.isNull()) {
+                settings.forced_settings.erase(name);
+            } else {
+                settings.forced_settings[name] = value;
+            }
+        });
+    }
+    void resetSettings() override
+    {
+        gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true);
+        gArgs.LockSettings([&](util::Settings& settings) {
+            settings.rw_settings.clear();
+        });
+        gArgs.WriteSettingsFile();
+    }
     void mapPort(bool use_upnp, bool use_natpmp) override { StartMapPort(use_upnp, use_natpmp); }
     bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }
     size_t getNodeCount(ConnectionDirection flags) override

src/test/settings_tests.cpp

@@ -105,7 +105,7 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
 //! Check settings struct contents against expected json strings.
 static void CheckValues(const util::Settings& settings, const std::string& single_val, const std::string& list_val)
 {
-    util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false);
+    util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false, false);
     util::SettingsValue list_value(util::SettingsValue::VARR);
     for (const auto& item : GetSettingsList(settings, "section", "name", false)) {
         list_value.push_back(item);
@@ -141,9 +141,9 @@ BOOST_AUTO_TEST_CASE(NullOverride)
 {
     util::Settings settings;
     settings.command_line_options["name"].push_back("value");
-    BOOST_CHECK_EQUAL(R"("value")", GetSetting(settings, "section", "name", false, false).write().c_str());
+    BOOST_CHECK_EQUAL(R"("value")", GetSetting(settings, "section", "name", false, false, false).write().c_str());
     settings.forced_settings["name"] = {};
-    BOOST_CHECK_EQUAL(R"(null)", GetSetting(settings, "section", "name", false, false).write().c_str());
+    BOOST_CHECK_EQUAL(R"(null)", GetSetting(settings, "section", "name", false, false, false).write().c_str());
 }
 
 // Test different ways settings can be merged, and verify results. This test can
@@ -224,7 +224,7 @@ BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup)
         }
 
         desc += " || ";
-        desc += GetSetting(settings, network, name, ignore_default_section_config, /* get_chain_name= */ false).write();
+        desc += GetSetting(settings, network, name, ignore_default_section_config, /*ignore_nonpersistent=*/false, /*get_chain_name=*/false).write();
         desc += " |";
         for (const auto& s : GetSettingsList(settings, network, name, ignore_default_section_config)) {
             desc += " ";

src/util/settings.cpp

@@ -127,6 +127,7 @@ SettingsValue GetSetting(const Settings& settings,
     const std::string& section,
     const std::string& name,
     bool ignore_default_section_config,
+    bool ignore_nonpersistent,
     bool get_chain_name)
 {
     SettingsValue result;
@@ -162,6 +163,9 @@ SettingsValue GetSetting(const Settings& settings,
             return;
         }
 
+        // Ignore nonpersistent settings if requested.
+        if (ignore_nonpersistent && (source == Source::COMMAND_LINE || source == Source::FORCED)) return;
+
         // Skip negated command line settings.
         if (skip_negated_command_line && span.last_negated()) return;
 

src/util/settings.h

@@ -55,12 +55,18 @@ bool WriteSettings(const fs::path& path,
 //! @param ignore_default_section_config - ignore values in the default section
 //!                                        of the config file (part before any
 //!                                        [section] keywords)
+//! @param ignore_nonpersistent - ignore non-persistent settings values (forced
+//!                               settings values and values specified on the
+//!                               command line). Only return settings in the
+//!                               read-only config and read-write settings
+//!                               files.
 //! @param get_chain_name - enable special backwards compatible behavior
 //!                         for GetChainName
 SettingsValue GetSetting(const Settings& settings,
     const std::string& section,
     const std::string& name,
     bool ignore_default_section_config,
+    bool ignore_nonpersistent,
     bool get_chain_name);
 
 //! Get combined setting value similar to GetSetting(), except if setting was

src/util/system.cpp

@@ -530,12 +530,15 @@ bool ArgsManager::InitSettings(std::string& error)
     return true;
 }
 
-bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp) const
+bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp, bool backup) const
 {
     fs::path settings = GetPathArg("-settings", fs::path{BITCOIN_SETTINGS_FILENAME});
     if (settings.empty()) {
         return false;
     }
+    if (backup) {
+        settings += ".bak";
+    }
     if (filepath) {
         *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), temp ? settings + ".tmp" : settings);
     }
@@ -576,10 +579,10 @@ bool ArgsManager::ReadSettingsFile(std::vector<std::string>* errors)
     return true;
 }
 
-bool ArgsManager::WriteSettingsFile(std::vector<std::string>* errors) const
+bool ArgsManager::WriteSettingsFile(std::vector<std::string>* errors, bool backup) const
 {
     fs::path path, path_tmp;
-    if (!GetSettingsPath(&path, /* temp= */ false) || !GetSettingsPath(&path_tmp, /* temp= */ true)) {
+    if (!GetSettingsPath(&path, /*temp=*/false, backup) || !GetSettingsPath(&path_tmp, /*temp=*/true, backup)) {
         throw std::logic_error("Attempt to write settings file when dynamic settings are disabled.");
     }
 
@@ -596,6 +599,13 @@ bool ArgsManager::WriteSettingsFile(std::vector<std::string>* errors) const
     return true;
 }
 
+util::SettingsValue ArgsManager::GetPersistentSetting(const std::string& name) const
+{
+    LOCK(cs_args);
+    return util::GetSetting(m_settings, m_network, name, !UseDefaultSection("-" + name),
+        /*ignore_nonpersistent=*/true, /*get_chain_name=*/false);
+}
+
 bool ArgsManager::IsArgNegated(const std::string& strArg) const
 {
     return GetSetting(strArg).isFalse();
@@ -604,18 +614,33 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 {
     const util::SettingsValue value = GetSetting(strArg);
+    return SettingToString(value, strDefault);
+}
+
+std::string SettingToString(const util::SettingsValue& value, const std::string& strDefault)
+{
     return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str();
 }
 
 int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const
 {
     const util::SettingsValue value = GetSetting(strArg);
+    return SettingToInt(value, nDefault);
+}
+
+int64_t SettingToInt(const util::SettingsValue& value, int64_t nDefault)
+{
     return value.isNull() ? nDefault : value.isFalse() ? 0 : value.isTrue() ? 1 : value.isNum() ? value.getInt<int64_t>() : LocaleIndependentAtoi<int64_t>(value.get_str());
 }
 
 bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
 {
     const util::SettingsValue value = GetSetting(strArg);
+    return SettingToBool(value, fDefault);
+}
+
+bool SettingToBool(const util::SettingsValue& value, bool fDefault)
+{
     return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
 }
 
@@ -1006,6 +1031,7 @@ std::string ArgsManager::GetChainName() const
         LOCK(cs_args);
         util::SettingsValue value = util::GetSetting(m_settings, /* section= */ "", SettingName(arg),
             /* ignore_default_section_config= */ false,
+            /*ignore_nonpersistent=*/false,
             /* get_chain_name= */ true);
         return value.isNull() ? false : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
     };
@@ -1038,7 +1064,8 @@ util::SettingsValue ArgsManager::GetSetting(const std::string& arg) const
 {
     LOCK(cs_args);
     return util::GetSetting(
-        m_settings, m_network, SettingName(arg), !UseDefaultSection(arg), /* get_chain_name= */ false);
+        m_settings, m_network, SettingName(arg), !UseDefaultSection(arg),
+        /*ignore_nonpersistent=*/false, /*get_chain_name=*/false);
 }
 
 std::vector<util::SettingsValue> ArgsManager::GetSettingsList(const std::string& arg) const

src/util/system.h

@@ -160,6 +160,10 @@ struct SectionInfo
     int m_line;
 };
 
+std::string SettingToString(const util::SettingsValue&, const std::string&);
+int64_t SettingToInt(const util::SettingsValue&, int64_t);
+bool SettingToBool(const util::SettingsValue&, bool);
+
 class ArgsManager
 {
 public:
@@ -436,7 +440,7 @@ protected:
      * Get settings file path, or return false if read-write settings were
      * disabled with -nosettings.
      */
-    bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false) const;
+    bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const;
 
     /**
      * Read settings file. Push errors to vector, or log them if null.
@@ -444,9 +448,16 @@ protected:
     bool ReadSettingsFile(std::vector<std::string>* errors = nullptr);
 
     /**
-     * Write settings file. Push errors to vector, or log them if null.
+     * Write settings file or backup settings file. Push errors to vector, or
+     * log them if null.
      */
-    bool WriteSettingsFile(std::vector<std::string>* errors = nullptr) const;
+    bool WriteSettingsFile(std::vector<std::string>* errors = nullptr, bool backup = false) const;
+
+    /**
+     * Get current setting from config file or read/write settings file,
+     * ignoring nonpersistent command line or forced settings values.
+     */
+    util::SettingsValue GetPersistentSetting(const std::string& name) const;
 
     /**
      * Access settings with lock held.