Qt: fixes excessive thread blocking for large wallets.

[tomasz-gac]

wallet/wallet.h
- added NotifyBlockConnected slot to CWallet

wallet/wallet.cpp
- implemented signal invocation for NotifyBlockConnected

qt/walletmodel.cpp
- WalletModel::pollBalanceChanged no longer calls TransactionTableModel::updateConfirmations
every MODEL_UPDATE_DELAY == 250 ms.

qt/transactiontablemodel.cpp
- TransactionTablePriv::index does not handle cachedWallet updates anymore.
It was called from TransactionTableModel::updateConfirmations every 250 ms.
Profiling shows that it stalls the threads for big wallets because it locks cs_main and cs_wallet.
- TransactionTablePriv::updateWalletCache updates all cachedWallet entries.
- TransactonTableModel::updateConfirmations calls TransactionTablePriv::updateWalletCache
before emitting the dataChanged signal.
- TransactionTableModel subscribes to CWallet::NotifyBlockConnected to call updateConfirmations

[PastaPastaPasta]

Thanks for this!

I haven't actually looked at the code yet, however, the style is quite messed up. Please take a look here https://github.com/dashpay/dash/blob/master/doc/developer-notes.md at the style guidelines and adjust the style.

Thanks!

[UdjinM6]

Thanks for submitting! It's an interesting idea and but this patch makes qt completely unresponsive while syncing blocks and I had to kill the process to stop it.

[tomasz-gac]

Thanks for submitting! It's an interesting idea and but this patch makes qt completely unresponsive while syncing blocks and I had to kill the process to stop it.

Ah, yes. I bet there's a lot of BlockConnected signals during synchronization and it makes the qt unresponsive. I will either have defer the subscription to this signal after synchro, or revert to double TRY_LOCK in the TransactionTablePriv::updateWalletCache loop. That would, in turn, possibly skip some of the transaction table entries and make the information there unreliable.

[PastaPastaPasta]

One thing that I found was that qt was unresponsive on an existing datadir but on initial sync I wasn't getting those problems.

Edit: This is likely because the new datadir has no transactions the wallet has to deal with

[tomasz-gac]

Latest commit fixes cases where NotifyBlockConnected is called frequently (E.g. during synchronization). It will now be called at most once every MODEL_UPDATE_DELAY == 250 ms.

I also tried to change the style to conform to guidelines.

[PastaPastaPasta]

Haven't tested the new version yet, but... The style is still quite messed up. Couple of points. see comments in line, also, braces for for loops, ifs whiles etc should be on same line, brackets for functions should be on new line. Make sure your tabs/indents are four spaces, I'm seeing quite a few two spaces

[UdjinM6]

Slightly tested and it seems to be working fine now. 4194c09 is way too excessive however (which even causes a merge conflict) - you should not refactor/fix the code style for parts you did not touch in your actual fix, pls redo.

[tomasz-gac]

I reverted the previous style changes (which were applied to the whole file) and contained them to my own code.

[UdjinM6]

After testing it a bit more by reindexing a node with a huge wallet (and comparing results to develop) I see no improvement in reindex time but Qt is still much less responsive even with 9aed976. I think the reason is that you still have to run updateStatus for the whole cachedWallet every 250ms (cause blocks arrive quickly) while in develop you need to run it for visible txes only thanks to index. (EDIT: Actually, it's probably both this and switching from TRY_LOCK to LOCK, so it's "stuck until can lock and update" vs "can't update? no big deal, will try later") Another issue with this patch is that you have to update status not only when block is connected but also when it's disconnected e.g. e463991e32e68eba7a095749a6ccd474dc8cf330. So IMO, we should not touch transactiontablemodel which is actually working better with huge wallets comparing to the patch but we could reuse parts of this patch to fix excessive locking in walletmodel instead e.g. 8ae782ab2ab1139c8bb7df8ff673506a97ce1fbd.

EDIT: PS. there are also some style fixes missing, see f366ca801a0abeb85a837fd9a68c92c46f360c18

[tomasz-gac]

@UdjinM6 are your test results expressed in terms of subjective or objective responsiveness?
While it may be true that original filtered transaction table is faster while reindexing, I wonder whether it is the use case that we're supposed to optimize for.
When developing this change, I was tasked with researching why is dash-qt slowing down with respect to RPC calls for large (~100MB) wallets. Profiling shows that during unfiltered operation, the application spends most time in GUI handling and locking, as opposed to dashd's BLS operations.
profiling.zip

[github-actions]

This pull request has conflicts, please rebase.

[DashCoreAutoGuix]

❌ Backport Verification - Issues Detected

Original Bitcoin commit: None (Dash-specific change)
Reviewed commit hash: 54c20a2eba-verify-1753720946

Issues found:

• Not a Bitcoin backport - this is a Dash-specific Qt optimization
• Historical comments from 2020 indicate Qt becomes unresponsive during sync
• PR has been abandoned since 2020 with conflicts reported in 2021
• No current CI status available to verify functionality

❌ Analysis Results

This PR appears to be a Dash-specific optimization for Qt transaction table performance with large wallets. However:

1. Performance Issues: Original reviewer (@UdjinM6) reported Qt became completely unresponsive during block sync
2. Abandoned State: Last activity was in 2020, with bot reporting conflicts in 2021
3. No Bitcoin Origin: This is not a Bitcoin backport but a custom Dash optimization
4. Missing Validation: No CI status available to verify current functionality

Please address these issues and re-run verification. Consider closing this PR if the Qt responsiveness issues cannot be resolved.

[thepastaclaw]

⛔ Final review complete — 3 blocking finding(s) (commit 54c20a2)

[thepastaclaw]

✅ Review complete (commit 54c20a2e)

Review: #3362 (review)

[thepastaclaw]

Code Review

PR refactors Qt transaction table to batch-update statuses via a cache-invalidation flag set from NotifyBlockConnected, but the new path is too narrow and unsafe. Three blocking issues: (1) the wallet→GUI callback mutates the cache flag from the wallet thread without using Qt::QueuedConnection, racing with the GUI poll; (2) ChainLock-driven status transitions no longer refresh until an unrelated block arrives; (3) BlockDisconnected/reorgs never invalidate the cache, so confirmation counts can be stale for arbitrary periods. Additional issues include style/whitespace regressions, an unused txids vector built under cs_main+cs_wallet on every block, and no tests added.

Reviewed commit: 54c20a2

🔴 3 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)

9 additional finding(s)

🔴 blocking: NotifyBlockConnected mutates the model from the wallet thread without QueuedConnection

src/qt/transactiontablemodel.cpp (lines 842-845)

NotifyBlockConnected (transactiontablemodel.cpp:842-845) calls ttm->invalidateWalletCache() directly, which writes priv->fCacheOutdated from the validation/wallet thread (it is invoked synchronously from CWallet::BlockConnected at wallet.cpp:1486 while cs_main + cs_wallet are held). Meanwhile WalletModel::pollBalanceChanged on the Qt GUI thread reads the same fCacheOutdated via TransactionTableModel::updateConfirmations. There is no synchronization (the flag is a plain bool), which is a data race under the C++ memory model — formally UB, in practice usually benign on x86 but not portable.

Every other wallet→model notification in this file (NotifyTransactionChanged, NotifyAddressBookChanged, NotifyChainLockReceived in walletmodel.cpp) uses QMetaObject::invokeMethod(..., Qt::QueuedConnection) precisely to marshal the call onto the GUI thread. This new notifier must do the same (or fCacheOutdated must be std::atomic<bool>).

💡 Suggested change

static void NotifyBlockConnected(TransactionTableModel *ttm, CWallet * /*wallet*/, const std::vector<uint256>& /*txids*/)
{
    QMetaObject::invokeMethod(ttm, "invalidateWalletCache", Qt::QueuedConnection);
}

🔴 blocking: ChainLock status no longer refreshes until the next block

src/qt/transactiontablemodel.cpp (lines 316-319)

Before this PR, TransactionTablePriv::index() called rec->updateStatus() whenever statusUpdateNeeded() returned true (i.e. when cachedChainLockHeight had advanced past the row's cached value), and updateConfirmations() unconditionally re-emitted dataChanged every poll. That meant a ChainLock arrival caused affected rows to re-render on the next 250 ms poll.

After this PR, updateConfirmations() only refreshes when fCacheOutdated is true, and fCacheOutdated is set exclusively from NotifyBlockConnected. WalletModel::updateChainLockHeight() (walletmodel.cpp:198) forwards to TransactionTableModel::updateChainLockHeight() (transactiontablemodel.cpp:316), which only stores the int and never sets fCacheOutdated. As a result, when a ChainLock arrives without a new block connecting (the common case: a recently mined block transitioning to locked, or retroactive locks at the current height) the UI keeps showing rows as not-chainlocked until the next block ticks the cache. On mainnet this is up to ~2.5 min of stale lock state; in low-activity periods or tests it can be arbitrarily long.

The fix is to invalidate the cache whenever the ChainLock height advances.

💡 Suggested change

void TransactionTableModel::updateChainLockHeight(int chainLockHeight)
{
    cachedChainLockHeight = chainLockHeight;
    invalidateWalletCache();
}

🔴 blocking: Confirmation counts stop updating on block disconnects / reorgs

src/qt/walletmodel.cpp (lines 141-152)

pollBalanceChanged() (walletmodel.cpp:141) still triggers transactionTableModel->updateConfirmations() whenever chainActive.Height() changes, but updateConfirmations() (transactiontablemodel.cpp:303-313) is now a no-op unless fCacheOutdated was set. The flag is only set from NotifyBlockConnected; CWallet::BlockDisconnected() (wallet.cpp:1489) does not emit any matching invalidation. On a disconnect/reorg, transactions not explicitly touched by SyncTransaction() will keep showing the old (now-incorrect) confirmation depth until some later connected block happens to invalidate the cache.

The simplest fix is to also invalidate the cache when chainActive.Height() decreases (or unconditionally on any height change) in pollBalanceChanged, or to add a parallel NotifyBlockDisconnected signal.

💡 Suggested change

    if(fForceCheckBalanceChanged || chainActive.Height() != cachedNumBlocks || privateSendClient.nPrivateSendRounds != cachedPrivateSendRounds)
    {
        const bool block_height_changed = chainActive.Height() != cachedNumBlocks;
        fForceCheckBalanceChanged = false;

        // Balance and number of transactions might have changed
        cachedNumBlocks = chainActive.Height();
        cachedPrivateSendRounds = privateSendClient.nPrivateSendRounds;

        checkBalanceChanged();
        if (transactionTableModel) {
            if (block_height_changed) {
                transactionTableModel->invalidateWalletCache();
            }
            transactionTableModel->updateConfirmations();
        }
    }

🟡 suggestion: Per-block txids vector is built but never used

src/wallet/wallet.cpp (lines 1481-1486)

BlockConnected (wallet.cpp:1481-1486) allocates a std::vector<uint256> and computes tx->GetHash() for every transaction in the block, all while holding cs_main + cs_wallet. The signal's only subscriber is NotifyBlockConnected in transactiontablemodel.cpp:842, which takes the parameter as const std::vector<uint256>& /*txids*/ and discards it. Each GetHash() is a SHA256d, so this is non-trivial work serialized under both locks for nothing.

Either drop the txids argument from the signal entirely (the only subscriber just needs a 'block connected' edge), or pass pblock by const-ref so a future subscriber that actually wants the hashes can compute them lazily.

🟡 suggestion: LOCK2 inside switch case without braces; eager lock acquisition on GUI thread

src/qt/transactiontablemodel.cpp (lines 187-200)

Two issues with the CT_UPDATED branch (transactiontablemodel.cpp:187-200):

1. LOCK2(cs_main, wallet->cs_wallet); is declared directly under case CT_UPDATED: with no surrounding { }. This compiles today only because CT_UPDATED is the last case label; adding another label later in the switch would yield a 'jump to case label crosses initialization' compile error, or worse, leak the lock variable into another case. Wrap the case body in braces.

2. The previous CT_UPDATED handler was a cheap rec->status.needsUpdate = true; flag flip with the actual updateStatus() (and the lock) happening lazily inside index() on the GUI thread. This handler is invoked via QMetaObject::invokeMethod(..., Qt::QueuedConnection), so it now performs a blocking LOCK2(cs_main, cs_wallet) on the GUI thread — exactly the problem this PR is trying to fix elsewhere. During a rescan this can stall the UI.

🟡 suggestion: Tab indentation introduced; mixed with spaces

src/qt/transactiontablemodel.cpp (lines 86-92)

Multiple new hunks introduce tab indentation in a file that uses 4-space indentation throughout (e.g. lines 86-92, 167-171, 190-198, 309-311 of transactiontablemodel.cpp, and lines 150-151 of walletmodel.cpp). The latest commit is titled 'Style fixes' but tabs remain. The walletmodel.cpp hunk in particular is a pure whitespace change (4 spaces → tab + space) with no functional effect — drop it. Run clang-format on the changed files.

🟡 suggestion: fCacheOutdated initialized to true causes redundant refresh on first poll

src/qt/transactiontablemodel.cpp (lines 73-74)

refreshWallet() is called from the TransactionTableModel constructor and already runs updateStatus() for every cached record under cs_main + cs_wallet. Because fCacheOutdated is initialized to true (line 74), the first pollBalanceChanged() after startup will call updateWalletCache() and walk every record again under the same locks — duplicate work for large wallets. Initialize the flag to false and reset it to false at the end of refreshWallet().

💡 Suggested change

    QList<TransactionRecord> cachedWallet;
    bool fCacheOutdated = false;

🟡 suggestion: No tests added for the new caching/notification contract

src/qt/transactiontablemodel.cpp (lines 78-95)

This PR changes a wallet/UI contract: a new wallet signal (NotifyBlockConnected payload), a new cache-invalidation slot, removal of lazy refresh in index(), and eager refresh in CT_NEW / CT_UPDATED. No functional tests (test/functional/) or unit tests (src/test/wallet_tests.cpp) were added or modified. At minimum a unit test that the new NotifyBlockConnected signal fires with the expected payload, and a test that ChainLock-only updates eventually reach the model, would be valuable given the regressions flagged above.

💬 nitpick: Trailing whitespace introduced after // clear

src/qt/transactiontablemodel.cpp (line 874)

An unrelated trailing tab character was appended to the // clear comment on this line — drop it.

💡 Suggested change

        std::vector<TransactionNotification >().swap(vQueueNotifications); // clear

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/qt/transactiontablemodel.cpp`:
- [BLOCKING] lines 842-845: NotifyBlockConnected mutates the model from the wallet thread without QueuedConnection
  `NotifyBlockConnected` (transactiontablemodel.cpp:842-845) calls `ttm->invalidateWalletCache()` directly, which writes `priv->fCacheOutdated` from the validation/wallet thread (it is invoked synchronously from `CWallet::BlockConnected` at wallet.cpp:1486 while `cs_main` + `cs_wallet` are held). Meanwhile `WalletModel::pollBalanceChanged` on the Qt GUI thread reads the same `fCacheOutdated` via `TransactionTableModel::updateConfirmations`. There is no synchronization (the flag is a plain `bool`), which is a data race under the C++ memory model — formally UB, in practice usually benign on x86 but not portable.
  
  Every other wallet→model notification in this file (`NotifyTransactionChanged`, `NotifyAddressBookChanged`, `NotifyChainLockReceived` in walletmodel.cpp) uses `QMetaObject::invokeMethod(..., Qt::QueuedConnection)` precisely to marshal the call onto the GUI thread. This new notifier must do the same (or `fCacheOutdated` must be `std::atomic<bool>`).
- [BLOCKING] lines 316-319: ChainLock status no longer refreshes until the next block
  Before this PR, `TransactionTablePriv::index()` called `rec->updateStatus()` whenever `statusUpdateNeeded()` returned true (i.e. when `cachedChainLockHeight` had advanced past the row's cached value), and `updateConfirmations()` unconditionally re-emitted `dataChanged` every poll. That meant a ChainLock arrival caused affected rows to re-render on the next 250 ms poll.
  
  After this PR, `updateConfirmations()` only refreshes when `fCacheOutdated` is true, and `fCacheOutdated` is set exclusively from `NotifyBlockConnected`. `WalletModel::updateChainLockHeight()` (walletmodel.cpp:198) forwards to `TransactionTableModel::updateChainLockHeight()` (transactiontablemodel.cpp:316), which only stores the int and never sets `fCacheOutdated`. As a result, when a ChainLock arrives without a new block connecting (the common case: a recently mined block transitioning to locked, or retroactive locks at the current height) the UI keeps showing rows as not-chainlocked until the next block ticks the cache. On mainnet this is up to ~2.5 min of stale lock state; in low-activity periods or tests it can be arbitrarily long.
  
  The fix is to invalidate the cache whenever the ChainLock height advances.
- [SUGGESTION] lines 187-200: LOCK2 inside switch case without braces; eager lock acquisition on GUI thread
  Two issues with the `CT_UPDATED` branch (transactiontablemodel.cpp:187-200):
  
  1. `LOCK2(cs_main, wallet->cs_wallet);` is declared directly under `case CT_UPDATED:` with no surrounding `{ }`. This compiles today only because `CT_UPDATED` is the last case label; adding another label later in the switch would yield a 'jump to case label crosses initialization' compile error, or worse, leak the lock variable into another case. Wrap the case body in braces.
  
  2. The previous `CT_UPDATED` handler was a cheap `rec->status.needsUpdate = true;` flag flip with the actual `updateStatus()` (and the lock) happening lazily inside `index()` on the GUI thread. This handler is invoked via `QMetaObject::invokeMethod(..., Qt::QueuedConnection)`, so it now performs a blocking `LOCK2(cs_main, cs_wallet)` on the GUI thread — exactly the problem this PR is trying to fix elsewhere. During a rescan this can stall the UI.
- [SUGGESTION] lines 86-92: Tab indentation introduced; mixed with spaces
  Multiple new hunks introduce tab indentation in a file that uses 4-space indentation throughout (e.g. lines 86-92, 167-171, 190-198, 309-311 of transactiontablemodel.cpp, and lines 150-151 of walletmodel.cpp). The latest commit is titled 'Style fixes' but tabs remain. The walletmodel.cpp hunk in particular is a pure whitespace change (4 spaces → tab + space) with no functional effect — drop it. Run `clang-format` on the changed files.
- [SUGGESTION] lines 73-74: fCacheOutdated initialized to true causes redundant refresh on first poll
  `refreshWallet()` is called from the `TransactionTableModel` constructor and already runs `updateStatus()` for every cached record under `cs_main` + `cs_wallet`. Because `fCacheOutdated` is initialized to `true` (line 74), the first `pollBalanceChanged()` after startup will call `updateWalletCache()` and walk every record again under the same locks — duplicate work for large wallets. Initialize the flag to `false` and reset it to `false` at the end of `refreshWallet()`.
- [SUGGESTION] lines 78-95: No tests added for the new caching/notification contract
  This PR changes a wallet/UI contract: a new wallet signal (`NotifyBlockConnected` payload), a new cache-invalidation slot, removal of lazy refresh in `index()`, and eager refresh in `CT_NEW` / `CT_UPDATED`. No functional tests (`test/functional/`) or unit tests (`src/test/wallet_tests.cpp`) were added or modified. At minimum a unit test that the new `NotifyBlockConnected` signal fires with the expected payload, and a test that ChainLock-only updates eventually reach the model, would be valuable given the regressions flagged above.

In `src/qt/walletmodel.cpp`:
- [BLOCKING] lines 141-152: Confirmation counts stop updating on block disconnects / reorgs
  `pollBalanceChanged()` (walletmodel.cpp:141) still triggers `transactionTableModel->updateConfirmations()` whenever `chainActive.Height()` changes, but `updateConfirmations()` (transactiontablemodel.cpp:303-313) is now a no-op unless `fCacheOutdated` was set. The flag is only set from `NotifyBlockConnected`; `CWallet::BlockDisconnected()` (wallet.cpp:1489) does not emit any matching invalidation. On a disconnect/reorg, transactions not explicitly touched by `SyncTransaction()` will keep showing the old (now-incorrect) confirmation depth until some later connected block happens to invalidate the cache.
  
  The simplest fix is to also invalidate the cache when `chainActive.Height()` decreases (or unconditionally on any height change) in `pollBalanceChanged`, or to add a parallel `NotifyBlockDisconnected` signal.

In `src/wallet/wallet.cpp`:
- [SUGGESTION] lines 1481-1486: Per-block txids vector is built but never used
  `BlockConnected` (wallet.cpp:1481-1486) allocates a `std::vector<uint256>` and computes `tx->GetHash()` for every transaction in the block, all while holding `cs_main` + `cs_wallet`. The signal's only subscriber is `NotifyBlockConnected` in transactiontablemodel.cpp:842, which takes the parameter as `const std::vector<uint256>& /*txids*/` and discards it. Each `GetHash()` is a SHA256d, so this is non-trivial work serialized under both locks for nothing.
  
  Either drop the `txids` argument from the signal entirely (the only subscriber just needs a 'block connected' edge), or pass `pblock` by const-ref so a future subscriber that actually wants the hashes can compute them lazily.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.