Call interfaces::Wallet::getWalletTxs asynchronous #20241

This function is only used by TransactionTableModel constructor which, for big wallets, causes a delay/hang on startup. This is even noticeable in #10102.

There's two simple changes to improve this:

1. defer getWalletTxs to when the transactions view is opened
2. call in a background thread to not block Qt event loop, like wallet loading

However the following issues would still be present:

• doesn't reduce IPC traffic
• keeps a huge lock on cs_wallet
• loads the whole wallet in the transaction model unnecessarily (the view only shows a subset)

Considering the above, one way to improve is to paginate getWalletTxs.

However, TransactionTableModel can decomposes each transaction in multiple records, so it's not possible to know beforehand the row count and also because each page of transactions results in an indeterminate number of rows.

I have 2 different approaches to improve:

a) manage wallet records on CWallet and expose them with interfaces::Wallet::getRecords that supports pagination b) instead of table model, make records children of a row for each txid - this would allow to reopen #12578.

I'd love to know other's thoughts on this matter. Are the 2 simple changes 1. and 2. enough for the moment?

I think the simple fix is to move work off of the GUI thread so the so GUI is responsive. It seems like a much smaller problem if a few wallet operations are temporarily unavailable while a big wallet is being loaded, as long as other GUI & RPC operations for other wallets and the node are available and everything is responsive.

Making the wallet/gui interface paginated seems useful if wallet code accessed transactions through database lookups, but much less useful if everything is in memory anyway

Reading more closely, I guess I'm just describing the approach suggested in the issue description (not the issue title). Sounds good!

@ryanofsky while working on this I've found

• an issue that is described and fixed in https://github.com/bitcoin-core/gui/pull/120
• an unlikely issue regarding transaction notifications but can be problematic with the background getWalletTxs approach, see https://github.com/bitcoin-core/gui/pull/121.

After these, the idea is to just call getWalletTxs in a background thread and have some kind of non-blocking approach to update the Qt model - probably transaction decompose also in background thread.

Are the 2 simple changes 1. and 2. enough for the moment?

Yes, I think so. It will be a huge improvement.

@hebasto is it worth moving this issue to the GUI repo?

@hebasto is it worth moving this issue to the GUI repo?

I'd keep it here as this issue's solution potentially can touch code out of the qt subdirectory.

This issue hasn’t attracted much interest from other contributors in quite some time.

Given that, it doesn’t seem important enough to keep open indefinitely. I’m going to close it for now due to lack of activity, but pull requests or renewed discussion are always welcome.

Comment here if you think this should be re-opened.