gui: Long or blocking calls should be async

[promag]

The comment here is not entirely correct:

bitcoin/src/qt/walletmodel.cpp

Lines 71 to 74 in 4bfef0d

	// Try to get balances and return early if locks can't be acquired. This
	// avoids the GUI from getting stuck on periodical polls if the core is
	// holding the locks for a longer time - for example, during a wallet
	// rescan.

The problem happens when the locks are held and calculating the balance takes a lot of time - causing the GUI to stuck.

Beside the obvious improvement - drop polling and update only when necessary - the actual balance calculation must be asynchronous otherwise Qt's event loop isn't able to make the GUI smooth - noticeable on big wallet. This also applies for anything that takes a lot - whether it requires other locks or not.

[ryanofsky]

I think ideally the way for the gui to receive these updates would be with a new wallet callback handleLastBlockChanged analogous to existing handleAddressBookChanged, handleTransactionChanged, etc callbacks. The callback could be invoked whenever CWallet::m_last_block_processed is updated and pass along
CWallet::m_last_block_processed_height or other useful state values.

This way no polling would be required at all.

---

In the meantime, there have been a lot of PRs affecting polling and tip updates. Making a list here to keep track.

Two changes were made to make polling less expensive and block less:

• gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18160 (merged) avoids expensive wallet balance calls during polling
• gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587 (pending) reverts previous change and takes it further, implementing it entirely in the GUI and not calling out to node or wallet

Other polling changes:

• gui: Balance/TxStatus polling update based on last block hash. #17993 (pending) switches all block change checks in GUI code from height checks to hash checks so the checks are more reliable

• gui: Avoid redundant tx status updates #17905 (merged) avoids repeated calls from the gui to the wallet to update transaction statuses as they are displayed, by skipping updates if the last block isn't changed

Other notification changes:

• gui: Throttle GUI update pace when -reindex #18121 (merged) - avoids rapid gui updates in tip changed callback during reindexing
• qt: Use SynchronizationState enum for signals to GUI #18152 (pending) - avoids getReindex and isInitialBlockDownload calls during tip changed callback and
• relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload #19007 (pending) - subset of qt: Use SynchronizationState enum for signals to GUI #18152, avoid isInitialBlockDownload call during tip changed callback

[promag]

This way no polling would be required at all.

Yes! Title is misleading.

The approach you suggest looks fine to me but it doesn't prevent going back to node/wallet and request more data which in turn can disturb the GUI responsiveness.

[jonasschnelli]

When running with a wallet, I encountered that one of the most annoying blockings I had in mainnet-catchup was SendCoinsDialog::updateSmartFeeLabel().
We call CTxMemPool::GetMinFee() on all tip updates which is fine when in-sync but problematic otherwise.

It seems to be a mempool lock issue. The backtrace where the GUI thread gets stuck for seconds:

    frame #8: 0x000000010fe75136 bitcoin-qt`CTxMemPool::GetMinFee(this=0x0000000110cd75e8, sizelimit=300000000) const at txmempool.cpp:1004:5
    frame #9: 0x000000010fa6c531 bitcoin-qt`interfaces::(anonymous namespace)::ChainImpl::mempoolMinFee(this=0x00007fee41f46730) at chain.cpp:332:26
    frame #10: 0x0000000110194e21 bitcoin-qt`GetMinimumFeeRate(wallet=0x00007fee35804080, coin_control=0x00007fee35b8a880, feeCalc=0x00007ffee04d10f8) at fees.cpp:67:55
    frame #11: 0x0000000110194b06 bitcoin-qt`GetMinimumFee(wallet=0x00007fee35804080, nTxBytes=1000, coin_control=0x00007fee35b8a880, feeCalc=0x00007ffee04d10f8) at fees.cpp:20:12
    frame #12: 0x000000011011fa26 bitcoin-qt`interfaces::(anonymous namespace)::WalletImpl::getMinimumFee(this=0x00007fee367175b0, tx_bytes=1000, coin_control=0x00007fee35b8a880, returned_target=0x00007ffee04d12a4, reason=0x00007ffee04d12a0) at wallet.cpp:428:18
    frame #13: 0x000000010f8d4ace bitcoin-qt`SendCoinsDialog::updateSmartFeeLabel(this=0x00007fee35b89e50) at sendcoinsdialog.cpp:755:49

[ryanofsky]

I guess the scope of this issue has changed since or was different than I thought when I replied #16874 (comment). At the time the issue title was "gui: Poll for balances should be async" not "gui: Long or blocking calls should be async". These are different problems. It is relatively easy to move slow polling code off the main event loop thread to a different thread, as opposed to getting slow code which is actually triggered by UI events (like SendCoinsDialog::updateSmartFeeLabel in Jonas's example) to not block the event loop. In the latter case you need one of the approaches from https://doc.qt.io/archives/qq/qq27-responsive-guis.html or #10504, beyond just a different choice of thread for polling.

[promag]

@ryanofsky right, the real problem are the long/blocking calls on the GUI thread - polling one of those just makes it more noticeable. Polling in some thread and sending the results to the GUI thread is what makes more sense to me.

as opposed to getting slow code which is actually triggered by UI events

This is what is done in OpenWalletActivity::open.