From 0e55bc6e7fe439404dc56093a0949395dae51e6b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 1/3] settings: Add update/getPersistent/isIgnored methods Add interfaces::Node methods to give GUI finer grained control over settings.json file. Update method is used to write settings to the file, getPersistent and isIgnored methods are used to find out about settings file and command line option interactions. --- src/interfaces/node.h | 25 ++++++++++++++++++++----- src/node/interfaces.cpp | 32 ++++++++++++++++++++++++++++++++ src/test/settings_tests.cpp | 8 ++++---- src/util/settings.cpp | 4 ++++ src/util/settings.h | 6 ++++++ src/util/system.cpp | 26 +++++++++++++++++++++++++- src/util/system.h | 10 ++++++++++ 7 files changed, 101 insertions(+), 10 deletions(-) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index c4dc303dd5..1585174a6f 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -5,12 +5,13 @@ #ifndef BITCOIN_INTERFACES_NODE_H #define BITCOIN_INTERFACES_NODE_H -#include -#include // For NodeId -#include // For banmap_t -#include // For Network -#include // For ConnectionDirection +#include // For CAmount +#include // For NodeId +#include // For banmap_t +#include // For Network +#include // For ConnectionDirection #include // For SecureString +#include // For util::SettingsValue #include #include @@ -97,6 +98,20 @@ public: //! Return whether shutdown was requested. virtual bool shutdownRequested() = 0; + //! Return whether a particular setting in /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 /settings.json or bitcoin.conf. + virtual util::SettingsValue getPersistentSetting(const std::string& name) = 0; + + //! Update a setting in /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; + //! Map port. virtual void mapPort(bool use_upnp, bool use_natpmp) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f2debc790c..09404a5f85 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -112,6 +112,38 @@ 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 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 diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index 15ffd068c7..0feb68b9b1 100644 --- a/src/test/settings_tests.cpp +++ b/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 += " "; diff --git a/src/util/settings.cpp b/src/util/settings.cpp index 26439b010b..924a9cfab2 100644 --- a/src/util/settings.cpp +++ b/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; diff --git a/src/util/settings.h b/src/util/settings.h index 261a0a032f..e97158dc09 100644 --- a/src/util/settings.h +++ b/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 diff --git a/src/util/system.cpp b/src/util/system.cpp index cbe9d2ceb0..87d3be9d23 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -592,6 +592,13 @@ bool ArgsManager::WriteSettingsFile(std::vector* 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(); @@ -600,18 +607,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() : LocaleIndependentAtoi(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()); } @@ -1002,6 +1024,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()); }; @@ -1034,7 +1057,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 ArgsManager::GetSettingsList(const std::string& arg) const diff --git a/src/util/system.h b/src/util/system.h index a7f4d16911..bfc9698221 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -161,6 +161,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: @@ -448,6 +452,12 @@ protected: */ bool WriteSettingsFile(std::vector* errors = nullptr) 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. */ From 77fabffef4ea840ee15c97061048fe8443d74658 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Apr 2019 15:29:00 -0400 Subject: [PATCH 2/3] init: Remove Shutdown() node.args reset This commit removes the `node.args = nullptr` assignment in the Shutdown() function. Clearing node.args there never made sense because it made the Shutdown() function not idempotent, making it fragile and causing issues like https://github.com/bitcoin/bitcoin/issues/23186. The assignment also causes segfaults in GUI unit tests when a new node().initParameterInteraction() call is added in OptionsModel to apply to Qt settings (happens because AppTests calls Shutdown() which sets node.args to null, and OptionTests runs after AppTests and then needs node.args not to be null.) --- src/init.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index f06b5bd0d0..c9e05026cf 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -320,7 +320,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__); } From f9fdcec7e932843a91ddf7f377e00bd2a6efb82a Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 16 May 2022 14:37:00 -0400 Subject: [PATCH 3/3] settings: Add resetSettings() method Allows the GUI to clear settings.json file and save settings.json.bak file when GUI "Reset Options" button is pressed or -resetguisettings command line option is used. (GUI code already backs up and resets the "guisettings.ini" file this way, so this just makes the same behavior possible for "settings.json") --- src/interfaces/node.h | 4 ++++ src/node/interfaces.cpp | 8 ++++++++ src/util/system.cpp | 9 ++++++--- src/util/system.h | 7 ++++--- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 1585174a6f..2c31e12ada 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -112,6 +112,10 @@ public: //! source, but not being persisted. virtual void forceSetting(const std::string& name, const util::SettingsValue& value) = 0; + //! Clear all settings in /settings.json and store a backup of + //! previous settings in /settings.json.bak. + virtual void resetSettings() = 0; + //! Map port. virtual void mapPort(bool use_upnp, bool use_natpmp) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 09404a5f85..01db843976 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -144,6 +144,14 @@ public: } }); } + 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 diff --git a/src/util/system.cpp b/src/util/system.cpp index 87d3be9d23..fa2e564791 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -526,12 +526,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); } @@ -572,10 +575,10 @@ bool ArgsManager::ReadSettingsFile(std::vector* errors) return true; } -bool ArgsManager::WriteSettingsFile(std::vector* errors) const +bool ArgsManager::WriteSettingsFile(std::vector* 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."); } diff --git a/src/util/system.h b/src/util/system.h index bfc9698221..6af4f1dc18 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -440,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. @@ -448,9 +448,10 @@ protected: bool ReadSettingsFile(std::vector* 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* errors = nullptr) const; + bool WriteSettingsFile(std::vector* errors = nullptr, bool backup = false) const; /** * Get current setting from config file or read/write settings file,