Merge #15934: Merge settings one place instead of five places

083c954b02a4e7d0708349eeaf3bac2b5947fb0e Add settings_tests (Russell Yanofsky)
7f40528cd50fc43ac0bd3e785de24d661adddb7a Deduplicate settings merge code (Russell Yanofsky)
9dcb952fe5f85529ab28e091af7534e72c21c90f Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cfe8af088bd8ea884be2f79f0f3cac555d5 Remove includeconf nested scope (Russell Yanofsky)
5a84aa880f6da0bac0e2144733fdef3b8558c761 Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e75487461ec9bff433144f0db831b682403 Clarify emptyIncludeConf logic (Russell Yanofsky)

Pull request description:

  This is a refactoring-only change that makes it easier to add a new settings source.

  This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

  This change:

  - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935).
  - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways.
  - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108).

  The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:

  * 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
  * 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_

ACKs for top commit:
  ariard:
    ACK 083c954
  jnewbery:
    ACK 083c954b02a4e7d0708349eeaf3bac2b5947fb0e
  jamesob:
    ACK 083c954b02a4e7d0708349eeaf3bac2b5947fb0e

Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659

4 files changed, 396 insertions, 242 deletions

diff --git a/src/util/settings.cpp b/src/util/settings.cpp
new file mode 100644
index 000000000..af75fef31
--- /dev/null
+++ b/src/util/settings.cpp
@@ -0,0 +1,169 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/settings.h>
+
+#include <univalue.h>
+
+namespace util {
+namespace {
+
+enum class Source {
+   FORCED,
+   COMMAND_LINE,
+   CONFIG_FILE_NETWORK_SECTION,
+   CONFIG_FILE_DEFAULT_SECTION
+};
+
+//! Merge settings from multiple sources in precedence order:
+//! Forced config > command line > config file network-specific section > config file default section
+//!
+//! This function is provided with a callback function fn that contains
+//! specific logic for how to merge the sources.
+template <typename Fn>
+static void MergeSettings(const Settings& settings, const std::string& section, const std::string& name, Fn&& fn)
+{
+    // Merge in the forced settings
+    if (auto* value = FindKey(settings.forced_settings, name)) {
+        fn(SettingsSpan(*value), Source::FORCED);
+    }
+    // Merge in the command-line options
+    if (auto* values = FindKey(settings.command_line_options, name)) {
+        fn(SettingsSpan(*values), Source::COMMAND_LINE);
+    }
+    // Merge in the network-specific section of the config file
+    if (!section.empty()) {
+        if (auto* map = FindKey(settings.ro_config, section)) {
+            if (auto* values = FindKey(*map, name)) {
+                fn(SettingsSpan(*values), Source::CONFIG_FILE_NETWORK_SECTION);
+            }
+        }
+    }
+    // Merge in the default section of the config file
+    if (auto* map = FindKey(settings.ro_config, "")) {
+        if (auto* values = FindKey(*map, name)) {
+            fn(SettingsSpan(*values), Source::CONFIG_FILE_DEFAULT_SECTION);
+        }
+    }
+}
+} // namespace
+
+SettingsValue GetSetting(const Settings& settings,
+    const std::string& section,
+    const std::string& name,
+    bool ignore_default_section_config,
+    bool get_chain_name)
+{
+    SettingsValue result;
+    MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
+        // Weird behavior preserved for backwards compatibility: Apply negated
+        // setting even if non-negated setting would be ignored. A negated
+        // value in the default section is applied to network specific options,
+        // even though normal non-negated values there would be ignored.
+        const bool never_ignore_negated_setting = span.last_negated();
+
+        // Weird behavior preserved for backwards compatibility: Take first
+        // assigned value instead of last. In general, later settings take
+        // precedence over early settings, but for backwards compatibility in
+        // the config file the precedence is reversed for all settings except
+        // chain name settings.
+        const bool reverse_precedence = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !get_chain_name;
+
+        // Weird behavior preserved for backwards compatibility: Negated
+        // -regtest and -testnet arguments which you would expect to override
+        // values set in the configuration file are currently accepted but
+        // silently ignored. It would be better to apply these just like other
+        // negated values, or at least warn they are ignored.
+        const bool skip_negated_command_line = get_chain_name;
+
+        // Ignore settings in default config section if requested.
+        if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && !never_ignore_negated_setting) return;
+
+        // Skip negated command line settings.
+        if (skip_negated_command_line && span.last_negated()) return;
+
+        // Stick with highest priority value, keeping result if already set.
+        if (!result.isNull()) return;
+
+        if (!span.empty()) {
+            result = reverse_precedence ? span.begin()[0] : span.end()[-1];
+        } else if (span.last_negated()) {
+            result = false;
+        }
+    });
+    return result;
+}
+
+std::vector<SettingsValue> GetSettingsList(const Settings& settings,
+    const std::string& section,
+    const std::string& name,
+    bool ignore_default_section_config)
+{
+    std::vector<SettingsValue> result;
+    bool result_complete = false;
+    bool prev_negated_empty = false;
+    MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
+        // Weird behavior preserved for backwards compatibility: Apply config
+        // file settings even if negated on command line. Negating a setting on
+        // command line will ignore earlier settings on the command line and
+        // ignore settings in the config file, unless the negated command line
+        // value is followed by non-negated value, in which case config file
+        // settings will be brought back from the dead (but earlier command
+        // line settings will still be ignored).
+        const bool add_zombie_config_values = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !prev_negated_empty;
+
+        // Ignore settings in default config section if requested.
+        if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION) return;
+
+        // Add new settings to the result if isn't already complete, or if the
+        // values are zombies.
+        if (!result_complete || add_zombie_config_values) {
+            for (const auto& value : span) {
+                if (value.isArray()) {
+                    result.insert(result.end(), value.getValues().begin(), value.getValues().end());
+                } else {
+                    result.push_back(value);
+                }
+            }
+        }
+
+        // If a setting was negated, or if a setting was forced, set
+        // result_complete to true to ignore any later lower priority settings.
+        result_complete |= span.negated() > 0 || source == Source::FORCED;
+
+        // Update the negated and empty state used for the zombie values check.
+        prev_negated_empty |= span.last_negated() && result.empty();
+    });
+    return result;
+}
+
+bool OnlyHasDefaultSectionSetting(const Settings& settings, const std::string& section, const std::string& name)
+{
+    bool has_default_section_setting = false;
+    bool has_other_setting = false;
+    MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
+        if (span.empty()) return;
+        else if (source == Source::CONFIG_FILE_DEFAULT_SECTION) has_default_section_setting = true;
+        else has_other_setting = true;
+    });
+    // If a value is set in the default section and not explicitly overwritten by the
+    // user on the command line or in a different section, then we want to enable
+    // warnings about the value being ignored.
+    return has_default_section_setting && !has_other_setting;
+}
+
+SettingsSpan::SettingsSpan(const std::vector<SettingsValue>& vec) noexcept : SettingsSpan(vec.data(), vec.size()) {}
+const SettingsValue* SettingsSpan::begin() const { return data + negated(); }
+const SettingsValue* SettingsSpan::end() const { return data + size; }
+bool SettingsSpan::empty() const { return size == 0 || last_negated(); }
+bool SettingsSpan::last_negated() const { return size > 0 && data[size - 1].isFalse(); }
+size_t SettingsSpan::negated() const
+{
+    for (size_t i = size; i > 0; --i) {
+        if (data[i - 1].isFalse()) return i; // Return number of negated values (position of last false value)
+    }
+    return 0;
+}
+
+} // namespace util
diff --git a/src/util/settings.h b/src/util/settings.h
new file mode 100644
index 000000000..17832e4d2
--- /dev/null
+++ b/src/util/settings.h
@@ -0,0 +1,87 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_SETTINGS_H
+#define BITCOIN_UTIL_SETTINGS_H
+
+#include <map>
+#include <string>
+#include <vector>
+
+class UniValue;
+
+namespace util {
+
+//! Settings value type (string/integer/boolean/null variant).
+//!
+//! @note UniValue is used here for convenience and because it can be easily
+//!       serialized in a readable format. But any other variant type that can
+//!       be assigned strings, int64_t, and bool values and has get_str(),
+//!       get_int64(), get_bool(), isNum(), isBool(), isFalse(), isTrue() and
+//!       isNull() methods can be substituted if there's a need to move away
+//!       from UniValue. (An implementation with boost::variant was posted at
+//!       https://github.com/bitcoin/bitcoin/pull/15934/files#r337691812)
+using SettingsValue = UniValue;
+
+//! Stored bitcoin settings. This struct combines settings from the command line
+//! and a read-only configuration file.
+struct Settings {
+    //! Map of setting name to forced setting value.
+    std::map<std::string, SettingsValue> forced_settings;
+    //! Map of setting name to list of command line values.
+    std::map<std::string, std::vector<SettingsValue>> command_line_options;
+    //! Map of config section name and setting name to list of config file values.
+    std::map<std::string, std::map<std::string, std::vector<SettingsValue>>> ro_config;
+};
+
+//! Get settings value from combined sources: forced settings, command line
+//! arguments and the read-only config file.
+//!
+//! @param ignore_default_section_config - ignore values in the default section
+//!                                        of the config file (part before any
+//!                                        [section] keywords)
+//! @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 get_chain_name);
+
+//! Get combined setting value similar to GetSetting(), except if setting was
+//! specified multiple times, return a list of all the values specified.
+std::vector<SettingsValue> GetSettingsList(const Settings& settings, const std::string& section, const std::string& name, bool ignore_default_section_config);
+
+//! Return true if a setting is set in the default config file section, and not
+//! overridden by a higher priority command-line or network section value.
+//!
+//! This is used to provide user warnings about values that might be getting
+//! ignored unintentionally.
+bool OnlyHasDefaultSectionSetting(const Settings& settings, const std::string& section, const std::string& name);
+
+//! Accessor for list of settings that skips negated values when iterated over.
+//! The last boolean `false` value in the list and all earlier values are
+//! considered negated.
+struct SettingsSpan {
+    explicit SettingsSpan() = default;
+    explicit SettingsSpan(const SettingsValue& value) noexcept : SettingsSpan(&value, 1) {}
+    explicit SettingsSpan(const SettingsValue* data, size_t size) noexcept : data(data), size(size) {}
+    explicit SettingsSpan(const std::vector<SettingsValue>& vec) noexcept;
+    const SettingsValue* begin() const; //<! Pointer to first non-negated value.
+    const SettingsValue* end() const;   //<! Pointer to end of values.
+    bool empty() const;                 //<! True if there are any non-negated values.
+    bool last_negated() const;          //<! True if the last value is negated.
+    size_t negated() const;             //<! Number of negated values.
+
+    const SettingsValue* data = nullptr;
+    size_t size = 0;
+};
+
+//! Map lookup helper.
+template <typename Map, typename Key>
+auto FindKey(Map&& map, Key&& key) -> decltype(&map.at(key))
+{
+    auto it = map.find(key);
+    return it == map.end() ? nullptr : &it->second;
+}
+
+} // namespace util
+
+#endif // BITCOIN_UTIL_SETTINGS_H
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 7da408eda..2a2ae6fdf 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -63,6 +63,7 @@
 #endif
 
 #include <thread>
+#include <univalue.h>
 
 // Application startup time (used for uptime calculation)
 const int64_t nStartupTime = GetTime();
@@ -161,11 +162,14 @@ static bool InterpretBool(const std::string& strValue)
     return (atoi(strValue) != 0);
 }
 
+static std::string SettingName(const std::string& arg)
+{
+    return arg.size() > 0 && arg[0] == '-' ? arg.substr(1) : arg;
+}
+
 /** Internal helper functions for ArgsManager */
 class ArgsManagerHelper {
 public:
-    typedef std::map<std::string, std::vector<std::string>> MapArgs;
-
     /** Determine whether to use config settings in the default section,
      *  See also comments around ArgsManager::ArgsManager() below. */
     static inline bool UseDefaultSection(const ArgsManager& am, const std::string& arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args)
@@ -173,91 +177,10 @@ public:
         return (am.m_network == CBaseChainParams::MAIN || am.m_network_only_args.count(arg) == 0);
     }
 
-    /** Convert regular argument into the network-specific setting */
-    static inline std::string NetworkArg(const ArgsManager& am, const std::string& arg)
-    {
-        assert(arg.length() > 1 && arg[0] == '-');
-        return "-" + am.m_network + "." + arg.substr(1);
-    }
-
-    /** Find arguments in a map and add them to a vector */
-    static inline void AddArgs(std::vector<std::string>& res, const MapArgs& map_args, const std::string& arg)
-    {
-        auto it = map_args.find(arg);
-        if (it != map_args.end()) {
-            res.insert(res.end(), it->second.begin(), it->second.end());
-        }
-    }
-
-    /** Return true/false if an argument is set in a map, and also
-     *  return the first (or last) of the possibly multiple values it has
-     */
-    static inline std::pair<bool,std::string> GetArgHelper(const MapArgs& map_args, const std::string& arg, bool getLast = false)
-    {
-        auto it = map_args.find(arg);
-
-        if (it == map_args.end() || it->second.empty()) {
-            return std::make_pair(false, std::string());
-        }
-
-        if (getLast) {
-            return std::make_pair(true, it->second.back());
-        } else {
-            return std::make_pair(true, it->second.front());
-        }
-    }
-
-    /* Get the string value of an argument, returning a pair of a boolean
-     * indicating the argument was found, and the value for the argument
-     * if it was found (or the empty string if not found).
-     */
-    static inline std::pair<bool,std::string> GetArg(const ArgsManager &am, const std::string& arg)
+    static util::SettingsValue Get(const ArgsManager& am, const std::string& arg)
     {
         LOCK(am.cs_args);
-        std::pair<bool,std::string> found_result(false, std::string());
-
-        // We pass "true" to GetArgHelper in order to return the last
-        // argument value seen from the command line (so "bitcoind -foo=bar
-        // -foo=baz" gives GetArg(am,"foo")=={true,"baz"}
-        found_result = GetArgHelper(am.m_override_args, arg, true);
-        if (found_result.first) {
-            return found_result;
-        }
-
-        // But in contrast we return the first argument seen in a config file,
-        // so "foo=bar \n foo=baz" in the config file gives
-        // GetArg(am,"foo")={true,"bar"}
-        if (!am.m_network.empty()) {
-            found_result = GetArgHelper(am.m_config_args, NetworkArg(am, arg));
-            if (found_result.first) {
-                return found_result;
-            }
-        }
-
-        if (UseDefaultSection(am, arg)) {
-            found_result = GetArgHelper(am.m_config_args, arg);
-            if (found_result.first) {
-                return found_result;
-            }
-        }
-
-        return found_result;
-    }
-
-    /* Special test for -testnet and -regtest args, because we
-     * don't want to be confused by craziness like "[regtest] testnet=1"
-     */
-    static inline bool GetNetBoolArg(const ArgsManager &am, const std::string& net_arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args)
-    {
-        std::pair<bool,std::string> found_result(false,std::string());
-        found_result = GetArgHelper(am.m_override_args, net_arg, true);
-        if (!found_result.first) {
-            found_result = GetArgHelper(am.m_config_args, net_arg, true);
-            if (!found_result.first) {
-                return false; // not set
-            }
-        }
-        return InterpretBool(found_result.second); // is set, so evaluate
+        return GetSetting(am.m_settings, am.m_network, SettingName(arg), !UseDefaultSection(am, arg), /* get_chain_name= */ false);
     }
 };
 
@@ -268,13 +191,12 @@ public:
  * checks whether there was a double-negative (-nofoo=0 -> -foo=1).
  *
  * If there was not a double negative, it removes the "no" from the key
- * and clears the args vector to indicate a negated option.
+ * and returns false.
  *
- * If there was a double negative, it removes "no" from the key, sets the
- * value to "1" and pushes the key and the updated value to the args vector.
+ * If there was a double negative, it removes "no" from the key, and
+ * returns true.
  *
- * If there was no "no", it leaves key and value untouched and pushes them
- * to the args vector.
+ * If there was no "no", it returns the string value untouched.
  *
  * Where an option was negated can be later checked using the
  * IsArgNegated() method. One use case for this is to have a way to disable
@@ -282,34 +204,39 @@ public:
  * that debug log output is not sent to any file at all).
  */
 
-NODISCARD static bool InterpretOption(std::string key, std::string val, unsigned int flags,
-                                      std::map<std::string, std::vector<std::string>>& args,
-                                      std::string& error)
+static util::SettingsValue InterpretOption(std::string& section, std::string& key, const std::string& value)
 {
-    assert(key[0] == '-');
-
+    // Split section name from key name for keys like "testnet.foo" or "regtest.bar"
     size_t option_index = key.find('.');
-    if (option_index == std::string::npos) {
-        option_index = 1;
-    } else {
-        ++option_index;
+    if (option_index != std::string::npos) {
+        section = key.substr(0, option_index);
+        key.erase(0, option_index + 1);
     }
-    if (key.substr(option_index, 2) == "no") {
-        key.erase(option_index, 2);
-        if (flags & ArgsManager::ALLOW_BOOL) {
-            if (InterpretBool(val)) {
-                args[key].clear();
-                return true;
-            }
-            // Double negatives like -nofoo=0 are supported (but discouraged)
-            LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
-            val = "1";
-        } else {
-            error = strprintf("Negating of %s is meaningless and therefore forbidden", key);
-            return false;
+    if (key.substr(0, 2) == "no") {
+        key.erase(0, 2);
+        // Double negatives like -nofoo=0 are supported (but discouraged)
+        if (!InterpretBool(value)) {
+            LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key, value);
+            return true;
         }
+        return false;
+    }
+    return value;
+}
+
+/**
+ * Check settings value validity according to flags.
+ *
+ * TODO: Add more meaningful error checks here in the future
+ * See "here's how the flags are meant to behave" in
+ * https://github.com/bitcoin/bitcoin/pull/16097#issuecomment-514627823
+ */
+static bool CheckValid(const std::string& key, const util::SettingsValue& val, unsigned int flags, std::string& error)
+{
+    if (val.isBool() && !(flags & ArgsManager::ALLOW_BOOL)) {
+        error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);
+        return false;
     }
-    args[key].push_back(val);
     return true;
 }
 
@@ -331,22 +258,9 @@ const std::set<std::string> ArgsManager::GetUnsuitableSectionOnlyArgs() const
     if (m_network == CBaseChainParams::MAIN) return std::set<std::string> {};
 
     for (const auto& arg : m_network_only_args) {
-        std::pair<bool, std::string> found_result;
-
-        // if this option is overridden it's fine
-        found_result = ArgsManagerHelper::GetArgHelper(m_override_args, arg);
-        if (found_result.first) continue;
-
-        // if there's a network-specific value for this option, it's fine
-        found_result = ArgsManagerHelper::GetArgHelper(m_config_args, ArgsManagerHelper::NetworkArg(*this, arg));
-        if (found_result.first) continue;
-
-        // if there isn't a default value for this option, it's fine
-        found_result = ArgsManagerHelper::GetArgHelper(m_config_args, arg);
-        if (!found_result.first) continue;
-
-        // otherwise, issue a warning
-        unsuitables.insert(arg);
+        if (OnlyHasDefaultSectionSetting(m_settings, m_network, SettingName(arg))) {
+            unsuitables.insert(arg);
+        }
     }
     return unsuitables;
 }
@@ -375,7 +289,7 @@ void ArgsManager::SelectConfigNetwork(const std::string& network)
 bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::string& error)
 {
     LOCK(cs_args);
-    m_override_args.clear();
+    m_settings.command_line_options.clear();
 
     for (int i = 1; i < argc; i++) {
         std::string key(argv[i]);
@@ -408,49 +322,44 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
         if (key.length() > 1 && key[1] == '-')
             key.erase(0, 1);
 
+        // Transform -foo to foo
+        key.erase(0, 1);
+        std::string section;
+        util::SettingsValue value = InterpretOption(section, key, val);
         const unsigned int flags = FlagsOfKnownArg(key);
         if (flags) {
-            if (!InterpretOption(key, val, flags, m_override_args, error)) {
+            if (!CheckValid(key, value, flags, error)) {
                 return false;
             }
+            // Weird behavior preserved for backwards compatibility: command
+            // line options with section prefixes are allowed but ignored. It
+            // would be better if these options triggered the Invalid parameter
+            // error below.
+            if (section.empty()) {
+                m_settings.command_line_options[key].push_back(value);
+            }
         } else {
-            error = strprintf("Invalid parameter %s", key);
+            error = strprintf("Invalid parameter -%s", key);
             return false;
         }
     }
 
     // we do not allow -includeconf from command line, so we clear it here
-    auto it = m_override_args.find("-includeconf");
-    if (it != m_override_args.end()) {
-        if (it->second.size() > 0) {
-            for (const auto& ic : it->second) {
-                error += "-includeconf cannot be used from commandline; -includeconf=" + ic + "\n";
-            }
-            return false;
+    bool success = true;
+    if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) {
+        for (const auto& include : util::SettingsSpan(*includes)) {
+            error += "-includeconf cannot be used from commandline; -includeconf=" + include.get_str() + "\n";
+            success = false;
         }
     }
-    return true;
+    return success;
 }
 
 unsigned int ArgsManager::FlagsOfKnownArg(const std::string& key) const
 {
-    assert(key[0] == '-');
-
-    size_t option_index = key.find('.');
-    if (option_index == std::string::npos) {
-        option_index = 1;
-    } else {
-        ++option_index;
-    }
-    if (key.substr(option_index, 2) == "no") {
-        option_index += 2;
-    }
-
-    const std::string base_arg_name = '-' + key.substr(option_index);
-
     LOCK(cs_args);
     for (const auto& arg_map : m_available_args) {
-        const auto search = arg_map.second.find(base_arg_name);
+        const auto search = arg_map.second.find('-' + key);
         if (search != arg_map.second.end()) {
             return search->second.m_flags;
         }
@@ -460,69 +369,42 @@ unsigned int ArgsManager::FlagsOfKnownArg(const std::string& key) const
 
 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 {
-    std::vector<std::string> result = {};
-    if (IsArgNegated(strArg)) return result; // special case
-
     LOCK(cs_args);
-
-    ArgsManagerHelper::AddArgs(result, m_override_args, strArg);
-    if (!m_network.empty()) {
-        ArgsManagerHelper::AddArgs(result, m_config_args, ArgsManagerHelper::NetworkArg(*this, strArg));
+    bool ignore_default_section_config = !ArgsManagerHelper::UseDefaultSection(*this, strArg);
+    std::vector<std::string> result;
+    for (const util::SettingsValue& value :
+        util::GetSettingsList(m_settings, m_network, SettingName(strArg), ignore_default_section_config)) {
+        result.push_back(value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str());
     }
-
-    if (ArgsManagerHelper::UseDefaultSection(*this, strArg)) {
-        ArgsManagerHelper::AddArgs(result, m_config_args, strArg);
-    }
-
     return result;
 }
 
 bool ArgsManager::IsArgSet(const std::string& strArg) const
 {
-    if (IsArgNegated(strArg)) return true; // special case
-    return ArgsManagerHelper::GetArg(*this, strArg).first;
+    return !ArgsManagerHelper::Get(*this, strArg).isNull();
 }
 
 bool ArgsManager::IsArgNegated(const std::string& strArg) const
 {
-    LOCK(cs_args);
-
-    const auto& ov = m_override_args.find(strArg);
-    if (ov != m_override_args.end()) return ov->second.empty();
-
-    if (!m_network.empty()) {
-        const auto& cfs = m_config_args.find(ArgsManagerHelper::NetworkArg(*this, strArg));
-        if (cfs != m_config_args.end()) return cfs->second.empty();
-    }
-
-    const auto& cf = m_config_args.find(strArg);
-    if (cf != m_config_args.end()) return cf->second.empty();
-
-    return false;
+    return ArgsManagerHelper::Get(*this, strArg).isFalse();
 }
 
 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 {
-    if (IsArgNegated(strArg)) return "0";
-    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
-    if (found_res.first) return found_res.second;
-    return strDefault;
+    const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg);
+    return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str();
 }
 
 int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const
 {
-    if (IsArgNegated(strArg)) return 0;
-    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
-    if (found_res.first) return atoi64(found_res.second);
-    return nDefault;
+    const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg);
+    return value.isNull() ? nDefault : value.isFalse() ? 0 : value.isTrue() ? 1 : value.isNum() ? value.get_int64() : atoi64(value.get_str());
 }
 
 bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
 {
-    if (IsArgNegated(strArg)) return false;
-    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
-    if (found_res.first) return InterpretBool(found_res.second);
-    return fDefault;
+    const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg);
+    return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
 }
 
 bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue)
@@ -544,7 +426,7 @@ bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue)
 void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue)
 {
     LOCK(cs_args);
-    m_override_args[strArg] = {strValue};
+    m_settings.forced_settings[SettingName(strArg)] = strValue;
 }
 
 void ArgsManager::AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat)
@@ -860,12 +742,15 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
         return false;
     }
     for (const std::pair<std::string, std::string>& option : options) {
-        const std::string strKey = std::string("-") + option.first;
-        const unsigned int flags = FlagsOfKnownArg(strKey);
+        std::string section;
+        std::string key = option.first;
+        util::SettingsValue value = InterpretOption(section, key, option.second);
+        const unsigned int flags = FlagsOfKnownArg(key);
         if (flags) {
-            if (!InterpretOption(strKey, option.second, flags, m_config_args, error)) {
+            if (!CheckValid(key, value, flags, error)) {
                 return false;
             }
+            m_settings.ro_config[section][key].push_back(value);
         } else {
             if (ignore_invalid_keys) {
                 LogPrintf("Ignoring unknown configuration value %s\n", option.first);
@@ -882,7 +767,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 {
     {
         LOCK(cs_args);
-        m_config_args.clear();
+        m_settings.ro_config.clear();
         m_config_sections.clear();
     }
 
@@ -894,58 +779,64 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
         if (!ReadConfigStream(stream, confPath, error, ignore_invalid_keys)) {
             return false;
         }
-        // if there is an -includeconf in the override args, but it is empty, that means the user
-        // passed '-noincludeconf' on the command line, in which case we should not include anything
-        bool emptyIncludeConf;
+        // `-includeconf` cannot be included in the command line arguments except
+        // as `-noincludeconf` (which indicates that no conf file should be used).
+        bool use_conf_file{true};
         {
             LOCK(cs_args);
-            emptyIncludeConf = m_override_args.count("-includeconf") == 0;
+            if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) {
+                // ParseParameters() fails if a non-negated -includeconf is passed on the command-line
+                assert(util::SettingsSpan(*includes).last_negated());
+                use_conf_file = false;
+            }
         }
-        if (emptyIncludeConf) {
+        if (use_conf_file) {
             std::string chain_id = GetChainName();
-            std::vector<std::string> includeconf(GetArgs("-includeconf"));
-            {
-                // We haven't set m_network yet (that happens in SelectParams()), so manually check
-                // for network.includeconf args.
-                std::vector<std::string> includeconf_net(GetArgs(std::string("-") + chain_id + ".includeconf"));
-                includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end());
-            }
+            std::vector<std::string> conf_file_names;
 
-            // Remove -includeconf from configuration, so we can warn about recursion
-            // later
-            {
+            auto add_includes = [&](const std::string& network, size_t skip = 0) {
+                size_t num_values = 0;
                 LOCK(cs_args);
-                m_config_args.erase("-includeconf");
-                m_config_args.erase(std::string("-") + chain_id + ".includeconf");
-            }
-
-            for (const std::string& to_include : includeconf) {
-                fsbridge::ifstream include_config(GetConfigFile(to_include));
-                if (include_config.good()) {
-                    if (!ReadConfigStream(include_config, to_include, error, ignore_invalid_keys)) {
+                if (auto* section = util::FindKey(m_settings.ro_config, network)) {
+                    if (auto* values = util::FindKey(*section, "includeconf")) {
+                        for (size_t i = std::max(skip, util::SettingsSpan(*values).negated()); i < values->size(); ++i) {
+                            conf_file_names.push_back((*values)[i].get_str());
+                        }
+                        num_values = values->size();
+                    }
+                }
+                return num_values;
+            };
+
+            // We haven't set m_network yet (that happens in SelectParams()), so manually check
+            // for network.includeconf args.
+            const size_t chain_includes = add_includes(chain_id);
+            const size_t default_includes = add_includes({});
+
+            for (const std::string& conf_file_name : conf_file_names) {
+                fsbridge::ifstream conf_file_stream(GetConfigFile(conf_file_name));
+                if (conf_file_stream.good()) {
+                    if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
                         return false;
                     }
-                    LogPrintf("Included configuration file %s\n", to_include);
+                    LogPrintf("Included configuration file %s\n", conf_file_name);
                 } else {
-                    error = "Failed to include configuration file " + to_include;
+                    error = "Failed to include configuration file " + conf_file_name;
                     return false;
                 }
             }
 
             // Warn about recursive -includeconf
-            includeconf = GetArgs("-includeconf");
-            {
-                std::vector<std::string> includeconf_net(GetArgs(std::string("-") + chain_id + ".includeconf"));
-                includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end());
-                std::string chain_id_final = GetChainName();
-                if (chain_id_final != chain_id) {
-                    // Also warn about recursive includeconf for the chain that was specified in one of the includeconfs
-                    includeconf_net = GetArgs(std::string("-") + chain_id_final + ".includeconf");
-                    includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end());
-                }
+            conf_file_names.clear();
+            add_includes(chain_id, /* skip= */ chain_includes);
+            add_includes({}, /* skip= */ default_includes);
+            std::string chain_id_final = GetChainName();
+            if (chain_id_final != chain_id) {
+                // Also warn about recursive includeconf for the chain that was specified in one of the includeconfs
+                add_includes(chain_id_final);
             }
-            for (const std::string& to_include : includeconf) {
-                tfm::format(std::cerr, "warning: -includeconf cannot be used from included files; ignoring -includeconf=%s\n", to_include);
+            for (const std::string& conf_file_name : conf_file_names) {
+                tfm::format(std::cerr, "warning: -includeconf cannot be used from included files; ignoring -includeconf=%s\n", conf_file_name);
             }
         }
     }
@@ -961,9 +852,16 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 
 std::string ArgsManager::GetChainName() const
 {
-    LOCK(cs_args);
-    const bool fRegTest = ArgsManagerHelper::GetNetBoolArg(*this, "-regtest");
-    const bool fTestNet = ArgsManagerHelper::GetNetBoolArg(*this, "-testnet");
+    auto get_net = [&](const std::string& arg) {
+        LOCK(cs_args);
+        util::SettingsValue value = GetSetting(m_settings, /* section= */ "", SettingName(arg),
+                                               /* ignore_default_section_config= */ false,
+                                               /* get_chain_name= */ true);
+        return value.isNull() ? false : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
+    };
+
+    const bool fRegTest = get_net("-regtest");
+    const bool fTestNet = get_net("-testnet");
     const bool is_chain_arg_set = IsArgSet("-chain");
 
     if ((int)is_chain_arg_set + (int)fRegTest + (int)fTestNet > 1) {
diff --git a/src/util/system.h b/src/util/system.h
index 7452f186e..e0b6371dc 100644
--- a/src/util/system.h
+++ b/src/util/system.h
@@ -22,6 +22,7 @@
 #include <sync.h>
 #include <tinyformat.h>
 #include <util/memory.h>
+#include <util/settings.h>
 #include <util/threadnames.h>
 #include <util/time.h>
 
@@ -157,8 +158,7 @@ protected:
     };
 
     mutable CCriticalSection cs_args;
-    std::map<std::string, std::vector<std::string>> m_override_args GUARDED_BY(cs_args);
-    std::map<std::string, std::vector<std::string>> m_config_args GUARDED_BY(cs_args);
+    util::Settings m_settings GUARDED_BY(cs_args);
     std::string m_network GUARDED_BY(cs_args);
     std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);