Remove gArgs global from init #19779

maflcko · 2020-08-22T08:41:39Z

The gArgs global has several issues:

gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value.
Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. Net processing: move ProcessMessage() to PeerLogicValidation #19704 (comment) or Pass ArgsManager into functions that register args with AddArg #19511

The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead.

practicalswift · 2020-08-22T09:04:16Z

Concept ACK: decoupled is better than coupled!

DrahtBot · 2020-08-22T11:48:40Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Add dumpcoinstats #19792 (rpc: Add dumpcoinstats by fjahr)
Coinstats Index #19521 (Coinstats Index (without UTXO set hash) by fjahr)
util: Make default arg values more specific #19471 (util: Make default arg values more specific by hebasto)
refactor: Move wallet methods out of chain.h and node.h #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
refactor: Cleanup thread ctor calls #19064 (refactor: Cleanup thread ctor calls by hebasto)
net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)
util: Set safe permissions for data directory and wallets/ subdir #17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
feature: Added ability for users to add a startup command #15367 (feature: Added ability for users to add a startup command by benthecarman)
Net: Do not re-enable Onion network when it was disabled via onlynet #14425 (Net: Do not re-enable Onion network when it was disabled via onlynet by wodry)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag

Concept ACK.

In fa88257 instead of binding args, could do this

@@ -1828,19 +1828,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
     }

 #if HAVE_SYSTEM
-    if (args.IsArgSet("-blocknotify")) {
-        const auto BlockNotifyCallback = [&args](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) {
-            if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex)
-                return;
-
-            std::string strCmd = args.GetArg("-blocknotify", "");
-            if (!strCmd.empty()) {
-                boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
-                std::thread t(runCommand, strCmd);
-                t.detach(); // thread runs free
-            }
-        };
-        uiInterface.NotifyBlockTip_connect(BlockNotifyCallback);
+    std::string blocknotify = args.GetArg("-blocknotify", "");
+    if (!blocknotify.empty()) {
+        uiInterface.NotifyBlockTip_connect([blocknotify](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) {
+            if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex) return;
+            auto strCmd = blocknotify;
+            boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
+            std::thread t(runCommand, strCmd);
+            t.detach(); // thread runs free
+        });
     }
 #endif

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

-BEGIN VERIFY SCRIPT- # Replace gArgs with args sed -i 's/\<gArgs\>/args/g' src/init.cpp src/bitcoind.cpp sed -i 's/&args;/\&gArgs;/g' src/init.cpp # Format changed lines git diff -U0 | clang-format-diff -p1 -i -v -END VERIFY SCRIPT-

jonasschnelli · 2020-08-24T07:40:43Z

Concept ACK fa9d590

maflcko · 2020-08-24T07:53:05Z

Thanks @promag, applied the capture-copy-lambda fixup

src/init.cpp

ryanofsky

Code review ACK fa9d590. Looks good. Nice day to remove some globals, and add some lambdas 👍

src/bitcoind.cpp

fanquake

ACK fa9d590 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff.

fa9d590 scripted-diff: gArgs -> args (MarcoFalke) fa33bc2 init: Capture copy of blocknotify setting for BlockNotifyCallback (MarcoFalke) fa40017 init: Pass reference to ArgsManager around instead of relying on global (MarcoFalke) Pull request description: The gArgs global has several issues: * gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value. * Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. bitcoin#19704 (comment) or bitcoin#19511 The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead. ACKs for top commit: ryanofsky: Code review ACK fa9d590. Looks good. Nice day to remove some globals, and add some lambdas 👍 fanquake: ACK fa9d590 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff. jonasschnelli: Concept ACK fa9d590 Tree-SHA512: ed00db5f826566c7e3b4d0b3d2ee0fc1a49a6e748e04e5c93bdd694ac7da5598749e73937047d5fce86150d764a067d2ca344ba4ae3eb2704cc5c4fa0d20940f

Summary: ``` The gArgs global has several issues: gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value. Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. #19704 (comment) or #19511 The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead. ``` Backport of core [[bitcoin/bitcoin#19779 | PR19779]]. Depends on D8644 and D8645. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8646

maflcko added the Refactoring label Aug 22, 2020

promag reviewed Aug 24, 2020

View reviewed changes

init: Pass reference to ArgsManager around instead of relying on global

fa40017

maflcko force-pushed the 2008-gArgs branch from fa135c8 to fa396c7 Compare August 24, 2020 05:47

MarcoFalke added 2 commits August 24, 2020 07:51

init: Capture copy of blocknotify setting for BlockNotifyCallback

fa33bc2

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

scripted-diff: gArgs -> args

fa9d590

-BEGIN VERIFY SCRIPT- # Replace gArgs with args sed -i 's/\<gArgs\>/args/g' src/init.cpp src/bitcoind.cpp sed -i 's/&args;/\&gArgs;/g' src/init.cpp # Format changed lines git diff -U0 | clang-format-diff -p1 -i -v -END VERIFY SCRIPT-

maflcko force-pushed the 2008-gArgs branch from fa396c7 to fa9d590 Compare August 24, 2020 05:53

promag reviewed Aug 24, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

ryanofsky approved these changes Aug 24, 2020

View reviewed changes

src/bitcoind.cpp Show resolved Hide resolved

src/bitcoind.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 24, 2020

rpc: Add dumpcoinstats #19792

Closed

bitcoin deleted a comment from StanislavKlimov95 Aug 25, 2020

fanquake approved these changes Aug 26, 2020

View reviewed changes

fanquake merged commit 6a2ba62 into bitcoin:master Aug 26, 2020

maflcko deleted the 2008-gArgs branch August 26, 2020 07:27

DrahtBot mentioned this pull request Aug 26, 2020

Multiprocess bitcoin #10102

Draft

maflcko mentioned this pull request Aug 26, 2020

Pass argsmanager reference to intro instead of relying on global bitcoin-core/gui#75

Closed

kwvg mentioned this pull request Oct 7, 2021

merge bitcoin#15931, #16839, #17192, #17407, #18037, #17997, partial #15639, #17989: deglobalisation and mining rpc backports dashpay/dash#4357

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove gArgs global from init #19779

Remove gArgs global from init #19779

Uh oh!

maflcko commented Aug 22, 2020

Uh oh!

practicalswift commented Aug 22, 2020

Uh oh!

DrahtBot commented Aug 22, 2020 •

edited

Loading

Uh oh!

promag left a comment

Uh oh!

jonasschnelli commented Aug 24, 2020

Uh oh!

maflcko commented Aug 24, 2020

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

Remove gArgs global from init #19779

Remove gArgs global from init #19779

Uh oh!

Conversation

maflcko commented Aug 22, 2020

Uh oh!

practicalswift commented Aug 22, 2020

Uh oh!

DrahtBot commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 24, 2020

Uh oh!

maflcko commented Aug 24, 2020

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 22, 2020 •

edited

Loading