Separate AppInitStartClients from AppInitMain

hebasto commented at 9:55 pm on June 12, 2021: member

This change allows to make the GUI startup process more granular, and, eventually, to move the appInitStartClients call from BitcoinCore::initialize into LoadWalletsActivity (see https://github.com/bitcoin-core/gui/pull/342) and fix https://github.com/bitcoin-core/gui/issues/247.

~~No behavior change.~~

DrahtBot added the label GUI on Jun 12, 2021

DrahtBot added the label interfaces on Jun 12, 2021

DrahtBot added the label Refactoring on Jun 12, 2021

in src/qt/bitcoin.cpp:177 in e734449b80 outdated

173@@ -174,6 +174,7 @@ void BitcoinCore::initialize()
174         qDebug() << __func__ << ": Running initialization in thread";
175         interfaces::BlockAndHeaderTipInfo tip_info;
176         bool rv = m_node.appInitMain(&tip_info);
177+        m_node.appInitStartClients();

MarcoFalke commented at 5:38 am on June 13, 2021:

I don’t think this is supposed to run when the previous call failed

hebasto commented at 12:25 pm on June 13, 2021:

Thanks! Updated.

hebasto force-pushed on Jun 13, 2021

hebasto commented at 12:24 pm on June 13, 2021: member

Updated e734449b8025b7e5a7c184672a910737b9420706 -> 3268fcc6d65d2f35313a314a6560839715fe602c (pr22231.01 -> pr22231.02, diff):

addressed @MarcoFalke’s comment

DrahtBot added the label Needs rebase on Jul 22, 2021

refactor: Separate AppInitStartClients from AppInitMain

This change allows to make the GUI startup process more granular, and,
eventually, to move the appInitStartClients call from
BitcoinCore::initialize into another thread with better user feedback.

No behavior change.

32aad05fe8

hebasto force-pushed on Jul 22, 2021

hebasto commented at 6:03 pm on July 22, 2021: member

Rebased 3268fcc6d65d2f35313a314a6560839715fe602c -> 32aad05fe88f12bf7ff5ce3acb62e6f06d944bac (pr22231.02 -> pr22231.03) due to the conflict with https://github.com/bitcoin-core/gui/pull/381.

ryanofsky approved

ryanofsky commented at 6:42 pm on July 22, 2021: member

This seems ok. I tend to think an alternate approach where bitcoin node and wallet starts up by itself and sends more frequent notifications to the GUI would be better than having GUI code drive bitcoin node and wallet loading from the outside like this.

Code review ACK 3268fcc6d65d2f35313a314a6560839715fe602c in any case

MarcoFalke commented at 6:46 pm on July 22, 2021: member

How is this a refactor when the behaviour changes? Previously StartupNotify would be called after the wallets are started, now it is called before.

DrahtBot removed the label Needs rebase on Jul 22, 2021

DrahtBot commented at 4:40 pm on July 25, 2021: member

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

Conflicts

No conflicts as of last run.

ryanofsky commented at 5:56 pm on September 7, 2021: member

How is this a refactor when the behaviour changes? Previously StartupNotify would be called after the wallets are started, now it is called before.

I’m not sure this is a problem, since I think it’s just moving wallet postInit calls, not wallet loading calls. So wallets should still be loaded and RPCs should function. But this would be good to mention in the PR description. And if there actually are user-visible changes in behavior, it would probably be good to add release notes and update -startupnotify documentation, or to update the PR to avoid them.

This seems ok. I tend to think an alternate approach where bitcoin node and wallet starts up by itself and sends more frequent notifications to the GUI would be better than having GUI code drive bitcoin node and wallet loading from the outside like this.

Note that 71d524015b57610e50f62da57497dfa020855445 from #10102 makes a similar change as this PR, but takes the alternate approach of adding a new init notification instead of adding a new init method, so common init code is driving itself and notifying the GUI, instead of the GUI driving the init.

71d524015b57610e50f62da57497dfa020855445 and this PR are both complementary and both have a similar motivation of making GUI startup more responsive. For comparison, 71d524015b57610e50f62da57497dfa020855445 moves WalletClient construction (wallet process spawning) from the GUI thread to the init executor thread, while this PR moves WalletClient::start calls from earlier in init executor thread to later in the same thread.

MarcoFalke commented at 7:47 am on September 8, 2021: member

It is absolutely a behaviour change if -startupnotify previously waited for the wallet txs mempool state to be updated, and now it doesn’t.

ryanofsky commented at 12:51 pm on September 8, 2021: member

It is absolutely a behaviour change if -startupnotify previously waited for the wallet txs mempool state to be updated, and now it doesn’t.

Makes sense and I also wonder if this doesn’t really have an impact on bitcoin-core/gui#247, contrary to the description:

This change allows to make the GUI startup process more granular, and, eventually, to move the appInitStartClients call from BitcoinCore::initialize into LoadWalletsActivity (see bitcoin-core/gui#342) and fix the bitcoin-core/gui#247.

No behavior change.

This PR only affects wallet postinit (client->start), not wallet loading (client->load), or GUI WalletModel loading.

hebasto marked this as a draft on Sep 9, 2021

hebasto renamed this:
~~refactor: Separate AppInitStartClients from AppInitMain~~
Separate AppInitStartClients from AppInitMain
on Sep 9, 2021

hebasto commented at 12:46 pm on September 12, 2021: member

@ryanofsky

It is absolutely a behaviour change if -startupnotify previously waited for the wallet txs mempool state to be updated, and now it doesn’t.

Makes sense and I also wonder if this doesn’t really have an impact on bitcoin-core/gui#247, contrary to the description:

This change allows to make the GUI startup process more granular, and, eventually, to move the appInitStartClients call from BitcoinCore::initialize into LoadWalletsActivity (see bitcoin-core/gui#342) and fix the bitcoin-core/gui#247. No behavior change.

This PR only affects wallet postinit (client->start), not wallet loading (client->load), or GUI WalletModel loading.

You’re right. This PR does not have an impact on https://github.com/bitcoin-core/gui/issues/247.

Closing.

hebasto closed this on Sep 12, 2021

DrahtBot locked this on Oct 30, 2022

Separate AppInitStartClients from AppInitMain #22231

Conflicts