Remove gArgs global from init

MarcoFalke commented at 8:41 am on August 22, 2020: member

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.

MarcoFalke added the label Refactoring on Aug 22, 2020

practicalswift commented at 9:04 am on August 22, 2020: contributor

Concept ACK: decoupled is better than coupled!

DrahtBot commented at 11:48 am on August 22, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19792 (rpc: Add dumpcoinstats by fjahr)
#19521 (Coinstats Index (without UTXO set hash) by fjahr)
#19471 (util: Make default arg values more specific by hebasto)
#19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
#19064 (refactor: Cleanup thread ctor calls by hebasto)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
#15367 (feature: Added ability for users to add a startup command by benthecarman)
#14425 (Net: Do not re-enable Onion network when it was disabled via onlynet by wodry)
#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 commented at 0:11 am on August 24, 2020: member

Concept ACK.

In fa88257dc39b049a9415a9ccd1c1668f5b75e34f instead of binding args, could do this

 0@@ -1828,19 +1828,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
 1     }
 2
 3 #if HAVE_SYSTEM
 4-    if (args.IsArgSet("-blocknotify")) {
 5-        const auto BlockNotifyCallback = [&args](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) {
 6-            if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex)
 7-                return;
 8-
 9-            std::string strCmd = args.GetArg("-blocknotify", "");
10-            if (!strCmd.empty()) {
11-                boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
12-                std::thread t(runCommand, strCmd);
13-                t.detach(); // thread runs free
14-            }
15-        };
16-        uiInterface.NotifyBlockTip_connect(BlockNotifyCallback);
17+    std::string blocknotify = args.GetArg("-blocknotify", "");
18+    if (!blocknotify.empty()) {
19+        uiInterface.NotifyBlockTip_connect([blocknotify](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) {
20+            if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex) return;
21+            auto strCmd = blocknotify;
22+            boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex());
23+            std::thread t(runCommand, strCmd);
24+            t.detach(); // thread runs free
25+        });
26     }
27 #endif

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

MarcoFalke force-pushed on Aug 24, 2020

init: Capture copy of blocknotify setting for BlockNotifyCallback

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

fa33bc2dab

scripted-diff: gArgs -> args

-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-

fa9d5902f7

MarcoFalke force-pushed on Aug 24, 2020

jonasschnelli commented at 7:40 am on August 24, 2020: contributor

Concept ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10

MarcoFalke commented at 7:53 am on August 24, 2020: member

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

in src/init.cpp:1842 in fa33bc2dab outdated

1838+        const auto BlockNotifyCallback = [block_notify](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) {
1839+            if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex)
1840+                return;
1841+
1842+            std::string strCmd = block_notify;
1843+            if (!strCmd.empty()) {

promag commented at 10:48 am on August 24, 2020:

fa33bc2dabbbd2d73961f9b0ce51420a3b6e4ad5

Why not check this before connecting BlockNotifyCallback?

MarcoFalke commented at 11:11 am on August 24, 2020:

To keep the code move-only, as this commit is to be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

promag commented at 11:20 am on August 24, 2020:

Fair enough. I just think it would be fine to ignore that and make the change, even if in a separate commit. Otherwise I’d be happy to PR it if that’s relevant.

MarcoFalke commented at 5:26 pm on August 24, 2020:

Feel free to ping me on your PR, once you have one

in src/bitcoind.cpp:55 in fa40017706 outdated

49@@ -50,11 +50,9 @@ static bool AppInit(int argc, char* argv[])
50 
51     util::ThreadSetInternalName("init");
52 
53-    //
54-    // Parameters
55-    //
56     // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main()
57     SetupServerArgs(node);
58+    ArgsManager& args = *Assert(node.args);

ryanofsky commented at 3:48 pm on August 24, 2020:

In commit “init: Pass reference to ArgsManager around instead of relying on global” (fa40017706e08b4de111e8e57aabeced60881a57)

No need to change here, but it’d be nice for a few of my pr’s if SetupServerArgs took an ArgsManager parameter instead of a NodeContext parameter. (#10102 does this, but it’s nice to shrink that PR.) It’d make this code a little more straightforward too, replacing the assertion with an assignment.

MarcoFalke commented at 5:25 pm on August 24, 2020:

I’ll leave that for later because I don’t understand how passing a node context with an args pointer is different to passing an args reference. The cross-process setupServerArgs (lowercase s) doesn’t pass anything, so this seems unrelated to multiprocess, no?

ryanofsky commented at 6:23 pm on August 24, 2020:

re: #19779 (review)

I’ll leave that for later because I don’t understand how passing a node context with an args pointer is different to passing an args reference. The cross-process setupServerArgs (lowercase s) doesn’t pass anything, so this seems unrelated to multiprocess, no?

That’s fine. To answer the question about multiprocess, #10102 changes SetupServerArgs not to take a NodeContext because the gui process also calls SetupServerArgs directly to register arguments, and be able to call gArgs.GetChainName() and gArgs.GetArg() to get values like -prune, -proxy, -datadir, etc. Also https://github.com/bitcoin-core/gui/pull/35 removes the lowercase setupServerArgs function.

We should just keep in mind trying not to overuse NodeContext, because passing NodeContext around can obscure what functions are doing and make them harder to call in places where NodeContext isn’t available. If a function like SetupServerArg only uses one member of the NodeContext struct, probably better just to pass that one member instead of the whole struct.

MarcoFalke commented at 6:43 am on August 25, 2020:

Ah, I see. I think this can be changed nicely in one go when the type of node.args is changed from raw ptr to unique_ptr in a follow-up.

ryanofsky commented at 11:29 pm on August 26, 2020:

re: #19779 (review)

it’d be nice for a few of my pr’s if SetupServerArgs took an ArgsManager parameter

FWIW, this is now done earlier in the latest version of #19160#pullrequestreview-464492390 (compare)

in src/bitcoind.cpp:54 in fa9d5902f7

53@@ -54,22 +54,19 @@ static bool AppInit(int argc, char* argv[])
54     SetupServerArgs(node);

ryanofsky commented at 3:56 pm on August 24, 2020:

In commit “scripted-diff: gArgs -> args” (fa9d5902f7d72e8cce105dd1b1f5a1062e304b10)

Is the second line of the scripted diff actually doing anything?

0sed -i 's/&args;/\&gArgs;/g' src/init.cpp

I don’t see a &gArgs

MarcoFalke commented at 5:18 pm on August 24, 2020:

It is undoing one false replacement of the first line in the script. Offending replacement would be:

0-node.args = &gArgs;
1+node.args = &args;

ryanofsky approved

ryanofsky commented at 3:58 pm on August 24, 2020: member

Code review ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10. Looks good. Nice day to remove some globals, and add some lambdas :+1:

fanquake deleted a comment on Aug 25, 2020

fanquake approved

fanquake commented at 7:18 am on August 26, 2020: member

ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10 - 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.

fanquake merged this on Aug 26, 2020

fanquake closed this on Aug 26, 2020

MarcoFalke deleted the branch on Aug 26, 2020

sidhujag referenced this in commit 60a866c284 on Aug 26, 2020

Fabcien referenced this in commit 7eba616806 on Dec 14, 2020

DrahtBot locked this on Feb 15, 2022

Remove gArgs global from init #19779

Conflicts