Parse params directly instead of through node (partial revert #10244)

ryanofsky commented at 1:40 pm on July 16, 2020: contributor

This PR is part of the process separation project.

This is a partial revert of https://github.com/bitcoin/bitcoin/pull/10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

These changes were originally pushed as part of https://github.com/bitcoin/bitcoin/pull/19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ryanofsky renamed this:
~~gui: Partially revert #10244 gArgs and Params changes~~
Partially revert bitcoin/bitcoin#10244 gArgs and Params changes
on Jul 16, 2020

ryanofsky renamed this:
~~Partially revert bitcoin/bitcoin#10244 gArgs and Params changes~~
Partially revert #10244 gArgs and Params changes
on Jul 16, 2020

Sjors commented at 6:33 am on July 17, 2020: member

Concept ACK. The reasoning as explained in the commit message makes sense to me. Maybe a better PR title would be “Parse params directly instead of through node (partial revert #10244)”. And then in the PR description: “In addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node proces. So for that reason it should be able to parse its own parameters, rather than rely on the node.”

in src/qt/test/test_main.cpp:74 in 5adf00eb1e outdated

69@@ -70,7 +70,7 @@ int main(int argc, char* argv[])
70     BitcoinApplication app(*node);
71     app.setApplicationName("Bitcoin-Qt-test");
72 
73-    node->setupServerArgs();            // Make gArgs available in the NodeContext
74+    node->context()->args = &gArgs;     // Make gArgs available in the NodeContext
75     node->context()->args->ClearArgs(); // Clear added args again

MarcoFalke commented at 7:08 am on July 17, 2020:

This line won’t be needed anymore

ryanofsky commented at 5:06 pm on July 17, 2020:

re: #35 (review)

This line won’t be needed anymore

Thanks, removed

in src/qt/optionsmodel.cpp:250 in 5adf00eb1e outdated

243@@ -244,10 +244,10 @@ void OptionsModel::SetPruneEnabled(bool prune, bool force)
244     const int64_t prune_target_mib = PruneGBtoMiB(settings.value("nPruneSize").toInt());
245     std::string prune_val = prune ? ToString(prune_target_mib) : "0";
246     if (force) {
247-        m_node.forceSetArg("-prune", prune_val);
248+        gArgs.ForceSetArg("-prune", prune_val);
249         return;
250     }
251-    if (!m_node.softSetArg("-prune", prune_val)) {
252+    if (!gArgs.SoftSetArg("-prune", prune_val)) {

MarcoFalke commented at 7:14 am on July 17, 2020:

Can’t this use this->node().context()->args instead of the global, which should be gone hopefully :soon: ?

ryanofsky commented at 5:06 pm on July 17, 2020:

re: #35 (review)

Can’t this use this->node().context()->args instead of the global, which should be gone hopefully 🔜 ?

After https://github.com/bitcoin/bitcoin/pull/10102, there is no NodeContext in the gui process. An ArgsManager reference could be passed to OptionsModel if desired, though.

hebasto commented at 1:39 pm on July 17, 2020: member

I’ll try to describe the desired behavior as I understand it.

When a bitcoin-gui process starts there are two possibilities: (a) to spawn a new bitcoin-node process and connect to it, or (b) to connect to an existing bitcoin-node process.

In variant (a) the bitcoin-gui process parses arguments (from the command line and from a config file) and passes them to the bitcoin-node process, which in turn does not even look into the config file.

In variant (b) the existing bitcoin-node process already has parsed its arguments (from the command line and from a config file), and the bitcoin-gui process parses its own command-line arguments and (the same?) config file.

Did I understand the concept correct?

How the possible conflicts between different process parsed arguments should be resolved?

ryanofsky force-pushed on Jul 17, 2020

ryanofsky renamed this:
~~Partially revert #10244 gArgs and Params changes~~
Parse params directly instead of through node (partial revert #10244)
on Jul 17, 2020

ryanofsky commented at 5:28 pm on July 17, 2020: contributor

Updated 5adf00eb1eb11bc9f5f7b3b5e08b0341ddfbd01d -> 49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c (pr/ipc-guiarg.1 -> pr/ipc-guiarg.2, compare) with test simplification suggested by Marco, and 3 more commits pulled from https://github.com/bitcoin/bitcoin/pull/19461

re: Sjors #35 (comment)

Maybe a better PR title […]

Thanks! Used your title and description.

re: hebasto #35 (comment)

I’ll try to describe the desired behavior as I understand it. […]

Yes that’s a good description of https://github.com/bitcoin/bitcoin/pull/19461.

How the possible conflicts between different process parsed arguments should be resolved?

This PR doesn’t actually change how arguments are interpreted, it just allows them to be sent differently in https://github.com/bitcoin/bitcoin/pull/19461. Like you described above, in that PR if the gui process spawns a new node process, it forwards the arguments and there are no conflicts. But as mentioned in the PR description for https://github.com/bitcoin/bitcoin/pull/19461 “there are rough edges” in that PR and “if node command line options are passed to bitcoin-gui and bitcoin-gui connects to an exiting bitcoin-node process instead of spawning a new one, the node options will be silently ignored.” More ideally, passing mismatched node options to bitcoin-gui would trigger errors or warnings. Adding this error checking could be a new commit in https://github.com/bitcoin/bitcoin/pull/19461 or a followup PR.

hebasto commented at 5:32 pm on July 17, 2020: member

Concept ACK.

hebasto commented at 10:20 am on July 18, 2020: member

Reviewed commits:

- 551c50873da0b0581a68c0b594ff0d87e6292947 “gui: Partially revert #10244 gArgs and Params changes”
- 9139f5b347d005f13269ce9d0da4951199bcc1a5 “gui: Remove unused interfaces::Node references”
- 0cc3492a7c401ec71a2cb2a964495ab00d74bd97 “gui: Replace interface::Node references with pointers”
- 49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c “gui: Delay interfaces::Node initialization”

hebasto commented at 7:37 am on July 19, 2020: member

Not passing an interfaces::Node argument to the BitcoinApplication and OptionsModel constructors implies that the internal states of the constructed objects with m_node initialized to nullptr are consistent and legitimate. Therefore, their node() getters shouldn’t include assert(m_node). I think the responsibility to check the pointer should be moved to callers.

hebasto approved

hebasto commented at 10:59 am on July 19, 2020: member

ACK 49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c, tested on Linux Mint 20 (x86_64, Qt 5.12.8).

49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c I think the m_shutdown member should be dropped due to the following reasons:

currently, the SplashScreen::shutdown() could be called only after entering to the main event loop, i.e., after the first app.exec() call; therefore, SplashScreen::setNode() will always see the initial value of the m_shutdown
if in the future more complicated behavior will be introduced, SplashScreen::setNode() and SplashScreen::shutdown() are potentially data-racy, and the m_shutdown member could require be protected by a mutex

nano-nit: why not apply clang-format-diff.py for each commit?

ryanofsky commented at 5:04 pm on July 21, 2020: contributor

Thanks for reviewing! I looked into many things but wound up not making changes for reasons described below.

re: #35 (comment)

Not passing an interfaces::Node argument to the BitcoinApplication and OptionsModel constructors implies that the internal states of the constructed objects with m_node initialized to nullptr are consistent and legitimate. Therefore, their node() getters shouldn’t include assert(m_node).

I don’t think this follows. It’s ok for functions to have preconditions, and this is a pretty straightforward precondition to have for an accessor returning a reference.

I think the responsibility to check the pointer should be moved to callers.

We might just have a style disagreement here, but if there are specific changes you are looking for, maybe you could post them or describe them in more detail. I don’t think we need to have pointers and null checks in places where the pointers can never be null. There’s only one place in the PR that actually needs to check for a null node: the splash screen code handling the Q keypress needs to check if the node is connected before sending it a shutdown request. Everywhere else, references and not pointers are more appropriate and have been retained.

re: #35#pullrequestreview-451120856

I think the m_shutdown member should be dropped due to the following reasons:

m_shutdown is needed so the splash screen can be shown instantly without waiting for anything and the Q key can function requesting a shutdown as soon as possible. Maybe this change could be moved to https://github.com/bitcoin/bitcoin/pull/19461, but you could also say the same about all other changes in this PR. I hope the cleanups in this PR make sense by themselves, but the actual motivation for this PR is to enable https://github.com/bitcoin/bitcoin/pull/19461.

the m_shutdown member could require be protected by a mutex

A mutex would only be required if Qt signals are used incorrectly. GUI state should be accessed by one thread.

nano-nit: why not apply clang-format-diff.py for each commit?

I read diffs to understand changes and having unrelated formatting changes mixed into commits makes this harder.

hebasto commented at 5:30 pm on July 21, 2020: member

I think the m_shutdown member should be dropped due to the following reasons:

m_shutdown is needed so the splash screen can be shown instantly without waiting for anything and the Q key can function requesting a shutdown as soon as possible.

I understand this motivation. But the splash screen is not derived from QDialog with its own event loop, it is derived from QWidget and it will wait for the main event loop which starts in https://github.com/bitcoin-core/gui/blob/49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c/src/qt/bitcoin.cpp#L604

ryanofsky commented at 5:57 pm on July 21, 2020: contributor

I think the m_shutdown member should be dropped due to the following reasons:

m_shutdown is needed so the splash screen can be shown instantly without waiting for anything and the Q key can function requesting a shutdown as soon as possible.

I understand this motivation. But the splash screen is not derived from QDialog with its own event loop, it is derived from QWidget and it will wait for the main event loop which starts in

https://github.com/bitcoin-core/gui/blob/49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c/src/qt/bitcoin.cpp#L604

What is this comment referring to and what is it asking for? I think we both agree handling a null pointer in the splashscreen is not strictly needed here. It’s needed to handle the delayed gui->node connection implemented in https://github.com/bitcoin/bitcoin/pull/19461. The point of all the changes in this PR is to reduce size and complexity of https://github.com/bitcoin/bitcoin/pull/19461. I think it makes sense to handle null node pointer in the splash screen here in this PR which is first introducing the pointer variable, even though the variable won’t have a null value until https://github.com/bitcoin/bitcoin/pull/19461.

promag commented at 0:26 am on July 27, 2020: contributor

Concept ACK.

luke-jr commented at 9:36 pm on July 31, 2020: member

Concept ACK

hebasto commented at 11:04 am on August 1, 2020: member

@ryanofsky Mind rebasing? (no DrahtBot’s notifications in this repo for now)

DrahtBot added the label Needs rebase on Aug 3, 2020

ryanofsky force-pushed on Aug 3, 2020

ryanofsky commented at 9:09 pm on August 3, 2020: contributor

Rebased 49b993ddb2cea18b7ba8691ea9b572c9e9bbcb9c -> 1a0875f151be1827a2f880cc63b1022455665c0d (pr/ipc-guiarg.2 -> pr/ipc-guiarg.3, compare) due to conflicts with https://github.com/bitcoin/bitcoin/pull/19561 and https://github.com/bitcoin/bitcoin/pull/15935

DrahtBot removed the label Needs rebase on Aug 3, 2020

hebasto approved

hebasto commented at 2:02 am on August 6, 2020: member

re-ACK 1a0875f151be1827a2f880cc63b1022455665c0d, only rebased since my previous review (verified with git range-diff).

DrahtBot added the label Needs rebase on Aug 7, 2020

ryanofsky force-pushed on Aug 7, 2020

ryanofsky commented at 8:00 pm on August 7, 2020: contributor

Rebased 1a0875f151be1827a2f880cc63b1022455665c0d -> 37b7eba5c4d3a77aa3fbd40fa7f066dad6d393c1 (pr/ipc-guiarg.3 -> pr/ipc-guiarg.4, compare) due to conflict with https://github.com/bitcoin/bitcoin/pull/19098

DrahtBot removed the label Needs rebase on Aug 7, 2020

hebasto approved

hebasto commented at 7:29 am on August 8, 2020: member

re-ACK 37b7eba5c4d3a77aa3fbd40fa7f066dad6d393c1, only rebased since my previous review (verified with git range-diff).

DrahtBot added the label Needs rebase on Aug 14, 2020

ryanofsky force-pushed on Aug 17, 2020

ryanofsky commented at 1:43 am on August 17, 2020: contributor

Rebased 37b7eba5c4d3a77aa3fbd40fa7f066dad6d393c1 -> 333507a1f3e1941367756dc7974c7c9720d03b4a (pr/ipc-guiarg.4 -> pr/ipc-guiarg.5, compare) due to conflict with https://github.com/bitcoin/bitcoin/pull/19011

DrahtBot removed the label Needs rebase on Aug 17, 2020

in src/interfaces/node.h:59 in 333507a1f3 outdated

80-    //! needs to be called after selectParams() because the settings file
81-    //! location is network-specific.
82-    virtual bool initSettings(std::string& error) = 0;
83-
84     //! Get the (assumed) blockchain size.
85     virtual uint64_t getAssumedBlockchainSize() = 0;

MarcoFalke commented at 10:24 am on August 17, 2020:

in commit 427c8da711

why is this not removed?

ryanofsky commented at 11:00 am on August 17, 2020:

re: #35 (review)

in commit 427c8da

why is this not removed?

Good catch, removed now

MarcoFalke commented at 10:46 am on August 17, 2020: contributor

crACK 333507a1f3 🦌

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3crACK 333507a1f3 🦌
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgqNQv7BdH9+5Rmi/cztRlIzz//JI94OOHZY3r2lQ6GovtiSTstsj4eeGarqfNL
 8v5JOwpXU5NXyJ62BevybdeOwukJTsycj+N8ozCs8HP8ts6H4ji+DA0jpEQA8HBH3
 9vuLwZI2cV4UrYSebBAJsQZxEpsHqX+eFEhgxUUoZypSgGNDt6A4SuXOKBeT5uU+v
10aWjPs5sfsuCiFrMC7opq9/9rIzGNEAnf4zla748NLhbKof+W3HdH5+i1dPSf4tNl
11pKdlnt0AoJfuRTlior37qPftEWazXypvupxRnMZ92YRRD+qVrgiTn6c2K6+SvH0G
12IhY1//3dYqzPczpR8aAv9igOL1iN2svjrdxm8fPFIU5/feKi92al5RqeXFBUtGLs
13ZzJ3XY3paOnMm0A1Zy790TXyOc07Trxu/c3Xe90vR6DKqUoqvKj/443uMLxlMZdj
14OCEbjVJJ5jXCadwdQtQmA9uceNj22Hep9AfSdvExflPRq8gq7Q5PzdB5SaNh7VBE
15rb2JCP5V
16=OHa6
17-----END PGP SIGNATURE-----

Timestamp of file with hash 726f1597cbef5b840b8b43b9a0c4aada8e06894a7a3803c798f9d71e3ef4d656 -

in src/qt/bitcoin.cpp:463 in 427c8da711 outdated

459@@ -457,11 +460,11 @@ int GuiMain(int argc, char* argv[])
460 
461     /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
462     // Command-line options take precedence:
463-    node->setupServerArgs();
464+    SetupServerArgs(*node->context());

MarcoFalke commented at 10:49 am on August 17, 2020:

commit 427c8da7113527cf3809cb568cd3fe529e56b259: The documentation for context() says “Useful for testing”. Did you mean to write node_context instead?

0    SetupServerArgs(node_context);

ryanofsky commented at 11:07 am on August 17, 2020:

re: https://github.com/bitcoin-core/gui/pull/35/files#r471397426

commit 427c8da: The documentation for context() says “Useful for testing”. Did you mean to write node_context instead?

Changed, agree this is a little better. SetupServerArgs should really not be using NodeContext at all (argument is removed in https://github.com/bitcoin/bitcoin/pull/10102), but for this PR which is not in the GUI repository, I wanted to avoid changing non-gui code.

ryanofsky force-pushed on Aug 17, 2020

ryanofsky commented at 11:28 am on August 17, 2020: contributor

Thanks for review!

Updated 333507a1f3e1941367756dc7974c7c9720d03b4a -> 3ae9b4a482d53a050a67392f9a5bacf386fcc33d (pr/ipc-guiarg.5 -> pr/ipc-guiarg.6, compare) with suggestions

MarcoFalke closed this on Aug 19, 2020

MarcoFalke commented at 9:10 am on August 19, 2020: contributor

re-run ci

MarcoFalke reopened this on Aug 19, 2020

gui: Partially revert #10244 gArgs and Params changes

Change gui code to use gArgs, Params() functions directly instead of going
through interfaces::Node.

Remotely accessing bitcoin-node ArgsManager from bitcoin-gui works fine in
https://github.com/bitcoin/bitcoin/pull/10102, when bitcoin-gui spawns a new
bitcoin-node process and controls its startup, but for bitcoin-gui to support
-ipcconnect option in https://github.com/bitcoin/bitcoin/pull/19461 and connect
to an existing bitcoin-node process, it needs ability to parse arguments itself
before connecting out.

This change also simplifies https://github.com/bitcoin/bitcoin/pull/10102 a
bit, by making the bitcoin-gui -> bitcoin-node startup sequence more similar to
the bitcoin-node -> bitcoin-wallet startup sequence where the parent process
parses arguments and passes them to the child process instead of the parent
process using the child process to parse arguments.

e133631625

gui: Remove unused interfaces::Node references

Remove Node references no longer needed after previous commit

91aced7c7e

gui: Replace interface::Node references with pointers

No change in behavior. Replacing references with pointers allows Node interface
creation to be delayed until later during gui startup next commit to support
implementing -ipcconnect option

102abff9eb

gui: Delay interfaces::Node initialization

This is needed to allow bitcoin-gui to connect to existing node process with
-ipcconnect instead of spawning a new process. It's possible to spawn a new
bitcoin-node process without knowing the current data dir or network, but
connecting to an existing bitcoin-node requires knowing the datadir and network
first.

519cae8fd6

in src/qt/bitcoin.cpp:471 in 3ae9b4a482 outdated

468+    BitcoinApplication app;
469 
470     /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
471     // Command-line options take precedence:
472-    node->setupServerArgs();
473+    SetupServerArgs(node_context);

MarcoFalke commented at 10:40 am on August 26, 2020:

nit: Could add an unused reference to the argsmanager that is supposed to be used by gui code (instead of the global).

Would either be const ArgsManager& args = *node_context.args or = gArgs?

Feel free to ingore.

ryanofsky commented at 12:36 pm on August 26, 2020:

re: #35 (review)

nit: Could add an unused reference to the argsmanager that is supposed to be used by gui code (instead of the global).

I started implementing this but stopped because there are a number of other uses of gArgs in this function that I thought it would be better not to change here to avoid increasing the size of the PR.

Bigger picture, gArgs is used pretty widely in many parts of qt code and I think that if it’s going to be removed, it’d be best to do it in a focused PR updating all the references at the same time.

in src/qt/bitcoin.cpp:474 in 3ae9b4a482 outdated

473+    SetupServerArgs(node_context);
474     SetupUIArgs(gArgs);
475     std::string error;
476-    if (!node->parseParameters(argc, argv, error)) {
477-        node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
478+    if (!gArgs.ParseParameters(argc, argv, error)) {

MarcoFalke commented at 10:41 am on August 26, 2020:

nit: (if feedback is addressed)

Could use

0    if (!args.ParseParameters(argc, argv, error)) {

here?

ryanofsky commented at 1:12 pm on August 26, 2020:

re: #35 (review)

Could use

(same feedback)

MarcoFalke commented at 10:41 am on August 26, 2020: contributor

will re-review after rebase

DrahtBot added the label Needs rebase on Aug 26, 2020

DrahtBot commented at 11:04 am on August 26, 2020: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

ryanofsky force-pushed on Aug 26, 2020

ryanofsky commented at 1:21 pm on August 26, 2020: contributor

Thanks for review

Rebased 3ae9b4a482d53a050a67392f9a5bacf386fcc33d -> 519cae8fd6e44aef3470415d7c5e12acb0acd9f4 (pr/ipc-guiarg.6 -> pr/ipc-guiarg.7, compare) due to conflict with https://github.com/bitcoin/bitcoin/pull/19779

MarcoFalke commented at 2:27 pm on August 26, 2020: contributor

re-ACK 519cae8fd6, only change is rebase and addressed nits of my previous review last week 🌄

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 519cae8fd6, only change is rebase and addressed nits of my previous review last week 🌄
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUj6Ggv+LwP3f/BQc39c/CE6YAwsMHH0yuGCrTpPSZ2Mg4LFVAMNs5Frf7X/WTK3
 8knd0Wqn8uXL1ohllgYw/RpANRre871edpLXP9DANpAF1mkFM8oQIwLesi2B26TWK
 9eQWmTbXHeXzmx3TDeyRlNbnd7YQjYWv46qaLaqgtstSzTM5KQG24D2qLL85YgdvK
10iGzfDQqNsfVlt8KQa1edWtMi/tbnSKIERKdC1oGjBdWbg/hliwabsDpjaFtgACSS
11CwWyz4yVrKcN3fd9KRkkA+InJIdCGxNx2Tkqvof1f7d3P9RZ3OSlVhz6b7/Z2V6a
12kucVgTo/bFOdza6AQ+7K6yT3fuYOyhRvwUY2U9CD1heLUFVdaDouCVwqj1AsSTU7
13/eqMwOmldW/hrxxVM+Pnm767VryXYMSkOzjBUlEKjrBgSaQSR9PPe4gZ2spAzDYX
14R4CIzdXuxLnmYqgY6ynhaQI//XlBg+Kj68oNYcT/ccKuEkMreYjmnta3T7vtiB3b
15SONP0kHM
16=zDRb
17-----END PGP SIGNATURE-----

Timestamp of file with hash ecc68c0f8f95794d4615f84e2642dbc3ee12d9db23d521fe606e763336bf3551 -

MarcoFalke merged this on Aug 26, 2020

MarcoFalke closed this on Aug 26, 2020

apoelstra referenced this in commit 5da20250a1 on Dec 3, 2020

stevenroose referenced this in commit 89db65048d on Mar 19, 2021

bitcoin-core locked this on Feb 15, 2022

Parse params directly instead of through node (partial revert #10244) #35