refactor: Make BitcoinCore class reusable by hebasto · Pull Request #381 · bitcoin-core/gui

hebasto · 2021-07-14T11:02:43Z

This PR makes the BitcoinCore class reusable, i.e., it can be used by the widget-based GUI or by the QML-based one, and it makes the divergence between these two repos minimal.

The small benefit to the current branch is more structured code.

Actually, this PR is ported from bitcoin-core/gui-qml#10.
The example of the re-using of the BitcoinCore class is bitcoin-core/gui-qml#11.

hebasto · 2021-07-14T11:04:35Z

src/qt/bitcoincore.cpp

@@ -0,0 +1,66 @@
+// Copyright (c) 2014-2021 The Bitcoin Core developers


The initial code was contributed by @laanwj back in 2014.

hebasto · 2021-07-14T11:08:09Z

Begging my fellow colleagues for review :

@laanwj as an author of the initial code
@promag as an expert in Qt code
@ryanofsky as an expert in multi-threading

ryanofsky · 2021-07-14T11:36:25Z

This seems good. Also maybe if BitcoinCore class is moving to it's own file, it could be renamed to something that better describes what it does like InitExecutor (similar to RPCExecutor) or InitActivity (like CreateWalletActivity and OpenWalletActivity) or just InitThread InitTask.

(Longer term, I think these asynchronous executor / activity classes are mostly unnecessary and could be replaced with an AsyncUpdate or similar utility, but that's beyond the scope of this PR.)

hebasto · 2021-07-14T12:58:16Z

@ryanofsky

This seems good. Also maybe if BitcoinCore class is moving to it's own file, it could be renamed to something that better describes what it does like InitExecutor (similar to RPCExecutor) or InitActivity (like CreateWalletActivity and OpenWalletActivity) or just InitThread InitTask.

Many thanks for feedback. Which name could also describe that this class is also used to provide a shutdown activity?

ryanofsky

Code review ACK 7ec5526

Many thanks for feedback. Which name could also describe that this class is also used to provide a shutdown activity?

Personally I like InitExecutor. Shutdown just seems like part of initialization. Maybe StartupExecutor? 🤷 I do think pretty much any name would be more descriptive than a class or file called bitcoin core. But feel free to keep the current name or choose any name that seems good to you.

build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj

src/qt/bitcoin.cpp

ryanofsky · 2021-07-14T15:05:16Z

src/qt/bitcoin.cpp

+BitcoinCore::~BitcoinCore()
+{
+    qDebug() << __func__ << ": Stopping thread";
+    m_thread.quit();


In commit "qt: Add BitcoinCore::m_thread member" (6b769d9)

I'm not sure if it matters but a possible concern is that when bitcoinapplication is destroyed, this thread wait now happens after the window and platform objects are instead of before they are deleted in ~BitcoinApplication. This may be safe but it might be safer to make sure these objects are not deleted while the thread is running and maybe working. You could do this by adding m_core.reset() to the beginning of ~BitcoinApplication. But fine to skip this if we know the thread will already be done working when ~BitcoinApplication runs.

Thanks! Updated.

hebasto · 2021-07-14T16:23:48Z

Updated 6c2969d -> 0c7667f (pr381.02 -> pr381.03, diff).

@ryanofsky

Thank you for your review! Your comments are addressed.

ryanofsky

Code review ACK 0c7667f. Thanks! Just renaming and other suggested changes since last review

ryanofsky · 2021-07-14T18:00:14Z

src/qt/bitcoin.h


 private:
-    QThread *coreThread;
+    std::unique_ptr<BitcoinCore> m_executor;


In commit "qt: Add BitcoinCore::m_thread member" (efacacb)

Not important, but this could use std::optional instead of std::unique_ptr to avoid a dynamic allocation

Thank! Updated.

This change makes BitcoinCore self-contained to improve its re-usability. BitcoinApplication::coreThread member is now unused, and removed.

-BEGIN VERIFY SCRIPT- sed -i -e 's/BitcoinCore/InitExecutor/g' src/qt/bitcoin.* -END VERIFY SCRIPT-

This change makes InitExecutor class re-usable by an alternative GUI, e.g., QML-based one.

hebasto · 2021-07-14T18:58:21Z

Updated 0c7667f -> 8169fc4 (pr381.03 -> pr381.04, diff):

addressed @ryanofsky's comment

RandyMcMillan · 2021-07-17T01:01:22Z

MacOS 11.4

laanwj · 2021-07-21T16:27:56Z

I do think pretty much any name would be more descriptive than a class or file called bitcoin core.

Originally I called the class "BitcoinCore", not because the software is called like that (not sure if it even did at the time?), but because it wraps an instance of the core (=non-gui) functionality, to be executed in its own thread. But like bitcoind's main thread, it does nothing but initialization and shutdown.

(I don't mind it being renamed, but it was not as silly as a "let's call a class the same name as the software" at the time)

ryanofsky · 2021-07-21T17:06:14Z

Yeah I figured "core" was a synonym for "init". Just makes sense to make the old name more descriptive when the class is moved and more widely exposed now.

ryanofsky

Code review ACK 8169fc4. Only change is switching from m_executor from pointer to optional type (thanks for update!)

laanwj · 2021-07-22T07:57:25Z

Yeah I figured "core" was a synonym for "init". Just makes sense to make the old name more descriptive when the class is moved and more widely exposed now.

Well not "init", more like "root model object" (e.g. I think it was supposed to create and manage ClientModel, WalletModels etc). But it's okay, I'm fine with naming it after what it is now doing.

ACK 8169fc4

promag

Code review ACK 8169fc4.

hebasto commented Jul 14, 2021

View reviewed changes

hebasto force-pushed the 210714-core branch from 7495725 to 6c2969d Compare July 14, 2021 12:54

ryanofsky approved these changes Jul 14, 2021

View reviewed changes

hebasto force-pushed the 210714-core branch from 6c2969d to 0c7667f Compare July 14, 2021 16:18

hebasto mentioned this pull request Jul 14, 2021

refactor: Make BitcoinCore class reusable bitcoin-core/gui-qml#10

Closed

ryanofsky approved these changes Jul 14, 2021

View reviewed changes

hebasto added 4 commits July 14, 2021 21:53

qt: Add BitcoinCore::m_thread member

19a1d00

This change makes BitcoinCore self-contained to improve its re-usability. BitcoinApplication::coreThread member is now unused, and removed.

scripted-diff: Rename BitcoinCore class to InitExecutor

dbcf56b

-BEGIN VERIFY SCRIPT- sed -i -e 's/BitcoinCore/InitExecutor/g' src/qt/bitcoin.* -END VERIFY SCRIPT-

qt, refactor: Move InitExecutor class into its own module

c82165a

This change makes InitExecutor class re-usable by an alternative GUI, e.g., QML-based one.

qt, refactor: Fix code styling of moved InitExecutor class

8169fc4

hebasto force-pushed the 210714-core branch from 0c7667f to 8169fc4 Compare July 14, 2021 18:56

hebasto added the Refactoring label Jul 14, 2021

bitcoin-core deleted a comment Jul 16, 2021

bitcoin-core deleted a comment Jul 17, 2021

ryanofsky approved these changes Jul 21, 2021

View reviewed changes

laanwj merged commit 36aee0f into bitcoin-core:master Jul 22, 2021

promag reviewed Jul 22, 2021

View reviewed changes

hebasto mentioned this pull request Jul 22, 2021

Sync with the main repo bitcoin-core/gui-qml#13

Merged

hebasto deleted the 210714-core branch July 22, 2021 14:32

This was referenced Jul 22, 2021

Do not exit and re-enter main event loop during shutdown #336

Merged

Separate AppInitStartClients from AppInitMain bitcoin/bitcoin#22231

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

Merge bitcoin-core/gui#381: refactor: Make BitcoinCore class reusable

cd5840d

ryanofsky mentioned this pull request Aug 17, 2021

multiprocess: Start using init makeNode, makeChain, etc methods bitcoin/bitcoin#22219

Merged

hebasto mentioned this pull request Sep 27, 2021

Keep InitExecutor in main gui thread #434

Merged

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

		@@ -0,0 +1,66 @@
		// Copyright (c) 2014-2021 The Bitcoin Core developers

Conversation

hebasto commented Jul 14, 2021

Uh oh!

hebasto Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 14, 2021

Uh oh!

ryanofsky commented Jul 14, 2021

Uh oh!

hebasto commented Jul 14, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 14, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 14, 2021

Uh oh!

RandyMcMillan commented Jul 17, 2021

Uh oh!

laanwj commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 21, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

laanwj commented Jul 21, 2021 •

edited

Loading

laanwj commented Jul 22, 2021 •

edited

Loading