From 3673ca36ef84192b42d7e6acbdc8b5d2ffc7a0cf Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:01:00 +1000
Subject: ArgsManager: keep command line and config file arguments separate

---
 src/test/util_tests.cpp |  83 ++++++++++++++++++++++++--------------
 src/util.cpp            | 105 +++++++++++++++++++++++++++++++++++++-----------
 src/util.h              |   7 ++--
 3 files changed, 138 insertions(+), 57 deletions(-)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index d41c43a795..f33cda6ff5 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -187,13 +187,17 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Time)
 
 struct TestArgsManager : public ArgsManager
 {
-    std::map<std::string, std::string>& GetMapArgs() { return mapArgs; }
-    const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs() { return mapMultiArgs; }
+    std::map<std::string, std::vector<std::string> >& GetOverrideArgs() { return m_override_args; }
+    std::map<std::string, std::vector<std::string> >& GetConfigArgs() { return m_config_args; }
     const std::unordered_set<std::string>& GetNegatedArgs() { return m_negated_args; }
     void ReadConfigString(const std::string str_config)
     {
-        std::istringstream stream(str_config);
-        ReadConfigStream(stream);
+        std::istringstream streamConfig(str_config);
+        {
+            LOCK(cs_args);
+            m_config_args.clear();
+        }
+        ReadConfigStream(streamConfig);
     }
 };
 
@@ -203,22 +207,26 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
     const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"};
 
     testArgs.ParseParameters(0, (char**)argv_test);
-    BOOST_CHECK(testArgs.GetMapArgs().empty() && testArgs.GetMapMultiArgs().empty());
+    BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty());
 
     testArgs.ParseParameters(1, (char**)argv_test);
-    BOOST_CHECK(testArgs.GetMapArgs().empty() && testArgs.GetMapMultiArgs().empty());
+    BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty());
 
     testArgs.ParseParameters(7, (char**)argv_test);
     // expectation: -ignored is ignored (program name argument),
     // -a, -b and -ccc end up in map, -d ignored because it is after
     // a non-option argument (non-GNU option parsing)
-    BOOST_CHECK(testArgs.GetMapArgs().size() == 3 && testArgs.GetMapMultiArgs().size() == 3);
+    BOOST_CHECK(testArgs.GetOverrideArgs().size() == 3 && testArgs.GetConfigArgs().empty());
     BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc")
                 && !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d"));
-    BOOST_CHECK(testArgs.GetMapMultiArgs().count("-a") && testArgs.GetMapMultiArgs().count("-b") && testArgs.GetMapMultiArgs().count("-ccc")
-                && !testArgs.GetMapMultiArgs().count("f") && !testArgs.GetMapMultiArgs().count("-d"));
-
-    BOOST_CHECK(testArgs.GetMapArgs()["-a"] == "" && testArgs.GetMapArgs()["-ccc"] == "multiple");
+    BOOST_CHECK(testArgs.GetOverrideArgs().count("-a") && testArgs.GetOverrideArgs().count("-b") && testArgs.GetOverrideArgs().count("-ccc")
+                && !testArgs.GetOverrideArgs().count("f") && !testArgs.GetOverrideArgs().count("-d"));
+
+    BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].size() == 1);
+    BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].front() == "");
+    BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].size() == 2);
+    BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].front() == "argument");
+    BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].back() == "multiple");
     BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
 }
 
@@ -234,8 +242,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
         BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt);
 
     // Nothing else should be in the map
-    BOOST_CHECK(testArgs.GetMapArgs().size() == 6 &&
-                testArgs.GetMapMultiArgs().size() == 6);
+    BOOST_CHECK(testArgs.GetOverrideArgs().size() == 6 &&
+                testArgs.GetConfigArgs().empty());
 
     // The -no prefix should get stripped on the way in.
     BOOST_CHECK(!testArgs.IsArgSet("-nob"));
@@ -326,17 +334,17 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     test_args.ReadConfigString(str_config);
     // expectation: a, b, ccc, d, fff, ggg, h, i end up in map
 
-    BOOST_CHECK(test_args.GetMapArgs().size() == 8);
-    BOOST_CHECK(test_args.GetMapMultiArgs().size() == 8);
-
-    BOOST_CHECK(test_args.GetMapArgs().count("-a")
-                && test_args.GetMapArgs().count("-b")
-                && test_args.GetMapArgs().count("-ccc")
-                && test_args.GetMapArgs().count("-d")
-                && test_args.GetMapArgs().count("-fff")
-                && test_args.GetMapArgs().count("-ggg")
-                && test_args.GetMapArgs().count("-h")
-                && test_args.GetMapArgs().count("-i")
+    BOOST_CHECK(test_args.GetOverrideArgs().empty());
+    BOOST_CHECK(test_args.GetConfigArgs().size() == 8);
+
+    BOOST_CHECK(test_args.GetConfigArgs().count("-a")
+                && test_args.GetConfigArgs().count("-b")
+                && test_args.GetConfigArgs().count("-ccc")
+                && test_args.GetConfigArgs().count("-d")
+                && test_args.GetConfigArgs().count("-fff")
+                && test_args.GetConfigArgs().count("-ggg")
+                && test_args.GetConfigArgs().count("-h")
+                && test_args.GetConfigArgs().count("-i")
                );
 
     BOOST_CHECK(test_args.IsArgSet("-a")
@@ -411,16 +419,24 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 BOOST_AUTO_TEST_CASE(util_GetArg)
 {
     TestArgsManager testArgs;
-    testArgs.GetMapArgs().clear();
-    testArgs.GetMapArgs()["strtest1"] = "string...";
+    testArgs.GetOverrideArgs().clear();
+    testArgs.GetOverrideArgs()["strtest1"] = {"string..."};
     // strtest2 undefined on purpose
-    testArgs.GetMapArgs()["inttest1"] = "12345";
-    testArgs.GetMapArgs()["inttest2"] = "81985529216486895";
+    testArgs.GetOverrideArgs()["inttest1"] = {"12345"};
+    testArgs.GetOverrideArgs()["inttest2"] = {"81985529216486895"};
     // inttest3 undefined on purpose
-    testArgs.GetMapArgs()["booltest1"] = "";
+    testArgs.GetOverrideArgs()["booltest1"] = {""};
     // booltest2 undefined on purpose
-    testArgs.GetMapArgs()["booltest3"] = "0";
-    testArgs.GetMapArgs()["booltest4"] = "1";
+    testArgs.GetOverrideArgs()["booltest3"] = {"0"};
+    testArgs.GetOverrideArgs()["booltest4"] = {"1"};
+
+    // priorities
+    testArgs.GetOverrideArgs()["pritest1"] = {"a", "b"};
+    testArgs.GetConfigArgs()["pritest2"] = {"a", "b"};
+    testArgs.GetOverrideArgs()["pritest3"] = {"a"};
+    testArgs.GetConfigArgs()["pritest3"] = {"b"};
+    testArgs.GetOverrideArgs()["pritest4"] = {"a","b"};
+    testArgs.GetConfigArgs()["pritest4"] = {"c","d"};
 
     BOOST_CHECK_EQUAL(testArgs.GetArg("strtest1", "default"), "string...");
     BOOST_CHECK_EQUAL(testArgs.GetArg("strtest2", "default"), "default");
@@ -431,6 +447,11 @@ BOOST_AUTO_TEST_CASE(util_GetArg)
     BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest2", false), false);
     BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest3", false), false);
     BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest4", false), true);
+
+    BOOST_CHECK_EQUAL(testArgs.GetArg("pritest1", "default"), "b");
+    BOOST_CHECK_EQUAL(testArgs.GetArg("pritest2", "default"), "a");
+    BOOST_CHECK_EQUAL(testArgs.GetArg("pritest3", "default"), "a");
+    BOOST_CHECK_EQUAL(testArgs.GetArg("pritest4", "default"), "b");
 }
 
 BOOST_AUTO_TEST_CASE(util_GetChainName)
diff --git a/src/util.cpp b/src/util.cpp
index f55c9c8c34..f8366fe694 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -455,6 +455,67 @@ static bool InterpretBool(const std::string& strValue)
     return (atoi(strValue) != 0);
 }
 
+/** Internal helper functions for ArgsManager */
+class ArgsManagerHelper {
+public:
+    typedef std::map<std::string, std::vector<std::string>> MapArgs;
+
+    /** 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)
+    {
+        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"}
+        found_result = GetArgHelper(am.m_config_args, arg);
+        if (found_result.first) {
+            return found_result;
+        }
+
+        return found_result;
+    }
+};
+
 /**
  * Interpret -nofoo as if the user supplied -foo=0.
  *
@@ -485,8 +546,7 @@ void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val)
 void ArgsManager::ParseParameters(int argc, const char* const argv[])
 {
     LOCK(cs_args);
-    mapArgs.clear();
-    mapMultiArgs.clear();
+    m_override_args.clear();
     m_negated_args.clear();
 
     for (int i = 1; i < argc; i++) {
@@ -513,23 +573,23 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
         // Transform -nofoo to -foo=0
         InterpretNegatedOption(key, val);
 
-        mapArgs[key] = val;
-        mapMultiArgs[key].push_back(val);
+        m_override_args[key].push_back(val);
     }
 }
 
 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 {
+    std::vector<std::string> result = {};
+
     LOCK(cs_args);
-    auto it = mapMultiArgs.find(strArg);
-    if (it != mapMultiArgs.end()) return it->second;
-    return {};
+    ArgsManagerHelper::AddArgs(result, m_override_args, strArg);
+    ArgsManagerHelper::AddArgs(result, m_config_args, strArg);
+    return result;
 }
 
 bool ArgsManager::IsArgSet(const std::string& strArg) const
 {
-    LOCK(cs_args);
-    return mapArgs.count(strArg);
+    return ArgsManagerHelper::GetArg(*this, strArg).first;
 }
 
 bool ArgsManager::IsArgNegated(const std::string& strArg) const
@@ -540,25 +600,22 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
 
 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 {
-    LOCK(cs_args);
-    auto it = mapArgs.find(strArg);
-    if (it != mapArgs.end()) return it->second;
+    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
+    if (found_res.first) return found_res.second;
     return strDefault;
 }
 
 int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const
 {
-    LOCK(cs_args);
-    auto it = mapArgs.find(strArg);
-    if (it != mapArgs.end()) return atoi64(it->second);
+    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
+    if (found_res.first) return atoi64(found_res.second);
     return nDefault;
 }
 
 bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
 {
-    LOCK(cs_args);
-    auto it = mapArgs.find(strArg);
-    if (it != mapArgs.end()) return InterpretBool(it->second);
+    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
+    if (found_res.first) return InterpretBool(found_res.second);
     return fDefault;
 }
 
@@ -581,8 +638,7 @@ bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue)
 void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue)
 {
     LOCK(cs_args);
-    mapArgs[strArg] = strValue;
-    mapMultiArgs[strArg] = {strValue};
+    m_override_args[strArg] = {strValue};
 }
 
 bool HelpRequested(const ArgsManager& args)
@@ -749,14 +805,17 @@ void ArgsManager::ReadConfigStream(std::istream& stream)
         std::string strKey = std::string("-") + it->string_key;
         std::string strValue = it->value[0];
         InterpretNegatedOption(strKey, strValue);
-        if (mapArgs.count(strKey) == 0)
-            mapArgs[strKey] = strValue;
-        mapMultiArgs[strKey].push_back(strValue);
+        m_config_args[strKey].push_back(strValue);
     }
 }
 
 void ArgsManager::ReadConfigFile(const std::string& confPath)
 {
+    {
+        LOCK(cs_args);
+        m_config_args.clear();
+    }
+
     fs::ifstream stream(GetConfigFile(confPath));
 
     // ok to not have a config file
diff --git a/src/util.h b/src/util.h
index 3952461e48..01ff5992fb 100644
--- a/src/util.h
+++ b/src/util.h
@@ -223,11 +223,12 @@ inline bool IsSwitchChar(char c)
 class ArgsManager
 {
 protected:
+    friend class ArgsManagerHelper;
+
     mutable CCriticalSection cs_args;
-    std::map<std::string, std::string> mapArgs;
-    std::map<std::string, std::vector<std::string>> mapMultiArgs;
+    std::map<std::string, std::vector<std::string>> m_override_args;
+    std::map<std::string, std::vector<std::string>> m_config_args;
     std::unordered_set<std::string> m_negated_args;
-
     void ReadConfigStream(std::istream& stream);
 
 public:
-- 
cgit v1.2.3


From 4d34fcc7138f0ffc831f0f8601c50cc7f494c197 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:02:00 +1000
Subject: ArgsManager: drop m_negated_args

When a -nofoo option is seen, instead of adding it to a separate
set of negated args, set the arg as being an empty vector of strings.

This changes the behaviour in some ways:
 - -nofoo=0 still sets foo=1 but no longer treats it as a negated arg
 - -nofoo=1 -foo=2 has GetArgs() return [2] rather than [2,0]
 - "foo=2 \n -nofoo=1" in a config file no longer returns [2,0], just [0]
 - GetArgs returns an empty vector for negated args
---
 src/test/util_tests.cpp | 49 ++++++++++++++++----------------------
 src/util.cpp            | 63 ++++++++++++++++++++++++++++++++++---------------
 src/util.h              |  6 -----
 3 files changed, 64 insertions(+), 54 deletions(-)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index f33cda6ff5..f04f6cf171 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -189,7 +189,6 @@ struct TestArgsManager : public ArgsManager
 {
     std::map<std::string, std::vector<std::string> >& GetOverrideArgs() { return m_override_args; }
     std::map<std::string, std::vector<std::string> >& GetConfigArgs() { return m_config_args; }
-    const std::unordered_set<std::string>& GetNegatedArgs() { return m_negated_args; }
     void ReadConfigString(const std::string str_config)
     {
         std::istringstream streamConfig(str_config);
@@ -250,7 +249,6 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
 
     // The -b option is flagged as negated, and nothing else is
     BOOST_CHECK(testArgs.IsArgNegated("-b"));
-    BOOST_CHECK(testArgs.GetNegatedArgs().size() == 1);
     BOOST_CHECK(!testArgs.IsArgNegated("-a"));
 
     // Check expected values.
@@ -275,8 +273,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
     BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
     BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "");
 
-    // A double negative is a positive.
-    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
+    // A double negative is a positive, and not marked as negated.
+    BOOST_CHECK(!testArgs.IsArgNegated("-bar"));
     BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1");
 
     // Config test
@@ -285,12 +283,12 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
     testArgs.ReadConfigString(conf_test);
 
     // This was passed twice, second one overrides the negative setting,
-    // but not the value.
+    // and the value.
     BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
-    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0");
+    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "1");
 
-    // A double negative is a positive.
-    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
+    // A double negative is a positive, and does not count as negated.
+    BOOST_CHECK(!testArgs.IsArgNegated("-bar"));
     BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1");
 
     // Combined test
@@ -300,18 +298,15 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
     testArgs.ReadConfigString(combo_test_conf);
 
     // Command line overrides, but doesn't erase old setting
-    BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
+    BOOST_CHECK(testArgs.IsArgNegated("-foo"));
     BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0");
-    BOOST_CHECK(testArgs.GetArgs("-foo").size() == 2
-                && testArgs.GetArgs("-foo").front() == "0"
-                && testArgs.GetArgs("-foo").back() == "1");
+    BOOST_CHECK(testArgs.GetArgs("-foo").size() == 0);
 
     // Command line overrides, but doesn't erase old setting
-    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
+    BOOST_CHECK(!testArgs.IsArgNegated("-bar"));
     BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "");
-    BOOST_CHECK(testArgs.GetArgs("-bar").size() == 2
-                && testArgs.GetArgs("-bar").front() == ""
-                && testArgs.GetArgs("-bar").back() == "0");
+    BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1
+                && testArgs.GetArgs("-bar").front() == "");
 }
 
 BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
@@ -364,8 +359,8 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.GetArg("-d", "xxx") == "e"
                 && test_args.GetArg("-fff", "xxx") == "0"
                 && test_args.GetArg("-ggg", "xxx") == "1"
-                && test_args.GetArg("-h", "xxx") == "1" // 1st value takes precedence
-                && test_args.GetArg("-i", "xxx") == "0" // 1st value takes precedence
+                && test_args.GetArg("-h", "xxx") == "0"
+                && test_args.GetArg("-i", "xxx") == "1"
                 && test_args.GetArg("-zzz", "xxx") == "xxx"
                );
 
@@ -376,8 +371,8 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                      && !test_args.GetBoolArg("-d", def)
                      && !test_args.GetBoolArg("-fff", def)
                      && test_args.GetBoolArg("-ggg", def)
-                     && test_args.GetBoolArg("-h", def)
-                     && !test_args.GetBoolArg("-i", def)
+                     && !test_args.GetBoolArg("-h", def)
+                     && test_args.GetBoolArg("-i", def)
                      && test_args.GetBoolArg("-zzz", def) == def
                    );
     }
@@ -389,19 +384,15 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2
                 && test_args.GetArgs("-ccc").front() == "argument"
                 && test_args.GetArgs("-ccc").back() == "multiple");
-    BOOST_CHECK(test_args.GetArgs("-fff").size() == 1
-                && test_args.GetArgs("-fff").front() == "0");
+    BOOST_CHECK(test_args.GetArgs("-fff").size() == 0);
     BOOST_CHECK(test_args.GetArgs("-nofff").size() == 0);
     BOOST_CHECK(test_args.GetArgs("-ggg").size() == 1
                 && test_args.GetArgs("-ggg").front() == "1");
     BOOST_CHECK(test_args.GetArgs("-noggg").size() == 0);
-    BOOST_CHECK(test_args.GetArgs("-h").size() == 2
-                && test_args.GetArgs("-h").front() == "1"
-                && test_args.GetArgs("-h").back() == "0");
+    BOOST_CHECK(test_args.GetArgs("-h").size() == 0);
     BOOST_CHECK(test_args.GetArgs("-noh").size() == 0);
-    BOOST_CHECK(test_args.GetArgs("-i").size() == 2
-                && test_args.GetArgs("-i").front() == "0"
-                && test_args.GetArgs("-i").back() == "1");
+    BOOST_CHECK(test_args.GetArgs("-i").size() == 1
+                && test_args.GetArgs("-i").front() == "1");
     BOOST_CHECK(test_args.GetArgs("-noi").size() == 0);
     BOOST_CHECK(test_args.GetArgs("-zzz").size() == 0);
 
@@ -410,7 +401,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     BOOST_CHECK(!test_args.IsArgNegated("-ccc"));
     BOOST_CHECK(!test_args.IsArgNegated("-d"));
     BOOST_CHECK(test_args.IsArgNegated("-fff"));
-    BOOST_CHECK(test_args.IsArgNegated("-ggg")); // IsArgNegated==true when noggg=0
+    BOOST_CHECK(!test_args.IsArgNegated("-ggg"));
     BOOST_CHECK(test_args.IsArgNegated("-h")); // last setting takes precedence
     BOOST_CHECK(!test_args.IsArgNegated("-i")); // last setting takes precedence
     BOOST_CHECK(!test_args.IsArgNegated("-zzz"));
diff --git a/src/util.cpp b/src/util.cpp
index f8366fe694..3e69574022 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -519,35 +519,44 @@ public:
 /**
  * Interpret -nofoo as if the user supplied -foo=0.
  *
- * This method also tracks when the -no form was supplied, and treats "-foo" as
- * a negated option when this happens. This can be later checked using the
+ * This method also tracks when the -no form was supplied, and if so,
+ * 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 returns true, indicating the caller should clear the args vector
+ * to indicate a negated option.
+ *
+ * If there was a double negative, it removes "no" from the key, sets the
+ * value to "1" and returns false.
+ *
+ * If there was no "no", it leaves key and value untouched and returns
+ * false.
+ *
+ * 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
  * options that are not normally boolean (e.g. using -nodebuglogfile to request
  * that debug log output is not sent to any file at all).
  */
-void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val)
+static bool InterpretNegatedOption(std::string& key, std::string& val)
 {
     if (key.substr(0, 3) == "-no") {
         bool bool_val = InterpretBool(val);
+        key.erase(1, 2);
         if (!bool_val ) {
             // Double negatives like -nofoo=0 are supported (but discouraged)
             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
+            val = "1";
+        } else {
+            return true;
         }
-        key.erase(1, 2);
-        m_negated_args.insert(key);
-        val = bool_val ? "0" : "1";
-    } else {
-        // In an invocation like "bitcoind -nofoo -foo" we want to unmark -foo
-        // as negated when we see the second option.
-        m_negated_args.erase(key);
     }
+    return false;
 }
 
 void ArgsManager::ParseParameters(int argc, const char* const argv[])
 {
     LOCK(cs_args);
     m_override_args.clear();
-    m_negated_args.clear();
 
     for (int i = 1; i < argc; i++) {
         std::string key(argv[i]);
@@ -570,16 +579,19 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
         if (key.length() > 1 && key[1] == '-')
             key.erase(0, 1);
 
-        // Transform -nofoo to -foo=0
-        InterpretNegatedOption(key, val);
-
-        m_override_args[key].push_back(val);
+        // Check for -nofoo
+        if (InterpretNegatedOption(key, val)) {
+            m_override_args[key].clear();
+        } else {
+            m_override_args[key].push_back(val);
+        }
     }
 }
 
 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);
@@ -589,17 +601,26 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 
 bool ArgsManager::IsArgSet(const std::string& strArg) const
 {
+    if (IsArgNegated(strArg)) return true; // special case
     return ArgsManagerHelper::GetArg(*this, strArg).first;
 }
 
 bool ArgsManager::IsArgNegated(const std::string& strArg) const
 {
     LOCK(cs_args);
-    return m_negated_args.find(strArg) != m_negated_args.end();
+
+    const auto& ov = m_override_args.find(strArg);
+    if (ov != m_override_args.end()) return ov->second.empty();
+
+    const auto& cf = m_config_args.find(strArg);
+    if (cf != m_config_args.end()) return cf->second.empty();
+
+    return false;
 }
 
 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;
@@ -607,6 +628,7 @@ std::string ArgsManager::GetArg(const std::string& strArg, const std::string& st
 
 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;
@@ -614,6 +636,7 @@ int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const
 
 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;
@@ -801,11 +824,13 @@ void ArgsManager::ReadConfigStream(std::istream& stream)
 
     for (boost::program_options::detail::config_file_iterator it(stream, setOptions), end; it != end; ++it)
     {
-        // Don't overwrite existing settings so command line settings override bitcoin.conf
         std::string strKey = std::string("-") + it->string_key;
         std::string strValue = it->value[0];
-        InterpretNegatedOption(strKey, strValue);
-        m_config_args[strKey].push_back(strValue);
+        if (InterpretNegatedOption(strKey, strValue)) {
+            m_config_args[strKey].clear();
+        } else {
+            m_config_args[strKey].push_back(strValue);
+        }
     }
 }
 
diff --git a/src/util.h b/src/util.h
index 01ff5992fb..6b80afc58f 100644
--- a/src/util.h
+++ b/src/util.h
@@ -228,7 +228,6 @@ protected:
     mutable CCriticalSection cs_args;
     std::map<std::string, std::vector<std::string>> m_override_args;
     std::map<std::string, std::vector<std::string>> m_config_args;
-    std::unordered_set<std::string> m_negated_args;
     void ReadConfigStream(std::istream& stream);
 
 public:
@@ -314,11 +313,6 @@ public:
      * @return CBaseChainParams::MAIN by default; raises runtime error if an invalid combination is given.
      */
     std::string GetChainName() const;
-
-private:
-
-    // Munge -nofoo into -foo=0 and track the value as negated.
-    void InterpretNegatedOption(std::string &key, std::string &val);
 };
 
 extern ArgsManager gArgs;
-- 
cgit v1.2.3


From 95eb66d5842e5ccdeb7481b9ee92ac4aface6b0f Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:03:00 +1000
Subject: ArgsManager: support config file sections

---
 src/chainparamsbase.cpp |  1 +
 src/util.cpp            | 39 +++++++++++++++++++++++++++++++++++++--
 src/util.h              |  6 ++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
index e840a2ed30..3ef9c2cfe5 100644
--- a/src/chainparamsbase.cpp
+++ b/src/chainparamsbase.cpp
@@ -47,4 +47,5 @@ std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain
 void SelectBaseParams(const std::string& chain)
 {
     globalChainBaseParams = CreateBaseChainParams(chain);
+    gArgs.SelectConfigNetwork(chain);
 }
diff --git a/src/util.cpp b/src/util.cpp
index 3e69574022..85c3d221af 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -460,6 +460,13 @@ class ArgsManagerHelper {
 public:
     typedef std::map<std::string, std::vector<std::string>> MapArgs;
 
+    /** 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)
     {
@@ -507,6 +514,13 @@ public:
         // 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;
+            }
+        }
+
         found_result = GetArgHelper(am.m_config_args, arg);
         if (found_result.first) {
             return found_result;
@@ -539,9 +553,17 @@ public:
  */
 static bool InterpretNegatedOption(std::string& key, std::string& val)
 {
-    if (key.substr(0, 3) == "-no") {
+    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") {
         bool bool_val = InterpretBool(val);
-        key.erase(1, 2);
+        key.erase(option_index, 2);
         if (!bool_val ) {
             // Double negatives like -nofoo=0 are supported (but discouraged)
             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
@@ -553,6 +575,11 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
     return false;
 }
 
+void ArgsManager::SelectConfigNetwork(const std::string& network)
+{
+    m_network = network;
+}
+
 void ArgsManager::ParseParameters(int argc, const char* const argv[])
 {
     LOCK(cs_args);
@@ -595,6 +622,9 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 
     LOCK(cs_args);
     ArgsManagerHelper::AddArgs(result, m_override_args, strArg);
+    if (!m_network.empty()) {
+        ArgsManagerHelper::AddArgs(result, m_config_args, ArgsManagerHelper::NetworkArg(*this, strArg));
+    }
     ArgsManagerHelper::AddArgs(result, m_config_args, strArg);
     return result;
 }
@@ -612,6 +642,11 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
     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();
 
diff --git a/src/util.h b/src/util.h
index 6b80afc58f..ca6523ab14 100644
--- a/src/util.h
+++ b/src/util.h
@@ -228,9 +228,15 @@ protected:
     mutable CCriticalSection cs_args;
     std::map<std::string, std::vector<std::string>> m_override_args;
     std::map<std::string, std::vector<std::string>> m_config_args;
+    std::string m_network;
     void ReadConfigStream(std::istream& stream);
 
 public:
+    /**
+     * Select the network in use
+     */
+    void SelectConfigNetwork(const std::string& network);
+
     void ParseParameters(int argc, const char*const argv[]);
     void ReadConfigFile(const std::string& confPath);
 
-- 
cgit v1.2.3


From 30f94074c83f7c544a88251b7308bda8b9e0fdda Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:04:00 +1000
Subject: [tests] Unit tests for config file sections

---
 src/test/util_tests.cpp | 62 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index f04f6cf171..1eb0d654f4 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -322,15 +322,24 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
        "h=1\n"
        "noh=1\n"
        "noi=1\n"
-       "i=1\n";
+       "i=1\n"
+       "sec1.ccc=extend1\n"
+       "\n"
+       "[sec1]\n"
+       "ccc=extend2\n"
+       "h=1\n"
+       "[sec2]\n"
+       "ccc=extend3\n"
+       "iii=2\n";
 
     TestArgsManager test_args;
 
     test_args.ReadConfigString(str_config);
     // expectation: a, b, ccc, d, fff, ggg, h, i end up in map
+    // so do sec1.ccc, sec1.h, sec2.ccc, sec2.iii
 
     BOOST_CHECK(test_args.GetOverrideArgs().empty());
-    BOOST_CHECK(test_args.GetConfigArgs().size() == 8);
+    BOOST_CHECK(test_args.GetConfigArgs().size() == 12);
 
     BOOST_CHECK(test_args.GetConfigArgs().count("-a")
                 && test_args.GetConfigArgs().count("-b")
@@ -341,6 +350,11 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.GetConfigArgs().count("-h")
                 && test_args.GetConfigArgs().count("-i")
                );
+    BOOST_CHECK(test_args.GetConfigArgs().count("-sec1.ccc")
+                && test_args.GetConfigArgs().count("-sec1.h")
+                && test_args.GetConfigArgs().count("-sec2.ccc")
+                && test_args.GetConfigArgs().count("-sec2.iii")
+               );
 
     BOOST_CHECK(test_args.IsArgSet("-a")
                 && test_args.IsArgSet("-b")
@@ -351,6 +365,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.IsArgSet("-h")
                 && test_args.IsArgSet("-i")
                 && !test_args.IsArgSet("-zzz")
+                && !test_args.IsArgSet("-iii")
                );
 
     BOOST_CHECK(test_args.GetArg("-a", "xxx") == ""
@@ -362,6 +377,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                 && test_args.GetArg("-h", "xxx") == "0"
                 && test_args.GetArg("-i", "xxx") == "1"
                 && test_args.GetArg("-zzz", "xxx") == "xxx"
+                && test_args.GetArg("-iii", "xxx") == "xxx"
                );
 
     for (bool def : {false, true}) {
@@ -374,6 +390,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
                      && !test_args.GetBoolArg("-h", def)
                      && test_args.GetBoolArg("-i", def)
                      && test_args.GetBoolArg("-zzz", def) == def
+                     && test_args.GetBoolArg("-iii", def) == def
                    );
     }
 
@@ -405,6 +422,47 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     BOOST_CHECK(test_args.IsArgNegated("-h")); // last setting takes precedence
     BOOST_CHECK(!test_args.IsArgNegated("-i")); // last setting takes precedence
     BOOST_CHECK(!test_args.IsArgNegated("-zzz"));
+
+    // Test sections work
+    test_args.SelectConfigNetwork("sec1");
+
+    // same as original
+    BOOST_CHECK(test_args.GetArg("-a", "xxx") == ""
+                && test_args.GetArg("-b", "xxx") == "1"
+                && test_args.GetArg("-d", "xxx") == "e"
+                && test_args.GetArg("-fff", "xxx") == "0"
+                && test_args.GetArg("-ggg", "xxx") == "1"
+                && test_args.GetArg("-zzz", "xxx") == "xxx"
+                && test_args.GetArg("-iii", "xxx") == "xxx"
+               );
+    // section-specific setting
+    BOOST_CHECK(test_args.GetArg("-h", "xxx") == "1");
+    // section takes priority for multiple values
+    BOOST_CHECK(test_args.GetArg("-ccc", "xxx") == "extend1");
+    // check multiple values works
+    const std::vector<std::string> sec1_ccc_expected = {"extend1","extend2","argument","multiple"};
+    const auto& sec1_ccc_res = test_args.GetArgs("-ccc");
+    BOOST_CHECK_EQUAL_COLLECTIONS(sec1_ccc_res.begin(), sec1_ccc_res.end(), sec1_ccc_expected.begin(), sec1_ccc_expected.end());
+
+    test_args.SelectConfigNetwork("sec2");
+
+    // same as original
+    BOOST_CHECK(test_args.GetArg("-a", "xxx") == ""
+                && test_args.GetArg("-b", "xxx") == "1"
+                && test_args.GetArg("-d", "xxx") == "e"
+                && test_args.GetArg("-fff", "xxx") == "0"
+                && test_args.GetArg("-ggg", "xxx") == "1"
+                && test_args.GetArg("-zzz", "xxx") == "xxx"
+                && test_args.GetArg("-h", "xxx") == "0"
+               );
+    // section-specific setting
+    BOOST_CHECK(test_args.GetArg("-iii", "xxx") == "2");
+    // section takes priority for multiple values
+    BOOST_CHECK(test_args.GetArg("-ccc", "xxx") == "extend3");
+    // check multiple values works
+    const std::vector<std::string> sec2_ccc_expected = {"extend3","argument","multiple"};
+    const auto& sec2_ccc_res = test_args.GetArgs("-ccc");
+    BOOST_CHECK_EQUAL_COLLECTIONS(sec2_ccc_res.begin(), sec2_ccc_res.end(), sec2_ccc_expected.begin(), sec2_ccc_expected.end());
 }
 
 BOOST_AUTO_TEST_CASE(util_GetArg)
-- 
cgit v1.2.3


From 8a9817d175453c74ec060dcac026cd713bec98fa Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:05:00 +1000
Subject: [tests] Use regtest section in functional tests configs

---
 test/functional/feature_config_args.py | 7 ++++++-
 test/functional/test_framework/util.py | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index a1d22191af..e9924451d1 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -29,8 +29,13 @@ class ConfArgsTest(BitcoinTestFramework):
 
         # Check that using non-existent datadir in conf file fails
         conf_file = os.path.join(default_data_dir, "bitcoin.conf")
-        with open(conf_file, 'a', encoding='utf8') as f:
+
+        # datadir needs to be set before [regtest] section
+        conf_file_contents = open(conf_file, encoding='utf8').read()
+        with open(conf_file, 'w', encoding='utf8') as f:
             f.write("datadir=" + new_data_dir + "\n")
+            f.write(conf_file_contents)
+
         self.nodes[0].assert_start_raises_init_error(['-conf=' + conf_file], 'Error reading configuration file: specified data directory "' + new_data_dir + '" does not exist.')
 
         # Create the directory and ensure the config file now works
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index f22322fbbc..4ec3175cd6 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -294,6 +294,7 @@ def initialize_datadir(dirname, n):
         os.makedirs(datadir)
     with open(os.path.join(datadir, "bitcoin.conf"), 'w', encoding='utf8') as f:
         f.write("regtest=1\n")
+        f.write("[regtest]\n")
         f.write("port=" + str(p2p_port(n)) + "\n")
         f.write("rpcport=" + str(rpc_port(n)) + "\n")
         f.write("server=1\n")
-- 
cgit v1.2.3


From d1fc4d95afc191072fc650581a9b668b68b47b15 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:06:00 +1000
Subject: ArgsManager: limit some options to only apply on mainnet when in
 default section

When specified in bitcoin.conf without using the [regtest] or [test]
section header, or a "regtest." or "test." prefix, the "addnode",
"connect", "port", "bind", "rpcport", "rpcbind", and "wallet" settings
will only be applied when running on mainnet.
---
 src/util.cpp | 38 ++++++++++++++++++++++++++++++++++----
 src/util.h   |  5 +++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/src/util.cpp b/src/util.cpp
index 85c3d221af..51c28349f3 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -460,6 +460,13 @@ 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)
+    {
+        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)
     {
@@ -521,9 +528,11 @@ public:
             }
         }
 
-        found_result = GetArgHelper(am.m_config_args, 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;
@@ -575,6 +584,22 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
     return false;
 }
 
+ArgsManager::ArgsManager() :
+    /* These options would cause cross-contamination if values for
+     * mainnet were used while running on regtest/testnet (or vice-versa).
+     * Setting them as section_only_args ensures that sharing a config file
+     * between mainnet and regtest/testnet won't cause problems due to these
+     * parameters by accident. */
+    m_network_only_args{
+      "-addnode", "-connect",
+      "-port", "-bind",
+      "-rpcport", "-rpcbind",
+      "-wallet",
+    }
+{
+    // nothing to do
+}
+
 void ArgsManager::SelectConfigNetwork(const std::string& network)
 {
     m_network = network;
@@ -621,11 +646,16 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
     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));
     }
-    ArgsManagerHelper::AddArgs(result, m_config_args, strArg);
+
+    if (ArgsManagerHelper::UseDefaultSection(*this, strArg)) {
+        ArgsManagerHelper::AddArgs(result, m_config_args, strArg);
+    }
+
     return result;
 }
 
diff --git a/src/util.h b/src/util.h
index ca6523ab14..8132435628 100644
--- a/src/util.h
+++ b/src/util.h
@@ -24,6 +24,7 @@
 #include <exception>
 #include <map>
 #include <memory>
+#include <set>
 #include <stdint.h>
 #include <string>
 #include <unordered_set>
@@ -229,9 +230,13 @@ protected:
     std::map<std::string, std::vector<std::string>> m_override_args;
     std::map<std::string, std::vector<std::string>> m_config_args;
     std::string m_network;
+    std::set<std::string> m_network_only_args;
+
     void ReadConfigStream(std::istream& stream);
 
 public:
+    ArgsManager();
+
     /**
      * Select the network in use
      */
-- 
cgit v1.2.3


From 68797e20f478d835b7ff691a656242c14283446a Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:07:00 +1000
Subject: ArgsManager: Warn when ignoring network-specific config setting

When network-specific options such as -addnode, -connect, etc are
specified in the default section of the config file, but that setting is
ignored due to testnet or regtest being in use, and it is not overridden
by either a command line option or a setting in the [regtest] or [test]
section of the config file, a warning is added to the log, eg:

  Warning: Config setting for -connect only applied on regtest network when in [regtest] section.
---
 src/init.cpp |  5 +++++
 src/util.cpp | 28 ++++++++++++++++++++++++++++
 src/util.h   |  8 ++++++++
 3 files changed, 41 insertions(+)

diff --git a/src/init.cpp b/src/init.cpp
index 9edd93000f..8db1b922c4 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -803,6 +803,11 @@ void InitParameterInteraction()
         if (gArgs.SoftSetBoolArg("-whitelistrelay", true))
             LogPrintf("%s: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1\n", __func__);
     }
+
+    // Warn if network-specific options (-addnode, -connect, etc) are
+    // specified in default section of config file, but not overridden
+    // on the command line or in this network's section of the config file.
+    gArgs.WarnForSectionOnlyArgs();
 }
 
 static std::string ResolveErrMsg(const char * const optname, const std::string& strBind)
diff --git a/src/util.cpp b/src/util.cpp
index 51c28349f3..4101173091 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -600,6 +600,34 @@ ArgsManager::ArgsManager() :
     // nothing to do
 }
 
+void ArgsManager::WarnForSectionOnlyArgs()
+{
+    // if there's no section selected, don't worry
+    if (m_network.empty()) return;
+
+    // if it's okay to use the default section for this network, don't worry
+    if (m_network == CBaseChainParams::MAIN) return;
+
+    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
+        LogPrintf("Warning: Config setting for %s only applied on %s network when in [%s] section.\n", arg, m_network, m_network);
+    }
+}
+
 void ArgsManager::SelectConfigNetwork(const std::string& network)
 {
     m_network = network;
diff --git a/src/util.h b/src/util.h
index 8132435628..ba80c21ddc 100644
--- a/src/util.h
+++ b/src/util.h
@@ -245,6 +245,14 @@ public:
     void ParseParameters(int argc, const char*const argv[]);
     void ReadConfigFile(const std::string& confPath);
 
+    /**
+     * Log warnings for options in m_section_only_args when
+     * they are specified in the default section but not overridden
+     * on the command line or in a network-specific section in the
+     * config file.
+     */
+    void WarnForSectionOnlyArgs();
+
     /**
      * Return a vector of strings of the given argument
      *
-- 
cgit v1.2.3


From 608415d4e6c6518aed248f33a58e07ba1e3df4c0 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:08:00 +1000
Subject: [tests] Unit tests for network-specific config entries

---
 src/test/util_tests.cpp | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 1eb0d654f4..98ded7bc8c 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -187,6 +187,7 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Time)
 
 struct TestArgsManager : public ArgsManager
 {
+    TestArgsManager() { m_network_only_args.clear(); }
     std::map<std::string, std::vector<std::string> >& GetOverrideArgs() { return m_override_args; }
     std::map<std::string, std::vector<std::string> >& GetConfigArgs() { return m_config_args; }
     void ReadConfigString(const std::string str_config)
@@ -198,6 +199,11 @@ struct TestArgsManager : public ArgsManager
         }
         ReadConfigStream(streamConfig);
     }
+    void SetNetworkOnlyArg(const std::string arg)
+    {
+        LOCK(cs_args);
+        m_network_only_args.insert(arg);
+    }
 };
 
 BOOST_AUTO_TEST_CASE(util_ParseParameters)
@@ -327,6 +333,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
        "\n"
        "[sec1]\n"
        "ccc=extend2\n"
+       "d=eee\n"
        "h=1\n"
        "[sec2]\n"
        "ccc=extend3\n"
@@ -336,10 +343,10 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 
     test_args.ReadConfigString(str_config);
     // expectation: a, b, ccc, d, fff, ggg, h, i end up in map
-    // so do sec1.ccc, sec1.h, sec2.ccc, sec2.iii
+    // so do sec1.ccc, sec1.d, sec1.h, sec2.ccc, sec2.iii
 
     BOOST_CHECK(test_args.GetOverrideArgs().empty());
-    BOOST_CHECK(test_args.GetConfigArgs().size() == 12);
+    BOOST_CHECK(test_args.GetConfigArgs().size() == 13);
 
     BOOST_CHECK(test_args.GetConfigArgs().count("-a")
                 && test_args.GetConfigArgs().count("-b")
@@ -429,12 +436,13 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     // same as original
     BOOST_CHECK(test_args.GetArg("-a", "xxx") == ""
                 && test_args.GetArg("-b", "xxx") == "1"
-                && test_args.GetArg("-d", "xxx") == "e"
                 && test_args.GetArg("-fff", "xxx") == "0"
                 && test_args.GetArg("-ggg", "xxx") == "1"
                 && test_args.GetArg("-zzz", "xxx") == "xxx"
                 && test_args.GetArg("-iii", "xxx") == "xxx"
                );
+    // d is overridden
+    BOOST_CHECK(test_args.GetArg("-d", "xxx") == "eee");
     // section-specific setting
     BOOST_CHECK(test_args.GetArg("-h", "xxx") == "1");
     // section takes priority for multiple values
@@ -463,6 +471,29 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
     const std::vector<std::string> sec2_ccc_expected = {"extend3","argument","multiple"};
     const auto& sec2_ccc_res = test_args.GetArgs("-ccc");
     BOOST_CHECK_EQUAL_COLLECTIONS(sec2_ccc_res.begin(), sec2_ccc_res.end(), sec2_ccc_expected.begin(), sec2_ccc_expected.end());
+
+    // Test section only options
+
+    test_args.SetNetworkOnlyArg("-d");
+    test_args.SetNetworkOnlyArg("-ccc");
+    test_args.SetNetworkOnlyArg("-h");
+
+    test_args.SelectConfigNetwork(CBaseChainParams::MAIN);
+    BOOST_CHECK(test_args.GetArg("-d", "xxx") == "e");
+    BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2);
+    BOOST_CHECK(test_args.GetArg("-h", "xxx") == "0");
+
+    test_args.SelectConfigNetwork("sec1");
+    BOOST_CHECK(test_args.GetArg("-d", "xxx") == "eee");
+    BOOST_CHECK(test_args.GetArgs("-d").size() == 1);
+    BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2);
+    BOOST_CHECK(test_args.GetArg("-h", "xxx") == "1");
+
+    test_args.SelectConfigNetwork("sec2");
+    BOOST_CHECK(test_args.GetArg("-d", "xxx") == "xxx");
+    BOOST_CHECK(test_args.GetArgs("-d").size() == 0);
+    BOOST_CHECK(test_args.GetArgs("-ccc").size() == 1);
+    BOOST_CHECK(test_args.GetArg("-h", "xxx") == "0");
 }
 
 BOOST_AUTO_TEST_CASE(util_GetArg)
-- 
cgit v1.2.3


From 005ad266491f43d7a9bfd959396037416cb32a55 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:09:00 +1000
Subject: ArgsManager: special handling for -regtest and -testnet

---
 src/util.cpp | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/util.cpp b/src/util.cpp
index 4101173091..1fb40ae7a1 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -537,6 +537,22 @@ public:
 
         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)
+    {
+        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
+    }
 };
 
 /**
@@ -950,8 +966,8 @@ void ArgsManager::ReadConfigFile(const std::string& confPath)
 
 std::string ArgsManager::GetChainName() const
 {
-    bool fRegTest = GetBoolArg("-regtest", false);
-    bool fTestNet = GetBoolArg("-testnet", false);
+    bool fRegTest = ArgsManagerHelper::GetNetBoolArg(*this, "-regtest");
+    bool fTestNet = ArgsManagerHelper::GetNetBoolArg(*this, "-testnet");
 
     if (fTestNet && fRegTest)
         throw std::runtime_error("Invalid combination of -regtest and -testnet.");
-- 
cgit v1.2.3


From 5e3cbe020ddeaf4abf51cb561122056cce726d8b Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:10:00 +1000
Subject: [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections

---
 src/test/util_tests.cpp | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 98ded7bc8c..fa49d6d7ff 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -544,7 +544,8 @@ BOOST_AUTO_TEST_CASE(util_GetChainName)
     const char* argv_both[] = {"cmd", "-testnet", "-regtest"};
 
     // equivalent to "-testnet"
-    const char* testnetconf = "testnet=1\nregtest=0\n";
+    // regtest in testnet section is ignored
+    const char* testnetconf = "testnet=1\nregtest=0\n[test]\nregtest=1";
 
     test_args.ParseParameters(0, (char**)argv_testnet);
     BOOST_CHECK_EQUAL(test_args.GetChainName(), "main");
@@ -580,6 +581,30 @@ BOOST_AUTO_TEST_CASE(util_GetChainName)
     test_args.ParseParameters(3, (char**)argv_both);
     test_args.ReadConfigString(testnetconf);
     BOOST_CHECK_THROW(test_args.GetChainName(), std::runtime_error);
+
+    // check setting the network to test (and thus making
+    // [test] regtest=1 potentially relevent) doesn't break things
+    test_args.SelectConfigNetwork("test");
+
+    test_args.ParseParameters(0, (char**)argv_testnet);
+    test_args.ReadConfigString(testnetconf);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
+
+    test_args.ParseParameters(2, (char**)argv_testnet);
+    test_args.ReadConfigString(testnetconf);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
+
+    test_args.ParseParameters(2, (char**)argv_regtest);
+    test_args.ReadConfigString(testnetconf);
+    BOOST_CHECK_THROW(test_args.GetChainName(), std::runtime_error);
+
+    test_args.ParseParameters(2, (char**)argv_test_no_reg);
+    test_args.ReadConfigString(testnetconf);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
+
+    test_args.ParseParameters(3, (char**)argv_both);
+    test_args.ReadConfigString(testnetconf);
+    BOOST_CHECK_THROW(test_args.GetChainName(), std::runtime_error);
 }
 
 BOOST_AUTO_TEST_CASE(util_FormatMoney)
-- 
cgit v1.2.3


From c25321ff96737bdba80d626d2425ef02c7a4c181 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Wed, 4 Apr 2018 18:11:00 +1000
Subject: Add config changes to release notes

---
 doc/release-notes-pr12823.md | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 doc/release-notes-pr12823.md

diff --git a/doc/release-notes-pr12823.md b/doc/release-notes-pr12823.md
new file mode 100644
index 0000000000..b493908716
--- /dev/null
+++ b/doc/release-notes-pr12823.md
@@ -0,0 +1,20 @@
+Configuration sections for testnet and regtest
+----------------------------------------------
+
+It is now possible for a single configuration file to set different
+options for different networks. This is done by using sections or by
+prefixing the option with the network, such as:
+
+    main.uacomment=bitcoin
+    test.uacomment=bitcoin-testnet
+    regtest.uacomment=regtest
+    [main]
+    mempoolsize=300
+    [test]
+    mempoolsize=100
+    [regtest]
+    mempoolsize=20
+
+The `addnode=`, `connect=`, `port=`, `bind=`, `rpcport=`, `rpcbind=`
+and `wallet=` options will only apply to mainnet when specified in the
+configuration file, unless a network is specified.
-- 
cgit v1.2.3