Merge bitcoin/bitcoin#27239: refactor: Consistently use context args over gArgs in node/interfaces

fanquake · fanquake · commit 40d0b0a524df · 2023-03-11T11:20:01.000+01:00
fa3e9b4 refactor: Consistently use args over gArgs in init.cpp (MarcoFalke) fa89112 refactor: Consistently use context args over gArgs in node/interfaces (MarcoFalke) Pull request description: Currently `node/interfaces.cpp` uses a mix of `gArgs` vs `m_context->args`. This is fine, because outside of tests those should be identical. However, it makes the code inconsistent and harder to use in tests. Fix that by using `args` from the context consistently. Do the same in `init.cpp`, where `gArgs` and `args` are inconsistently used in the same scope or even line. ACKs for top commit: ryanofsky: Code review ACK fa3e9b4 Tree-SHA512: bcd52f176794ebb1ecb9e1922411f7b84d212ae13bad314a1961b85f3077f645fca71fb0124c0889ebfdd8b59a0903b99b9985b1a4fb8f152aa6d7f0126fe5c7
diff --git a/src/init.cpp b/src/init.cpp
@@ -867,7 +867,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         InitWarning(warnings);
     }
 
-    if (!fs::is_directory(gArgs.GetBlocksDirPath())) {
+    if (!fs::is_directory(args.GetBlocksDirPath())) {
         return InitError(strprintf(_("Specified blocks directory \"%s\" does not exist."), args.GetArg("-blocksdir", "")));
     }
 
@@ -1224,7 +1224,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         if (args.IsArgSet("-asmap")) {
             fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
             if (!asmap_path.is_absolute()) {
-                asmap_path = gArgs.GetDataDirNet() / asmap_path;
+                asmap_path = args.GetDataDirNet() / asmap_path;
             }
             if (!fs::exists(asmap_path)) {
                 InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
@@ -1254,7 +1254,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
 
     assert(!node.banman);
-    node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
+    node.banman = std::make_unique<BanMan>(args.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
     node.connman = std::make_unique<CConnman>(GetRand<uint64_t>(),
                                               GetRand<uint64_t>(),
@@ -1618,12 +1618,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 11: import blocks
 
-    if (!CheckDiskSpace(gArgs.GetDataDirNet())) {
-        InitError(strprintf(_("Error: Disk space is low for %s"), fs::quoted(fs::PathToString(gArgs.GetDataDirNet()))));
+    if (!CheckDiskSpace(args.GetDataDirNet())) {
+        InitError(strprintf(_("Error: Disk space is low for %s"), fs::quoted(fs::PathToString(args.GetDataDirNet()))));
         return false;
     }
-    if (!CheckDiskSpace(gArgs.GetBlocksDirPath())) {
-        InitError(strprintf(_("Error: Disk space is low for %s"), fs::quoted(fs::PathToString(gArgs.GetBlocksDirPath()))));
+    if (!CheckDiskSpace(args.GetBlocksDirPath())) {
+        InitError(strprintf(_("Error: Disk space is low for %s"), fs::quoted(fs::PathToString(args.GetBlocksDirPath()))));
         return false;
     }
 
@@ -1715,7 +1715,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     if (node.peerman) node.peerman->SetBestHeight(chain_active_height);
 
     // Map ports with UPnP or NAT-PMP.
-    StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), gArgs.GetBoolArg("-natpmp", DEFAULT_NATPMP));
+    StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), args.GetBoolArg("-natpmp", DEFAULT_NATPMP));
 
     CConnman::Options connOptions;
     connOptions.nLocalServices = nLocalServices;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -86,14 +86,14 @@ class NodeImpl : public Node
 {
 public:
     explicit NodeImpl(NodeContext& context) { setContext(&context); }
-    void initLogging() override { InitLogging(*Assert(m_context->args)); }
-    void initParameterInteraction() override { InitParameterInteraction(*Assert(m_context->args)); }
+    void initLogging() override { InitLogging(args()); }
+    void initParameterInteraction() override { InitParameterInteraction(args()); }
     bilingual_str getWarnings() override { return GetWarnings(true); }
     uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
     bool baseInitialize() override
     {
-        if (!AppInitBasicSetup(gArgs)) return false;
-        if (!AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false)) return false;
+        if (!AppInitBasicSetup(args())) return false;
+        if (!AppInitParameterInteraction(args(), /*use_syscall_sandbox=*/false)) return false;
 
         m_context->kernel = std::make_unique<kernel::Context>();
         if (!AppInitSanityChecks(*m_context->kernel)) return false;
@@ -116,7 +116,7 @@ class NodeImpl : public Node
     {
         StartShutdown();
         // Stop RPC for clean shutdown if any of waitfor* commands is executed.
-        if (gArgs.GetBoolArg("-server", false)) {
+        if (args().GetBoolArg("-server", false)) {
             InterruptRPC();
             StopRPC();
         }
@@ -125,28 +125,28 @@ class NodeImpl : public Node
     bool isSettingIgnored(const std::string& name) override
     {
         bool ignored = false;
-        gArgs.LockSettings([&](util::Settings& settings) {
+        args().LockSettings([&](util::Settings& settings) {
             if (auto* options = util::FindKey(settings.command_line_options, name)) {
                 ignored = !options->empty();
             }
         });
         return ignored;
     }
-    util::SettingsValue getPersistentSetting(const std::string& name) override { return gArgs.GetPersistentSetting(name); }
+    util::SettingsValue getPersistentSetting(const std::string& name) override { return args().GetPersistentSetting(name); }
     void updateRwSetting(const std::string& name, const util::SettingsValue& value) override
     {
-        gArgs.LockSettings([&](util::Settings& settings) {
+        args().LockSettings([&](util::Settings& settings) {
             if (value.isNull()) {
                 settings.rw_settings.erase(name);
             } else {
                 settings.rw_settings[name] = value;
             }
         });
-        gArgs.WriteSettingsFile();
+        args().WriteSettingsFile();
     }
     void forceSetting(const std::string& name, const util::SettingsValue& value) override
     {
-        gArgs.LockSettings([&](util::Settings& settings) {
+        args().LockSettings([&](util::Settings& settings) {
             if (value.isNull()) {
                 settings.forced_settings.erase(name);
             } else {
@@ -156,11 +156,11 @@ class NodeImpl : public Node
     }
     void resetSettings() override
     {
-        gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true);
-        gArgs.LockSettings([&](util::Settings& settings) {
+        args().WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true);
+        args().LockSettings([&](util::Settings& settings) {
             settings.rw_settings.clear();
         });
-        gArgs.WriteSettingsFile();
+        args().WriteSettingsFile();
     }
     void mapPort(bool use_upnp, bool use_natpmp) override { StartMapPort(use_upnp, use_natpmp); }
     bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }
@@ -237,7 +237,7 @@ class NodeImpl : public Node
     {
 #ifdef ENABLE_EXTERNAL_SIGNER
         std::vector<ExternalSigner> signers = {};
-        const std::string command = gArgs.GetArg("-signer", "");
+        const std::string command = args().GetArg("-signer", "");
         if (command == "") return {};
         ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
         std::vector<std::unique_ptr<interfaces::ExternalSigner>> result;
@@ -388,6 +388,7 @@ class NodeImpl : public Node
     {
         m_context = context;
     }
+    ArgsManager& args() { return *Assert(Assert(m_context)->args); }
     ChainstateManager& chainman() { return *Assert(m_context->chainman); }
     NodeContext* m_context{nullptr};
 };
@@ -744,16 +745,16 @@ class ChainImpl : public Chain
     int rpcSerializationFlags() override { return RPCSerializationFlags(); }
     util::SettingsValue getSetting(const std::string& name) override
     {
-        return gArgs.GetSetting(name);
+        return args().GetSetting(name);
     }
     std::vector<util::SettingsValue> getSettingsList(const std::string& name) override
     {
-        return gArgs.GetSettingsList(name);
+        return args().GetSettingsList(name);
     }
     util::SettingsValue getRwSetting(const std::string& name) override
     {
         util::SettingsValue result;
-        gArgs.LockSettings([&](const util::Settings& settings) {
+        args().LockSettings([&](const util::Settings& settings) {
             if (const util::SettingsValue* value = util::FindKey(settings.rw_settings, name)) {
                 result = *value;
             }
@@ -762,14 +763,14 @@ class ChainImpl : public Chain
     }
     bool updateRwSetting(const std::string& name, const util::SettingsValue& value, bool write) override
     {
-        gArgs.LockSettings([&](util::Settings& settings) {
+        args().LockSettings([&](util::Settings& settings) {
             if (value.isNull()) {
                 settings.rw_settings.erase(name);
             } else {
                 settings.rw_settings[name] = value;
             }
         });
-        return !write || gArgs.WriteSettingsFile();
+        return !write || args().WriteSettingsFile();
     }
     void requestMempoolTransactions(Notifications& notifications) override
     {
@@ -785,6 +786,7 @@ class ChainImpl : public Chain
     }
 
     NodeContext* context() override { return &m_node; }
+    ArgsManager& args() { return *Assert(m_node.args); }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     NodeContext& m_node;
 };

Original file line number	Diff line number	Diff line change
`@@ -86,14 +86,14 @@ class NodeImpl : public Node`
`86`	`86`	`{`
`87`	`87`	`public:`
`88`	`88`	`explicit NodeImpl(NodeContext& context) { setContext(&context); }`
`89`		`- void initLogging() override { InitLogging(*Assert(m_context->args)); }`
`90`		`- void initParameterInteraction() override { InitParameterInteraction(*Assert(m_context->args)); }`
	`89`	`+ void initLogging() override { InitLogging(args()); }`
	`90`	`+ void initParameterInteraction() override { InitParameterInteraction(args()); }`
`91`	`91`	`bilingual_str getWarnings() override { return GetWarnings(true); }`
`92`	`92`	`uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }`
`93`	`93`	`bool baseInitialize() override`
`94`	`94`	`{`
`95`		`- if (!AppInitBasicSetup(gArgs)) return false;`
`96`		`- if (!AppInitParameterInteraction(gArgs, /use_syscall_sandbox=/false)) return false;`
	`95`	`+ if (!AppInitBasicSetup(args())) return false;`
	`96`	`+ if (!AppInitParameterInteraction(args(), /use_syscall_sandbox=/false)) return false;`
`97`	`97`
`98`	`98`	`m_context->kernel = std::make_unique<kernel::Context>();`
`99`	`99`	`if (!AppInitSanityChecks(*m_context->kernel)) return false;`
`@@ -116,7 +116,7 @@ class NodeImpl : public Node`
`116`	`116`	`{`
`117`	`117`	`StartShutdown();`
`118`	`118`	`// Stop RPC for clean shutdown if any of waitfor* commands is executed.`
`119`		`- if (gArgs.GetBoolArg("-server", false)) {`
	`119`	`+ if (args().GetBoolArg("-server", false)) {`
`120`	`120`	`InterruptRPC();`
`121`	`121`	`StopRPC();`
`122`	`122`	`}`
`@@ -125,28 +125,28 @@ class NodeImpl : public Node`
`125`	`125`	`bool isSettingIgnored(const std::string& name) override`
`126`	`126`	`{`
`127`	`127`	`bool ignored = false;`
`128`		`- gArgs.LockSettings([&](util::Settings& settings) {`
	`128`	`+ args().LockSettings([&](util::Settings& settings) {`
`129`	`129`	`if (auto* options = util::FindKey(settings.command_line_options, name)) {`
`130`	`130`	`ignored = !options->empty();`
`131`	`131`	`}`
`132`	`132`	`});`
`133`	`133`	`return ignored;`
`134`	`134`	`}`
`135`		`- util::SettingsValue getPersistentSetting(const std::string& name) override { return gArgs.GetPersistentSetting(name); }`
	`135`	`+ util::SettingsValue getPersistentSetting(const std::string& name) override { return args().GetPersistentSetting(name); }`
`136`	`136`	`void updateRwSetting(const std::string& name, const util::SettingsValue& value) override`
`137`	`137`	`{`
`138`		`- gArgs.LockSettings([&](util::Settings& settings) {`
	`138`	`+ args().LockSettings([&](util::Settings& settings) {`
`139`	`139`	`if (value.isNull()) {`
`140`	`140`	`settings.rw_settings.erase(name);`
`141`	`141`	`} else {`
`142`	`142`	`settings.rw_settings[name] = value;`
`143`	`143`	`}`
`144`	`144`	`});`
`145`		`- gArgs.WriteSettingsFile();`
	`145`	`+ args().WriteSettingsFile();`
`146`	`146`	`}`
`147`	`147`	`void forceSetting(const std::string& name, const util::SettingsValue& value) override`
`148`	`148`	`{`
`149`		`- gArgs.LockSettings([&](util::Settings& settings) {`
	`149`	`+ args().LockSettings([&](util::Settings& settings) {`
`150`	`150`	`if (value.isNull()) {`
`151`	`151`	`settings.forced_settings.erase(name);`
`152`	`152`	`} else {`
`@@ -156,11 +156,11 @@ class NodeImpl : public Node`
`156`	`156`	`}`
`157`	`157`	`void resetSettings() override`
`158`	`158`	`{`
`159`		`- gArgs.WriteSettingsFile(/errors=/nullptr, /backup=/true);`
`160`		`- gArgs.LockSettings([&](util::Settings& settings) {`
	`159`	`+ args().WriteSettingsFile(/errors=/nullptr, /backup=/true);`
	`160`	`+ args().LockSettings([&](util::Settings& settings) {`
`161`	`161`	`settings.rw_settings.clear();`
`162`	`162`	`});`
`163`		`- gArgs.WriteSettingsFile();`
	`163`	`+ args().WriteSettingsFile();`
`164`	`164`	`}`
`165`	`165`	`void mapPort(bool use_upnp, bool use_natpmp) override { StartMapPort(use_upnp, use_natpmp); }`
`166`	`166`	`bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }`
`@@ -237,7 +237,7 @@ class NodeImpl : public Node`
`237`	`237`	`{`
`238`	`238`	`#ifdef ENABLE_EXTERNAL_SIGNER`
`239`	`239`	`std::vector<ExternalSigner> signers = {};`
`240`		`- const std::string command = gArgs.GetArg("-signer", "");`
	`240`	`+ const std::string command = args().GetArg("-signer", "");`
`241`	`241`	`if (command == "") return {};`
`242`	`242`	`ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());`
`243`	`243`	`std::vector<std::unique_ptr<interfaces::ExternalSigner>> result;`
`@@ -388,6 +388,7 @@ class NodeImpl : public Node`
`388`	`388`	`{`
`389`	`389`	`m_context = context;`
`390`	`390`	`}`
	`391`	`+ ArgsManager& args() { return *Assert(Assert(m_context)->args); }`
`391`	`392`	`ChainstateManager& chainman() { return *Assert(m_context->chainman); }`
`392`	`393`	`NodeContext* m_context{nullptr};`
`393`	`394`	`};`
`@@ -744,16 +745,16 @@ class ChainImpl : public Chain`
`744`	`745`	`int rpcSerializationFlags() override { return RPCSerializationFlags(); }`
`745`	`746`	`util::SettingsValue getSetting(const std::string& name) override`
`746`	`747`	`{`
`747`		`- return gArgs.GetSetting(name);`
	`748`	`+ return args().GetSetting(name);`
`748`	`749`	`}`
`749`	`750`	`std::vector<util::SettingsValue> getSettingsList(const std::string& name) override`
`750`	`751`	`{`
`751`		`- return gArgs.GetSettingsList(name);`
	`752`	`+ return args().GetSettingsList(name);`
`752`	`753`	`}`
`753`	`754`	`util::SettingsValue getRwSetting(const std::string& name) override`
`754`	`755`	`{`
`755`	`756`	`util::SettingsValue result;`
`756`		`- gArgs.LockSettings([&](const util::Settings& settings) {`
	`757`	`+ args().LockSettings([&](const util::Settings& settings) {`
`757`	`758`	`if (const util::SettingsValue* value = util::FindKey(settings.rw_settings, name)) {`
`758`	`759`	`result = *value;`
`759`	`760`	`}`
`@@ -762,14 +763,14 @@ class ChainImpl : public Chain`
`762`	`763`	`}`
`763`	`764`	`bool updateRwSetting(const std::string& name, const util::SettingsValue& value, bool write) override`
`764`	`765`	`{`
`765`		`- gArgs.LockSettings([&](util::Settings& settings) {`
	`766`	`+ args().LockSettings([&](util::Settings& settings) {`
`766`	`767`	`if (value.isNull()) {`
`767`	`768`	`settings.rw_settings.erase(name);`
`768`	`769`	`} else {`
`769`	`770`	`settings.rw_settings[name] = value;`
`770`	`771`	`}`
`771`	`772`	`});`
`772`		`- return !write \|\| gArgs.WriteSettingsFile();`
	`773`	`+ return !write \|\| args().WriteSettingsFile();`
`773`	`774`	`}`
`774`	`775`	`void requestMempoolTransactions(Notifications& notifications) override`
`775`	`776`	`{`
`@@ -785,6 +786,7 @@ class ChainImpl : public Chain`
`785`	`786`	`}`
`786`	`787`
`787`	`788`	`NodeContext* context() override { return &m_node; }`
	`789`	`+ ArgsManager& args() { return *Assert(m_node.args); }`
`788`	`790`	`ChainstateManager& chainman() { return *Assert(m_node.chainman); }`
`789`	`791`	`NodeContext& m_node;`
`790`	`792`	`};`