Unregister wallet notifications before unloading wallets

ryanofsky commented at 5:34 pm on June 10, 2021: contributor

This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.

gui: Unregister wallet notifications before unloading wallets

This change was originally part of both bitcoin/bitcoin#10102 and
bitcoin/bitcoin#19101 and is required for both because it avoids the IPC
wallet implementation in bitcoin/bitcoin#10102 and the WalletContext
implementation in bitcoin/bitcoin#19101 needing to deal with
notification objects that have stale pointers to deleted wallets.

93cc53a2b2

hebasto added the label Wallet on Jun 10, 2021

hebasto renamed this:
~~gui: Unregister wallet notifications before unloading wallets~~
Unregister wallet notifications before unloading wallets
on Jun 10, 2021

promag commented at 10:35 pm on June 11, 2021: contributor

Code review ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1.

in src/qt/bitcoin.cpp:359 in 93cc53a2b2

351@@ -352,6 +352,17 @@ void BitcoinApplication::requestShutdown()
352     window->setClientModel(nullptr);
353     pollShutdownTimer->stop();
354 
355+#ifdef ENABLE_WALLET
356+    // Delete wallet controller here manually, instead of relying on Qt object
357+    // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
358+    // walletmodel m_handle_* notification handlers are deleted before wallets
359+    // are unloaded, which can simplify wallet implementations. It also avoids

hebasto commented at 1:00 pm on August 5, 2021:

Delete wallet controller here manually… This makes sure walletmodel m_handle_* notification handlers are deleted before wallets are unloaded, which can simplify wallet implementations.

Sorry, but I cannot get it. To achieve the goal–notification handler deletion–the ~WalletModel destructor must be called. But I cannot see the way how deletion of the m_wallet_controller could works here, as an instance of the WalletController class does not own WalletModel objects, because WalletController::m_wallets is std::vector<WalletModel*>, not std::vector<std::unique_ptr<WalletModel>>.

Did I miss the point?

ryanofsky commented at 8:00 pm on August 5, 2021:

re: #360 (review)

To achieve the goal–notification handler deletion–the ~WalletModel destructor must be called. But I cannot see the way how deletion of the m_wallet_controller could works here, as an instance of the WalletController class does not own WalletModel objects, because WalletController::m_wallets is std::vector<WalletModel*>, not std::vector<std::unique_ptr<WalletModel>>.

Did I miss the point?

No, this is a reasonable question because wallet controller’s ownership of wallet models is implicit instead of explicit. But m_wallet vector usage is just emulating unique_ptr without using unique_ptr for some reason (probably no reason).

In the case where an m_wallet entry is erased, the pointer is deleted:

https://github.com/bitcoin-core/gui/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/qt/walletcontroller.cpp#L179-L186

In the case where the m_wallet vector is cleared, the pointers are deleted:

https://github.com/bitcoin-core/gui/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/qt/walletcontroller.cpp#L108-L114

And in the case where the controller object is destroyed, any models that weren’t cleared or erased are destroyed:

https://github.com/bitcoin-core/gui/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/qt/walletcontroller.cpp#L142-L146

I could add more details to this comment, but I don’t think it would be appropriate to mention implementation details of the wallet controller outside the wallet controller (especially when implementation seems unnecessarily complicated and could probably be simplified later). So hopefully this reply clears things up.

Thanks for looking into this!

hebasto commented at 11:21 am on August 7, 2021: member

Approach ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1.

The forced destroying of m_wallet_controller indeed achieves the desired goals.

But I’m still uncomfortable with the suggested comment, because we still rely on Qt object ownership.

Setting a ~WalletModel as a breakpoint clearly proves the statement above:

 0$ lldb ./src/qt/bitcoin-qt -- -signet
 1(lldb) target create "./src/qt/bitcoin-qt"
 2Current executable set to '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64).
 3(lldb) settings set -- target.run-args  "-signet"
 4(lldb) br s -n ~WalletModel
 5Breakpoint 1: 2 locations.
 6(lldb) run
 7Process 116788 launched: '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64)
 8Process 116788 stopped
 9* thread [#1](/bitcoin-core-gui/1/), name = 'bitcoin-qt', stop reason = breakpoint 1.2
10    frame [#0](/bitcoin-core-gui/0/): 0x000055555565c07a bitcoin-qt`WalletModel::~WalletModel(this=0x00005555571b4aa0) at walletmodel.cpp:61:1
11(lldb) bt
12* thread [#1](/bitcoin-core-gui/1/), name = 'bitcoin-qt', stop reason = breakpoint 1.2
13  * frame [#0](/bitcoin-core-gui/0/): 0x000055555565c07a bitcoin-qt`WalletModel::~WalletModel(this=0x00005555571b4aa0) at walletmodel.cpp:61:1
14    frame [#1](/bitcoin-core-gui/1/): 0x00007ffff7a6701e libQt5Core.so.5`QObjectPrivate::deleteChildren() + 110
15    frame [#2](/bitcoin-core-gui/2/): 0x00007ffff7a715ef libQt5Core.so.5`QObject::~QObject() + 1455
16    frame [#3](/bitcoin-core-gui/3/): 0x0000555555655157 bitcoin-qt`WalletController::~WalletController(this=0x0000555557358d10) at walletcontroller.cpp:61:1
17    frame [#4](/bitcoin-core-gui/4/): 0x00005555556551cf bitcoin-qt`WalletController::~WalletController(this=0x0000555557358d10) at walletcontroller.cpp:57:1
18    frame [#5](/bitcoin-core-gui/5/): 0x00005555555d9c63 bitcoin-qt`BitcoinApplication::requestShutdown(this=0x00007fffffffd730) at bitcoin.cpp:362:5
19    frame [#6](/bitcoin-core-gui/6/): 0x00005555555dc269 bitcoin-qt`GuiMain(argc=2, argv=<unavailable>) at bitcoin.cpp:650:17
20    frame [#7](/bitcoin-core-gui/7/): 0x00005555555d70ea bitcoin-qt`main(argc=<unavailable>, argv=<unavailable>) at main.cpp:21:43
21    frame [#8](/bitcoin-core-gui/8/): 0x00007ffff61fa0b3 libc.so.6`__libc_start_main + 243
22    frame [#9](/bitcoin-core-gui/9/): 0x00005555555d700e bitcoin-qt`_start + 46

A WalletModel object becomes a child of the WalletController object here:https://github.com/bitcoin-core/gui/blob/03826aecc56c5c5c76570805897c2ddf92e11ab6/src/qt/walletcontroller.cpp#L143

ryanofsky commented at 4:54 pm on August 12, 2021: contributor

But I’m still uncomfortable with the suggested comment, because we still rely on Qt object ownership.

Hmm, maybe you have a suggestion to reword the comment. The comment “Delete wallet controller here manually, instead of relying on Qt object tracking” is not saying we no longer rely on Qt object tracking generally. It is just saying we no longer rely on it to delete the wallet controller object.

The wallet model is a different object from the wallet controller. It would be nice to manage wallet model objects with smart pointer ownership, but not necessary. This PR is necessary and is blocking bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 because these PR’s can’t deal with the GUI having stale walletmodel m_handle_* notification handlers associated with unloaded wallets.

hebasto commented at 4:58 pm on August 12, 2021: member

@ryanofsky

The comment “Delete wallet controller here manually, instead of relying on Qt object tracking” is not saying we no longer rely on Qt object tracking generally. It is just saying we no longer rely on it to delete the wallet controller object.

You are right.

hebasto approved

hebasto commented at 4:59 pm on August 12, 2021: member

ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1

hebasto merged this on Aug 12, 2021

hebasto closed this on Aug 12, 2021

bitcoin-core locked this on Aug 16, 2022

Unregister wallet notifications before unloading wallets #360