Ditch wallet model juggling

[promag]

Instantiate WalletModel in the main thread - avoids callingsetParent and moveToThread.

Also ensures loading (fetching transactions, addresses, etc) still occurs in the background thread.

[hebasto]

There is a change in behavior because now everything that is loaded in WalletModel instantiation happens in the GUI thread.

Shouldn't we move in the opposite direction?

[promag]

@hebasto at the moment loading happens on a separate thread but it loads everything, which is not ideal. For now, I think I'll move the loading code from constructors to separate functions so that only QObject instantiations happen on the GUI thread. But later we need to lazy load wallet data.

[promag]

@hebasto added fa425d10753fb1a0ce331343b4548a68fcb0b954 to address the above comment, also updated OP.

[promag]

Rebased.

[hebasto]

Depends on bitcoin/bitcoin#22868 because otherwise, a deadlock would occur during the wallet loading.

bitcoin/bitcoin#22868 has been merged. Rebase?

[DrahtBot]

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

Conflicts

No conflicts as of last run.

[hebasto]

This change takes advantage of GUIUtil::ObjectInvoke to avoid workarounds around how Qt handles object instantiation and threads.

GUIUtil::ObjectInvoke seems a bit outdated :)

[hebasto]

cc @ryanofsky

[ryanofsky]

IIUC, the second commit "qt: Move wallet preloading out of model constructors" (e2ffc98e73df68d0de9755d2ebbb26daa17c92fc) seems like a good change, moving wallet "preloading" tasks off of the GUI thread into timer threads or notification callback threads. But I'm not 100% sure this is what it is doing. Would be helpful if the commit description said what loading behavior was before the change and how it is different afterward.

The first commit "qt: Ditch wallet model juggling" (f3e70479257770bc9f61de694db8704140139548) is more confusing, because it seems like the new code is doing the same thing as the old code, just making slightly different function calls to do the same thing. This is probably ok, but it would be helpful if commit message said whether this is just supposed to be a local code cleanup, or a bigger change that affects other code. Also that commit seems to delete comments which I think are helpful about why particular threads are assigned. The new comment says what thread is assigned but not why.

[promag]

@ryanofsky

The first commit "qt: Ditch wallet model juggling" (f3e7047) is more confusing, because it seems like the new code is doing the same thing as the old code, just making slightly different function calls to do the same thing. This is probably ok, but it would be helpful if commit message said whether this is just supposed to be a local code cleanup, or a bigger change that affects other code. Also that commit seems to delete comments which I think are helpful about why particular threads are assigned. The new comment says what thread is assigned but not why.

It doesn't do the same thing as before since WalletModel constructor is executed in the GUI thread whereas before it was executed in the wallet controller background thread or on the RPC handling thread. This change makes loading big wallets block the GUI. But this blocking issue is "fixed" in the following commit.

In terms of Qt calls, it avoids moveToThread and setParent, since now WalletModel is instanced in the GUI thread with the right parent.

IIUC, the second commit "qt: Move wallet preloading out of model constructors" (e2ffc98) seems like a good change, moving wallet "preloading" tasks off of the GUI thread into timer threads or notification callback threads. But I'm not 100% sure this is what it is doing. Would be helpful if the commit description said what loading behavior was before the change and how it is different afterward.

So, the WalletModel constructor instantiated the remaining models (like TransactionTableModel) and those models would load everything from interfaces::Wallet. This happened on the wallet controller background thread or the RPC handling thread, depending on how the wallet was loaded. So, the WalletModel constructor could block for a while for big wallets - that was the reason for not instantiating the model in the GUI thread.

This commit splits instancing models and loading data. Instancing models still happens on the GUI thread while loading data happens on a separate thread. This "fixes" the blocking issue introduced in the 1st commit.

---

Note that, even though preload happens on a background thread, we can have a blocked GUI because underneath loading locks some mutexes that other parts of the GUI could also try to lock. For instance, WalletImpl::getWalletTxs locks cs_wallet, and the more transactions the worse.

Now that we have the loading split from the instancing, we can improve the loading for big wallets. For instance:

• load transactions only when the user opens the transactions list, same for addresses, etc
• adopt a lazy loading approach
• chunked loading where locks don't get locked for so long.
• read-write locks (?)

[promag]

@ryanofsky cool, I'll address your comments, makes sense to reorder commits.