Merge #12878: [refactor] Config handling refactoring in preparation for network-specific sections

77a733a99 [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns)
af173c2be [tests] Check GetChainName works with config entries (Anthony Towns)
fa27f1c23 [tests] Add unit tests for ReadConfigStream (Anthony Towns)
087c5d204 ReadConfigStream: assume the stream is good (Anthony Towns)
6d5815aad Separate out ReadConfigStream from ReadConfigFile (Anthony Towns)
834d30341 [tests] Add unit tests for GetChainName (Anthony Towns)
11b6b5b86 Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns)

Pull request description:

  This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in #11862 easier. Should not cause any behaviour changes.

Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

10 files changed, 245 insertions, 46 deletions

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 618b0d16bc..26a9231308 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -114,7 +114,7 @@ static int AppInitRPC(int argc, char* argv[])
     }
     // Check for -testnet or -regtest parameter (BaseParams() calls are only valid after this clause)
     try {
-        SelectBaseParams(ChainNameFromCommandLine());
+        SelectBaseParams(gArgs.GetChainName());
     } catch (const std::exception& e) {
         fprintf(stderr, "Error: %s\n", e.what());
         return EXIT_FAILURE;
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 65f8177daf..deb8212a8f 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -44,7 +44,7 @@ static int AppInitRawTx(int argc, char* argv[])
 
     // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
     try {
-        SelectParams(ChainNameFromCommandLine());
+        SelectParams(gArgs.GetChainName());
     } catch (const std::exception& e) {
         fprintf(stderr, "Error: %s\n", e.what());
         return EXIT_FAILURE;
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index b00c2a6308..4aa811b86b 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -102,7 +102,7 @@ bool AppInit(int argc, char* argv[])
         }
         // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
         try {
-            SelectParams(ChainNameFromCommandLine());
+            SelectParams(gArgs.GetChainName());
         } catch (const std::exception& e) {
             fprintf(stderr, "Error: %s\n", e.what());
             return false;
diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
index 726616e650..c51a3fc960 100644
--- a/src/chainparamsbase.cpp
+++ b/src/chainparamsbase.cpp
@@ -49,17 +49,3 @@ void SelectBaseParams(const std::string& chain)
 {
     globalChainBaseParams = CreateBaseChainParams(chain);
 }
-
-std::string ChainNameFromCommandLine()
-{
-    bool fRegTest = gArgs.GetBoolArg("-regtest", false);
-    bool fTestNet = gArgs.GetBoolArg("-testnet", false);
-
-    if (fTestNet && fRegTest)
-        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
-    if (fRegTest)
-        return CBaseChainParams::REGTEST;
-    if (fTestNet)
-        return CBaseChainParams::TESTNET;
-    return CBaseChainParams::MAIN;
-}
diff --git a/src/chainparamsbase.h b/src/chainparamsbase.h
index 2cb860380e..5b11f36770 100644
--- a/src/chainparamsbase.h
+++ b/src/chainparamsbase.h
@@ -54,10 +54,4 @@ const CBaseChainParams& BaseParams();
 /** Sets the params returned by Params() to those for the given network. */
 void SelectBaseParams(const std::string& chain);
 
-/**
- * Looks for -regtest, -testnet and returns the appropriate BIP70 chain name.
- * @return CBaseChainParams::MAX_NETWORK_TYPES if an invalid combination is given. CBaseChainParams::MAIN by default.
- */
-std::string ChainNameFromCommandLine();
-
 #endif // BITCOIN_CHAINPARAMSBASE_H
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index f6714a8e18..599c3c0985 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -627,7 +627,7 @@ int main(int argc, char *argv[])
 
     // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
     try {
-        node->selectParams(ChainNameFromCommandLine());
+        node->selectParams(gArgs.GetChainName());
     } catch(std::exception &e) {
         QMessageBox::critical(0, QObject::tr(PACKAGE_NAME), QObject::tr("Error: %1").arg(e.what()));
         return EXIT_FAILURE;
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
index c76fc4ca6d..563f930dec 100644
--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -599,7 +599,7 @@ TableViewLastColumnResizingFixer::TableViewLastColumnResizingFixer(QTableView* t
 #ifdef WIN32
 fs::path static StartupShortcutPath()
 {
-    std::string chain = ChainNameFromCommandLine();
+    std::string chain = gArgs.GetChainName();
     if (chain == CBaseChainParams::MAIN)
         return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin.lnk";
     if (chain == CBaseChainParams::TESTNET) // Remove this special case when CBaseChainParams::TESTNET = "testnet4"
@@ -697,7 +697,7 @@ fs::path static GetAutostartDir()
 
 fs::path static GetAutostartFilePath()
 {
-    std::string chain = ChainNameFromCommandLine();
+    std::string chain = gArgs.GetChainName();
     if (chain == CBaseChainParams::MAIN)
         return GetAutostartDir() / "bitcoin.desktop";
     return GetAutostartDir() / strprintf("bitcoin-%s.lnk", chain);
@@ -739,7 +739,7 @@ bool SetStartOnSystemStartup(bool fAutoStart)
         fs::ofstream optionFile(GetAutostartFilePath(), std::ios_base::out|std::ios_base::trunc);
         if (!optionFile.good())
             return false;
-        std::string chain = ChainNameFromCommandLine();
+        std::string chain = gArgs.GetChainName();
         // Write a bitcoin.desktop file to the autostart directory:
         optionFile << "[Desktop Entry]\n";
         optionFile << "Type=Application\n";
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 4b44bbadac..b4af3617f1 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -190,6 +190,11 @@ 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; }
     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);
+    }
 };
 
 BOOST_AUTO_TEST_CASE(util_ParseParameters)
@@ -253,16 +258,154 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
 {
     // Test some awful edge cases that hopefully no user will ever exercise.
     TestArgsManager testArgs;
+
+    // Params test
     const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"};
     testArgs.ParseParameters(4, (char**)argv_test);
 
     // This was passed twice, second one overrides the negative setting.
     BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
-    BOOST_CHECK(testArgs.GetBoolArg("-foo", false) == true);
+    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "");
+
+    // A double negative is a positive.
+    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
+    BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1");
+
+    // Config test
+    const char *conf_test = "nofoo=1\nfoo=1\nnobar=0\n";
+    testArgs.ParseParameters(1, (char**)argv_test);
+    testArgs.ReadConfigString(conf_test);
+
+    // This was passed twice, second one overrides the negative setting,
+    // but not the value.
+    BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
+    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0");
 
     // A double negative is a positive.
     BOOST_CHECK(testArgs.IsArgNegated("-bar"));
-    BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true);
+    BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1");
+
+    // Combined test
+    const char *combo_test_args[] = {"ignored", "-nofoo", "-bar"};
+    const char *combo_test_conf = "foo=1\nnobar=1\n";
+    testArgs.ParseParameters(3, (char**)combo_test_args);
+    testArgs.ReadConfigString(combo_test_conf);
+
+    // Command line overrides, but doesn't erase old setting
+    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");
+
+    // Command line overrides, but doesn't erase old setting
+    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_AUTO_TEST_CASE(util_ReadConfigStream)
+{
+    const char *str_config =
+       "a=\n"
+       "b=1\n"
+       "ccc=argument\n"
+       "ccc=multiple\n"
+       "d=e\n"
+       "nofff=1\n"
+       "noggg=0\n"
+       "h=1\n"
+       "noh=1\n"
+       "noi=1\n"
+       "i=1\n";
+
+    TestArgsManager test_args;
+
+    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.IsArgSet("-a")
+                && test_args.IsArgSet("-b")
+                && test_args.IsArgSet("-ccc")
+                && test_args.IsArgSet("-d")
+                && test_args.IsArgSet("-fff")
+                && test_args.IsArgSet("-ggg")
+                && test_args.IsArgSet("-h")
+                && test_args.IsArgSet("-i")
+                && !test_args.IsArgSet("-zzz")
+               );
+
+    BOOST_CHECK(test_args.GetArg("-a", "xxx") == ""
+                && test_args.GetArg("-b", "xxx") == "1"
+                && test_args.GetArg("-ccc", "xxx") == "argument"
+                && 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("-zzz", "xxx") == "xxx"
+               );
+
+    for (bool def : {false, true}) {
+        BOOST_CHECK(test_args.GetBoolArg("-a", def)
+                     && test_args.GetBoolArg("-b", def)
+                     && !test_args.GetBoolArg("-ccc", def)
+                     && !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("-zzz", def) == def
+                   );
+    }
+
+    BOOST_CHECK(test_args.GetArgs("-a").size() == 1
+                && test_args.GetArgs("-a").front() == "");
+    BOOST_CHECK(test_args.GetArgs("-b").size() == 1
+                && test_args.GetArgs("-b").front() == "1");
+    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("-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("-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("-noi").size() == 0);
+    BOOST_CHECK(test_args.GetArgs("-zzz").size() == 0);
+
+    BOOST_CHECK(!test_args.IsArgNegated("-a"));
+    BOOST_CHECK(!test_args.IsArgNegated("-b"));
+    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("-h")); // last setting takes precedence
+    BOOST_CHECK(!test_args.IsArgNegated("-i")); // last setting takes precedence
+    BOOST_CHECK(!test_args.IsArgNegated("-zzz"));
 }
 
 BOOST_AUTO_TEST_CASE(util_GetArg)
@@ -290,6 +433,54 @@ BOOST_AUTO_TEST_CASE(util_GetArg)
     BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest4", false), true);
 }
 
+BOOST_AUTO_TEST_CASE(util_GetChainName)
+{
+    TestArgsManager test_args;
+
+    const char* argv_testnet[] = {"cmd", "-testnet"};
+    const char* argv_regtest[] = {"cmd", "-regtest"};
+    const char* argv_test_no_reg[] = {"cmd", "-testnet", "-noregtest"};
+    const char* argv_both[] = {"cmd", "-testnet", "-regtest"};
+
+    // equivalent to "-testnet"
+    const char* testnetconf = "testnet=1\nregtest=0\n";
+
+    test_args.ParseParameters(0, (char**)argv_testnet);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "main");
+
+    test_args.ParseParameters(2, (char**)argv_testnet);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
+
+    test_args.ParseParameters(2, (char**)argv_regtest);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "regtest");
+
+    test_args.ParseParameters(3, (char**)argv_test_no_reg);
+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
+
+    test_args.ParseParameters(3, (char**)argv_both);
+    BOOST_CHECK_THROW(test_args.GetChainName(), std::runtime_error);
+
+    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(3, (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)
 {
     BOOST_CHECK_EQUAL(FormatMoney(0), "0.00");
diff --git a/src/util.cpp b/src/util.cpp
index 8b36c3e5f7..393cc413d6 100644
--- a/src/util.cpp
+++ b/src/util.cpp
@@ -736,28 +736,34 @@ fs::path GetConfigFile(const std::string& confPath)
     return AbsPathForConfigVal(fs::path(confPath), false);
 }
 
-void ArgsManager::ReadConfigFile(const std::string& confPath)
+void ArgsManager::ReadConfigStream(std::istream& stream)
 {
-    fs::ifstream streamConfig(GetConfigFile(confPath));
-    if (!streamConfig.good())
-        return; // No bitcoin.conf file is OK
+    LOCK(cs_args);
 
+    std::set<std::string> setOptions;
+    setOptions.insert("*");
+
+    for (boost::program_options::detail::config_file_iterator it(stream, setOptions), end; it != end; ++it)
     {
-        LOCK(cs_args);
-        std::set<std::string> setOptions;
-        setOptions.insert("*");
+        // 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);
+        if (mapArgs.count(strKey) == 0)
+            mapArgs[strKey] = strValue;
+        mapMultiArgs[strKey].push_back(strValue);
+    }
+}
 
-        for (boost::program_options::detail::config_file_iterator it(streamConfig, 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);
-            if (mapArgs.count(strKey) == 0)
-                mapArgs[strKey] = strValue;
-            mapMultiArgs[strKey].push_back(strValue);
-        }
+void ArgsManager::ReadConfigFile(const std::string& confPath)
+{
+    fs::ifstream stream(GetConfigFile(confPath));
+
+    // ok to not have a config file
+    if (stream.good()) {
+        ReadConfigStream(stream);
     }
+
     // If datadir is changed in .conf file:
     ClearDatadirCache();
     if (!fs::is_directory(GetDataDir(false))) {
@@ -765,6 +771,20 @@ void ArgsManager::ReadConfigFile(const std::string& confPath)
     }
 }
 
+std::string ArgsManager::GetChainName() const
+{
+    bool fRegTest = GetBoolArg("-regtest", false);
+    bool fTestNet = GetBoolArg("-testnet", false);
+
+    if (fTestNet && fRegTest)
+        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
+    if (fRegTest)
+        return CBaseChainParams::REGTEST;
+    if (fTestNet)
+        return CBaseChainParams::TESTNET;
+    return CBaseChainParams::MAIN;
+}
+
 #ifndef WIN32
 fs::path GetPidFile()
 {
diff --git a/src/util.h b/src/util.h
index f625a61d8b..3952461e48 100644
--- a/src/util.h
+++ b/src/util.h
@@ -228,6 +228,8 @@ protected:
     std::map<std::string, std::vector<std::string>> mapMultiArgs;
     std::unordered_set<std::string> m_negated_args;
 
+    void ReadConfigStream(std::istream& stream);
+
 public:
     void ParseParameters(int argc, const char*const argv[]);
     void ReadConfigFile(const std::string& confPath);
@@ -306,6 +308,12 @@ public:
     // been set. Also called directly in testing.
     void ForceSetArg(const std::string& strArg, const std::string& strValue);
 
+    /**
+     * Looks for -regtest, -testnet and returns the appropriate BIP70 chain name.
+     * @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.