Merge 5447097 into merged_master (Bitcoin PR #18571)

This resulted in double-initializing the asset directory (and from multiple places), so I added a `ClearGlobalAssetDir` method and called it from the qt test setup.
ElementsProject · Nov 26, 2020 · 2016bea · 2016bea
2 parents 713ff5a + 5447097
commit 2016bea
Show file tree

Hide file tree

Showing 13 changed files with 71 additions and 15 deletions.
diff --git a/src/assetsdir.cpp b/src/assetsdir.cpp
@@ -104,3 +104,10 @@ void InitGlobalAssetDir(const std::vector<std::string>& assetsToInit, const std:
 {
     _gAssetsDir.InitFromStrings(assetsToInit, pegged_asset_name);
 }
+
+// Used in testing
+void ClearGlobalAssetDir()
+{
+    _gAssetsDir = CAssetsDir();
+}
+
diff --git a/src/assetsdir.h b/src/assetsdir.h
@@ -59,5 +59,7 @@ class CAssetsDir;
 extern const CAssetsDir& gAssetsDir;
 
 void InitGlobalAssetDir(const std::vector<std::string>& assetsToInit, const std::string& pegged_asset_name);
+// Used in testing
+void ClearGlobalAssetDir(void);
 
 #endif // BITCOIN_ASSETSDIR_H
diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
@@ -73,6 +73,8 @@ int main(int argc, char** argv)
             gArgs.GetArg("-plot-height", DEFAULT_PLOT_HEIGHT)));
     }
 
+    gArgs.ClearArgs(); // gArgs no longer needed. Clear it here to avoid interactions with the testing setup in the benches
+
     benchmark::BenchRunner::RunAll(*printer, evaluations, scaling_factor, regex_filter, is_list_only);
 
     return EXIT_SUCCESS;

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
@@ -53,7 +53,7 @@ static bool AppInit(int argc, char* argv[])
     // Parameters
     //
     // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main()
-    SetupServerArgs();
+    SetupServerArgs(node);
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
         return InitError(strprintf("Error parsing command line arguments: %s\n", error));

diff --git a/src/init.cpp b/src/init.cpp
@@ -301,6 +301,7 @@ void Shutdown(NodeContext& node)
     GetMainSignals().UnregisterBackgroundSignalScheduler();
     globalVerifyHandle.reset();
     ECC_Stop();
+    node.args = nullptr;
     if (node.mempool) node.mempool = nullptr;
     node.scheduler.reset();
     node.reverification_scheduler.reset();
@@ -365,8 +366,11 @@ static void OnRPCStopped()
     LogPrint(BCLog::RPC, "RPC stopped.\n");
 }
 
-void SetupServerArgs()
+void SetupServerArgs(NodeContext& node)
 {
+    assert(!node.args);
+    node.args = &gArgs;
+
     SetupHelpOptions(gArgs);
     gArgs.AddArg("-help-debug", "Print help message with debugging options and exit", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); // server-only for now
 

diff --git a/src/init.h b/src/init.h
@@ -54,9 +54,9 @@ bool AppInitLockDataDirectory();
 bool AppInitMain(NodeContext& node);
 
 /**
- * Setup the arguments for gArgs
+ * Register all arguments with the ArgsManager
  */
-void SetupServerArgs();
+void SetupServerArgs(NodeContext& node);
 
 /** Returns licensing information (for -version) */
 std::string LicenseInfo();

diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
@@ -97,7 +97,7 @@ class NodeImpl : public Node
             StopMapPort();
         }
     }
-    void setupServerArgs() override { return SetupServerArgs(); }
+    void setupServerArgs() override { return SetupServerArgs(m_context); }
     bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
     size_t getNodeCount(CConnman::NumConnections flags) override
     {

diff --git a/src/node/context.h b/src/node/context.h
@@ -8,6 +8,7 @@
 #include <memory>
 #include <vector>
 
+class ArgsManager;
 class BanMan;
 class CConnman;
 class CScheduler;
@@ -33,6 +34,7 @@ struct NodeContext {
     CTxMemPool* mempool{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
     std::unique_ptr<PeerLogicValidation> peer_logic;
     std::unique_ptr<BanMan> banman;
+    ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
     std::unique_ptr<interfaces::Chain> chain;
     std::vector<std::unique_ptr<interfaces::ChainClient>> chain_clients;
     std::unique_ptr<CScheduler> scheduler;

diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
@@ -4,6 +4,7 @@
 
 #include <qt/test/apptests.h>
 
+#include <assetsdir.h>
 #include <chainparams.h>
 #include <key.h>
 #include <qt/bitcoin.h>
@@ -65,6 +66,9 @@ void AppTests::appTests()
     BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
     ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
     LogInstance().DisconnectTestLogger();
+    // ELEMENTS: asset dir initialized by common test setup, will be re-set by
+    //  baseInitialize below, clear it to avoid "duplicate asset" errors
+    ClearGlobalAssetDir();
 
     m_app.parameterSetup();
     m_app.createOptionsModel(true /* reset settings */);

diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
@@ -57,12 +57,17 @@ const std::map<std::string, std::set<std::string>> EXPECTED_DESERIALIZATION_EXCE
     {"Unknown transaction optional data: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}},
 };
 
-const RegTestingSetup* g_setup;
+const TestingSetup* g_setup;
 } // namespace
 
 void initialize()
 {
-    static RegTestingSetup setup{};
+    static TestingSetup setup{
+        CBaseChainParams::REGTEST,
+        {
+            "-nodebuglogfile",
+        },
+    };
     g_setup = &setup;
 
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {

diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
@@ -16,11 +16,16 @@
 #include <validation.h>
 #include <validationinterface.h>
 
-const RegTestingSetup* g_setup;
+const TestingSetup* g_setup;
 
 void initialize()
 {
-    static RegTestingSetup setup{};
+    static TestingSetup setup{
+        CBaseChainParams::REGTEST,
+        {
+            "-nodebuglogfile",
+        },
+    };
     g_setup = &setup;
 
     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -5,6 +5,7 @@
 #include <test/util/setup_common.h>
 
 #include <asset.h>
+#include <assetsdir.h>
 #include <banman.h>
 #include <chainparams.h>
 #include <consensus/consensus.h>
@@ -31,6 +32,7 @@
 #include <util/time.h>
 #include <util/translation.h>
 #include <util/url.h>
+#include <util/vector.h>
 #include <validation.h>
 #include <validationinterface.h>
 
@@ -68,7 +70,7 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
     return os;
 }
 
-BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::string& fedpegscript)
+BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::string& fedpegscript, const std::vector<const char*>& extra_args)
     : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()}
 {
     // Hack to allow testing of fedpeg args
@@ -78,14 +80,32 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::st
         gArgs.SoftSetBoolArg("-validatepegin", false);
     }
 
+    const std::vector<const char*> arguments = Cat(
+        {
+            "dummy",
+            "-printtoconsole=0",
+            "-logtimemicros",
+            "-debug",
+            "-debugexclude=libevent",
+            "-debugexclude=leveldb",
+        },
+        extra_args);
+
     fs::create_directories(m_path_root);
     gArgs.ForceSetArg("-datadir", m_path_root.string());
     ClearDatadirCache();
+    {
+        SetupServerArgs(m_node);
+        std::string error;
+        const bool success{m_node.args->ParseParameters(arguments.size(), arguments.data(), error)};
+        assert(success);
+        assert(error.empty());
+    }
     SelectParams(chainName);
     SeedInsecureRand();
-    gArgs.ForceSetArg("-printtoconsole", "0");
     if (G_TEST_LOG_FUN) LogInstance().PushBackCallback(G_TEST_LOG_FUN);
     InitLogging();
+    AppInitParameterInteraction();
     LogInstance().StartLogging();
     SHA256AutoDetect();
     ECC_Start();
@@ -115,12 +135,15 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::st
 
 BasicTestingSetup::~BasicTestingSetup()
 {
+    ClearGlobalAssetDir();
     LogInstance().DisconnectTestLogger();
     fs::remove_all(m_path_root);
+    gArgs.ClearArgs();
     ECC_Stop();
 }
 
-TestingSetup::TestingSetup(const std::string& chainName, const std::string& fedpegscript) : BasicTestingSetup(chainName, fedpegscript)
+TestingSetup::TestingSetup(const std::string& chainName, const std::string& fedpegscript, const std::vector<const char*>& extra_args)
+    : BasicTestingSetup(chainName, fedpegscript, extra_args)
 {
     const CChainParams& chainparams = Params();
     // Ideally we'd move all the RPC tests to the functional testing framework
@@ -181,6 +204,7 @@ TestingSetup::~TestingSetup()
     g_rpc_node = nullptr;
     m_node.connman.reset();
     m_node.banman.reset();
+    m_node.args = nullptr;
     m_node.mempool = nullptr;
     m_node.scheduler.reset();
     UnloadBlockIndex();

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
@@ -73,9 +73,11 @@ static constexpr CAmount CENT{1000000};
  */
 struct BasicTestingSetup {
     ECCVerifyHandle globalVerifyHandle;
+    NodeContext m_node;
 
-    explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::string& fedpegscript = "");
+    explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::string& fedpegscript = "", const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();
+
 private:
     const fs::path m_path_root;
 };
@@ -84,10 +86,9 @@ struct BasicTestingSetup {
  * Included are coins database, script check threads setup.
  */
 struct TestingSetup : public BasicTestingSetup {
-    NodeContext m_node;
     boost::thread_group threadGroup;
 
-    explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::string& fedpegscript = "");
+    explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::string& fedpegscript = "", const std::vector<const char*>& extra_args = {});
     ~TestingSetup();
 };