Separate AppInitStartClients from AppInitMain #22231
Conversation
|
Updated e734449 -> 3268fcc (pr22231.01 -> pr22231.02, diff):
|
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.
|
Rebased 3268fcc -> 32aad05 (pr22231.02 -> pr22231.03) due to the conflict with bitcoin-core/gui#381. |
ryanofsky
left a comment
There was a problem hiding this comment.
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 3268fcc in any case
|
How is this a refactor when the behaviour changes? Previously |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
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
Note that 71d5240 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. 71d5240 and this PR are both complementary and both have a similar motivation of making GUI startup more responsive. For comparison, 71d5240 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. |
|
It is absolutely a behaviour change if |
Makes sense and I also wonder if this doesn't really have an impact on bitcoin-core/gui#247, contrary to the description:
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 bitcoin-core/gui#247. Closing. |
This change allows to make the GUI startup process more granular, and, eventually, to move the
appInitStartClientscall fromBitcoinCore::initializeintoLoadWalletsActivity(see bitcoin-core/gui#342) and fix bitcoin-core/gui#247.No behavior change.