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

ryanofsky · 2020-07-16T13:40:56Z

This PR is part of the process separation project.

This is a partial revert of bitcoin/bitcoin#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 bitcoin/bitcoin#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.

Sjors · 2020-07-17T06:33:16Z

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

src/qt/test/test_main.cpp

maflcko · 2020-07-17T07:14:36Z

src/qt/optionsmodel.cpp

 return;
 }
- if (!m_node.softSetArg("-prune", prune_val)) {
+ if (!gArgs.SoftSetArg("-prune", prune_val)) {


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

re: #35 (comment)

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

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

hebasto · 2020-07-17T13:39:07Z

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

Updated 5adf00e -> 49b993d (pr/ipc-guiarg.1 -> pr/ipc-guiarg.2, compare) with test simplification suggested by Marco, and 3 more commits pulled from bitcoin/bitcoin#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 bitcoin/bitcoin#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 bitcoin/bitcoin#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 bitcoin/bitcoin#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 bitcoin/bitcoin#19461 or a followup PR.

ryanofsky · 2020-07-17T17:06:12Z

src/qt/optionsmodel.cpp

 return;
 }
- if (!m_node.softSetArg("-prune", prune_val)) {
+ if (!gArgs.SoftSetArg("-prune", prune_val)) {


re: #35 (comment)

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

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

src/qt/test/test_main.cpp

hebasto · 2020-07-17T17:32:34Z

Concept ACK.

hebasto

Reviewed commits:

- 551c508 "gui: Partially revert #10244 gArgs and Params changes"
- 9139f5b "gui: Remove unused interfaces::Node references"
- 0cc3492 "gui: Replace interface::Node references with pointers"
- 49b993d "gui: Delay interfaces::Node initialization"

hebasto · 2020-07-19T07:37:56Z

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

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

49b993d
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 · 2020-07-21T17:04:24Z

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 (review)

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 bitcoin/bitcoin#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 bitcoin/bitcoin#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 · 2020-07-21T17:30:58Z

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

gui/src/qt/bitcoin.cpp

Line 604 in 49b993d

app.exec();

ryanofsky · 2020-07-21T17:57:51Z

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

gui/src/qt/bitcoin.cpp

Line 604 in 49b993d

app.exec();

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 bitcoin/bitcoin#19461. The point of all the changes in this PR is to reduce size and complexity of bitcoin/bitcoin#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 bitcoin/bitcoin#19461.

promag · 2020-07-27T00:26:26Z

Concept ACK.

luke-jr · 2020-07-31T21:36:41Z

Concept ACK

hebasto · 2020-08-01T11:04:50Z

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

ryanofsky · 2020-08-03T21:09:31Z

Rebased 49b993d -> 1a0875f (pr/ipc-guiarg.2 -> pr/ipc-guiarg.3, compare) due to conflicts with bitcoin/bitcoin#19561 and bitcoin/bitcoin#15935

hebasto

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

ryanofsky · 2020-08-07T20:00:34Z

Rebased 1a0875f -> 37b7eba (pr/ipc-guiarg.3 -> pr/ipc-guiarg.4, compare) due to conflict with bitcoin/bitcoin#19098

hebasto

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

maflcko

crACK 333507a 🦌

Show signature and timestamp

Signature:

Timestamp of file with hash 726f1597cbef5b840b8b43b9a0c4aada8e06894a7a3803c798f9d71e3ef4d656 -

maflcko · 2020-08-17T10:24:41Z

src/interfaces/node.h

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


in commit 427c8da

why is this not removed?

re: #35 (comment)

in commit 427c8da

why is this not removed?

Good catch, removed now

maflcko · 2020-08-17T10:49:07Z

src/qt/bitcoin.cpp

@@ -457,11 +460,11 @@ int GuiMain(int argc, char* argv[])

 /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
 // Command-line options take precedence:
- node->setupServerArgs();
+ SetupServerArgs(*node->context());


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

Suggested change

SetupServerArgs(*node->context());

SetupServerArgs(node_context);

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 bitcoin/bitcoin#10102), but for this PR which is not in the GUI repository, I wanted to avoid changing non-gui code.

ryanofsky

Thanks for review!

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

ryanofsky · 2020-08-17T11:00:13Z

src/interfaces/node.h

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


re: #35 (comment)

in commit 427c8da

why is this not removed?

Good catch, removed now

ryanofsky · 2020-08-17T11:07:01Z

src/qt/bitcoin.cpp

@@ -457,11 +460,11 @@ int GuiMain(int argc, char* argv[])

 /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
 // Command-line options take precedence:
- node->setupServerArgs();
+ SetupServerArgs(*node->context());


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 bitcoin/bitcoin#10102), but for this PR which is not in the GUI repository, I wanted to avoid changing non-gui code.

maflcko · 2020-08-19T09:10:43Z

re-run ci

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 bitcoin/bitcoin#10102, when bitcoin-gui spawns a new bitcoin-node process and controls its startup, but for bitcoin-gui to support -ipcconnect option in bitcoin/bitcoin#19461 and connect to an existing bitcoin-node process, it needs ability to parse arguments itself before connecting out. This change also simplifies bitcoin/bitcoin#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.

Remove Node references no longer needed after previous commit

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

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.

maflcko

will re-review after rebase

maflcko · 2020-08-26T10:40:37Z

src/qt/bitcoin.cpp


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


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.

re: #35 (comment)

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.

maflcko · 2020-08-26T10:41:19Z

src/qt/bitcoin.cpp

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


nit: (if feedback is addressed)

Could use

Suggested change

if (!gArgs.ParseParameters(argc, argv, error)) {

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

here?

re: #35 (comment)

Could use

(same feedback)

DrahtBot · 2020-08-26T11:04:45Z

🐙 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

Thanks for review

Rebased 3ae9b4a -> 519cae8 (pr/ipc-guiarg.6 -> pr/ipc-guiarg.7, compare) due to conflict with bitcoin/bitcoin#19779

ryanofsky · 2020-08-26T12:36:38Z

src/qt/bitcoin.cpp


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


re: #35 (comment)

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.

ryanofsky · 2020-08-26T13:12:06Z

src/qt/bitcoin.cpp

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


re: #35 (comment)

Could use

(same feedback)

maflcko · 2020-08-26T14:27:47Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash ecc68c0f8f95794d4615f84e2642dbc3ee12d9db23d521fe606e763336bf3551 -

ryanofsky changed the title ~~gui: Partially revert #10244 gArgs and Params changes~~ Partially revert bitcoin/bitcoin#10244 gArgs and Params changes Jul 16, 2020

ryanofsky changed the title ~~Partially revert bitcoin/bitcoin#10244 gArgs and Params changes~~ Partially revert #10244 gArgs and Params changes Jul 16, 2020

maflcko reviewed Jul 17, 2020

View reviewed changes

ryanofsky force-pushed the pr/ipc-guiarg branch from 5adf00e to 49b993d Compare July 17, 2020 17:22

ryanofsky changed the title ~~Partially revert #10244 gArgs and Params changes~~ Parse params directly instead of through node (partial revert #10244) Jul 17, 2020

ryanofsky commented Jul 17, 2020

View reviewed changes

hebasto reviewed Jul 18, 2020

View reviewed changes

hebasto approved these changes Jul 19, 2020

View reviewed changes

ryanofsky mentioned this pull request Jul 21, 2020

test: Remove duplicate NodeContext hacks bitcoin/bitcoin#19098

Merged

DrahtBot added the Needs rebase label Aug 3, 2020

ryanofsky force-pushed the pr/ipc-guiarg branch from 49b993d to 1a0875f Compare August 3, 2020 21:01

DrahtBot removed the Needs rebase label Aug 3, 2020

hebasto approved these changes Aug 6, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 7, 2020

ryanofsky force-pushed the pr/ipc-guiarg branch from 1a0875f to 37b7eba Compare August 7, 2020 19:58

DrahtBot removed the Needs rebase label Aug 7, 2020

hebasto approved these changes Aug 8, 2020

View reviewed changes

ryanofsky mentioned this pull request Aug 12, 2020

util: Make default arg values more specific bitcoin/bitcoin#19471

Closed

maflcko reviewed Aug 17, 2020

View reviewed changes

ryanofsky force-pushed the pr/ipc-guiarg branch from 333507a to 3ae9b4a Compare August 17, 2020 11:20

ryanofsky commented Aug 17, 2020

View reviewed changes

maflcko closed this Aug 19, 2020

maflcko reopened this Aug 19, 2020

ryanofsky mentioned this pull request Aug 24, 2020

Remove gArgs global from init bitcoin/bitcoin#19779

Merged

ryanofsky added 4 commits August 26, 2020 05:52

gui: Remove unused interfaces::Node references

91aced7

Remove Node references no longer needed after previous commit

gui: Replace interface::Node references with pointers

102abff

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

maflcko reviewed Aug 26, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 26, 2020

ryanofsky force-pushed the pr/ipc-guiarg branch from 3ae9b4a to 519cae8 Compare August 26, 2020 13:20

ryanofsky commented Aug 26, 2020

View reviewed changes

maflcko merged commit 93ab136 into bitcoin-core:master Aug 26, 2020

maflcko mentioned this pull request Aug 26, 2020

Pass argsmanager reference to intro instead of relying on global #75

Closed

This was referenced Aug 26, 2020

net: Add NAT-PMP port forwarding support bitcoin/bitcoin#18077

Merged

refactor: Fix deprecation warnings when building against Qt 5.15 #46

Merged

refactor: Make BitcoinUnits::Unit a scoped enum #60

Closed

Do not block GUI thread in RPCConsole #59

Closed

This was referenced Aug 28, 2020

multiprocess: Add bitcoin-wallet -ipcconnect option bitcoin/bitcoin#19460

Draft

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

refactor: remove ::vpwallets and related global variables bitcoin/bitcoin#19101

Merged

apoelstra added a commit to apoelstra/elements that referenced this pull request Dec 3, 2020

Merge 93ab136 into merged_master (Bitcoin PR bitcoin-core/gui#35)

5da2025

stevenroose pushed a commit to ElementsProject/elements that referenced this pull request Mar 19, 2021

Merge 93ab136 into merged_master (Bitcoin PR bitcoin-core/gui#35)

89db650

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

	SetupServerArgs(*node->context());
	SetupServerArgs(node_context);

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

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

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

Conversation

ryanofsky commented Jul 16, 2020 • edited Loading

Sjors commented Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Jul 17, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Jul 17, 2020

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

hebasto commented Jul 19, 2020

hebasto left a comment

Choose a reason for hiding this comment

ryanofsky commented Jul 21, 2020

hebasto commented Jul 21, 2020 • edited Loading

ryanofsky commented Jul 21, 2020

promag commented Jul 27, 2020

luke-jr commented Jul 31, 2020

hebasto commented Aug 1, 2020

ryanofsky commented Aug 3, 2020

hebasto left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 7, 2020

hebasto left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Aug 19, 2020

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Aug 26, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Aug 26, 2020

ryanofsky commented Jul 16, 2020 •

edited

Loading

Sjors commented Jul 17, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto commented Jul 21, 2020 •

edited

Loading