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 https://github.com/bitcoin-core/gui-qml/pull/10.
The example of the re-using of the BitcoinCore class is https://github.com/bitcoin-core/gui-qml/pull/11.
0@@ -0,0 +1,66 @@
1+// Copyright (c) 2014-2021 The Bitcoin Core developers
Begging my fellow colleagues for review :
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.)
This seems good. Also maybe if
BitcoinCoreclass is moving to it’s own file, it could be renamed to something that better describes what it does likeInitExecutor(similar toRPCExecutor) orInitActivity(likeCreateWalletActivityandOpenWalletActivity) or justInitThreadInitTask.
Many thanks for feedback. Which name could also describe that this class is also used to provide a shutdown activity?
13@@ -14,6 +14,7 @@
14 <ClCompile Include="..\..\src\qt\bitcoin.cpp" />
15 <ClCompile Include="..\..\src\qt\bitcoinaddressvalidator.cpp" />
16 <ClCompile Include="..\..\src\qt\bitcoinamountfield.cpp" />
17+ <ClCompile Include="..\..\src\qt\bitcoincore.cpp" />
In commit “qt, refactor: Fix code styling of moved BitcoinCore class” (6c2969da1dd164c6d7acfeb45b55ed95d530c76e)
I think these changes were probably meant for the earlier commit introducing this file instead of this commit
300@@ -291,22 +301,14 @@ bool BitcoinApplication::baseInitialize()
301
302 void BitcoinApplication::startThread()
303 {
304- if(coreThread)
305- return;
306- coreThread = new QThread(this);
307- BitcoinCore *executor = new BitcoinCore(node());
308- executor->moveToThread(coreThread);
309+ m_core = std::make_unique<BitcoinCore>(node());
In commit “qt: Add BitcoinCore::m_thread member” (6b769d9e9e1c73cef37826e3c842fe9b65737456)
Might be good to assert(!m_core) before setting this. Also I think m_executor would be a more descriptive name than m_core
300@@ -291,22 +301,14 @@ bool BitcoinApplication::baseInitialize()
301
302 void BitcoinApplication::startThread()
303 {
304- if(coreThread)
305- return;
306- coreThread = new QThread(this);
In commit “qt: Add BitcoinCore::m_thread member” (6b769d9e9e1c73cef37826e3c842fe9b65737456)
The coreThread member doesn’t actually do anything after this change. Would be less confusing to squash the next commit into this one and delete coreThread entirely instead of keeping it half-used.
163+}
164+
165+BitcoinCore::~BitcoinCore()
166+{
167+ qDebug() << __func__ << ": Stopping thread";
168+ m_thread.quit();
In commit “qt: Add BitcoinCore::m_thread member” (6b769d9e9e1c73cef37826e3c842fe9b65737456)
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.
Code review ACK 7ec552676c66488fe00fb503d02ec4a389a715b7
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? :shrug: 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.
Updated 6c2969da1dd164c6d7acfeb45b55ed95d530c76e -> 0c7667f522f7da8155b7e959e550868210dbf033 (pr381.02 -> pr381.03, diff). @ryanofsky
Thank you for your review! Your comments are addressed.
115@@ -112,7 +116,7 @@ public Q_SLOTS:
116 void windowShown(BitcoinGUI* window);
117
118 private:
119- QThread *coreThread;
120+ std::unique_ptr<BitcoinCore> m_executor;
In commit “qt: Add BitcoinCore::m_thread member” (efacacba98aa6d9d0794f62032eb353ced1e0791)
Not important, but this could use std::optional instead of std::unique_ptr to avoid a dynamic allocation
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.
MacOS 11.4
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)
m_executor from pointer to optional type (thanks for update!)
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 8169fc4e73a87331e02502fc24e293831765c8b1
