Fix largest part of GUI lockups with large wallets (#3155)

* [Qt] make sure transaction table entry gets updated after bump

* Remove unnecessary tracking of IS lock count

* Track lockedByChainLocks in TransactionRecord

* Only update record when the TX was not ChainLocked before

* Emit dataChanged for CT_UPDATED transactions

* Use plain seconds since epoch comparison in TransactionFilterProxy::filterAcceptsRow

The QDateTime::operator< calls inside TransactionFilterProxy::filterAcceptsRow
turned out to be the slowest part in the UI when many TXs are inside
the wallet. DateRoleInt allows us to request the plain seconds since epoch
which we then use to compare against dateFrom/dateTo, which are also both
stored as seconds since epoch now.

* Don't invoke updateConfirmations directly and let pollBalanceChanged handle it

* Implement AddressTableModel::labelForDestination

This one avoids converting from string to CBitcoinAddress and calling
.Get() on the result.

* Also store CBitcoinAddress object and CTxDestination in TransactionRecord

This avoids frequent and slow conversion

* Use labelForDestination when possible

This avoids unnecessary conversions

* Don't set fForceCheckBalanceChanged to true when IS lock is received

We already do this through updateTransaction(), which is also called when
an IS lock is received for one of our own TXs.

* Only update lockedByChainLocks and lockedByInstantSend when a change is possible

lockedByChainLocks can never get back to false, so no need to re-check it.
Same with lockedByInstantSend, except when a ChainLock overrides it.

* Hold and update label in TransactionRecord

Instead of looking it up in data()

* Review suggestions

* Use proper columns in dataChanged call in updateAddressBook

Author: codablock

src/qt/addresstablemodel.cpp

@@ -421,11 +421,21 @@ bool AddressTableModel::removeRows(int row, int count, const QModelIndex &parent
 /* Look up label for address in address book, if not found return empty string.
  */
 QString AddressTableModel::labelForAddress(const QString &address) const
+{
+    CBitcoinAddress address_parsed(address.toStdString());
+    return labelForAddress(address_parsed);
+}
+
+QString AddressTableModel::labelForAddress(const CBitcoinAddress &address) const
+{
+    return labelForDestination(address.Get());
+}
+
+QString AddressTableModel::labelForDestination(const CTxDestination &dest) const
 {
     {
         LOCK(wallet->cs_wallet);
-        CBitcoinAddress address_parsed(address.toStdString());
-        std::map<CTxDestination, CAddressBookData>::iterator mi = wallet->mapAddressBook.find(address_parsed.Get());
+        std::map<CTxDestination, CAddressBookData>::iterator mi = wallet->mapAddressBook.find(dest);
         if (mi != wallet->mapAddressBook.end())
         {
             return QString::fromStdString(mi->second.name);

src/qt/addresstablemodel.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_QT_ADDRESSTABLEMODEL_H
 #define BITCOIN_QT_ADDRESSTABLEMODEL_H
 
+#include "base58.h"
+
 #include <QAbstractTableModel>
 #include <QStringList>
 
@@ -66,6 +68,8 @@ class AddressTableModel : public QAbstractTableModel
     /* Look up label for address in address book, if not found return empty string.
      */
     QString labelForAddress(const QString &address) const;
+    QString labelForAddress(const CBitcoinAddress &address) const;
+    QString labelForDestination(const CTxDestination &dest) const;
 
     /* Look up row index of an address in the model.
        Return -1 if not found.

src/qt/transactiondesc.cpp

@@ -107,14 +107,14 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, TransactionReco
         if (nNet > 0)
         {
             // Credit
-            if (CBitcoinAddress(rec->address).IsValid())
+            if (rec->address.IsValid())
             {
-                CTxDestination address = CBitcoinAddress(rec->address).Get();
+                CTxDestination address = rec->txDest;
                 if (wallet->mapAddressBook.count(address))
                 {
                     strHTML += "<b>" + tr("From") + ":</b> " + tr("unknown") + "<br>";
                     strHTML += "<b>" + tr("To") + ":</b> ";
-                    strHTML += GUIUtil::HtmlEscape(rec->address);
+                    strHTML += GUIUtil::HtmlEscape(rec->strAddress);
                     QString addressOwned = (::IsMine(*wallet, address) == ISMINE_SPENDABLE) ? tr("own address") : tr("watch-only");
                     if (!wallet->mapAddressBook[address].name.empty())
                         strHTML += " (" + addressOwned + ", " + tr("label") + ": " + GUIUtil::HtmlEscape(wallet->mapAddressBook[address].name) + ")";

src/qt/transactionfilterproxy.cpp

@@ -18,8 +18,8 @@ const QDateTime TransactionFilterProxy::MAX_DATE = QDateTime::fromTime_t(0xFFFFF
 
 TransactionFilterProxy::TransactionFilterProxy(QObject *parent) :
     QSortFilterProxyModel(parent),
-    dateFrom(MIN_DATE),
-    dateTo(MAX_DATE),
+    dateFrom(MIN_DATE.toTime_t()),
+    dateTo(MAX_DATE.toTime_t()),
     addrPrefix(),
     typeFilter(COMMON_TYPES),
     watchOnlyFilter(WatchOnlyFilter_All),
@@ -35,7 +35,7 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
     QModelIndex index = sourceModel()->index(sourceRow, 0, sourceParent);
 
     int type = index.data(TransactionTableModel::TypeRole).toInt();
-    QDateTime datetime = index.data(TransactionTableModel::DateRole).toDateTime();
+    qint64 datetime = index.data(TransactionTableModel::DateRoleInt).toLongLong();
     bool involvesWatchAddress = index.data(TransactionTableModel::WatchonlyRole).toBool();
     bool lockedByInstantSend = index.data(TransactionTableModel::InstantSendRole).toBool();
     QString address = index.data(TransactionTableModel::AddressRole).toString();
@@ -67,8 +67,8 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
 
 void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime &to)
 {
-    this->dateFrom = from;
-    this->dateTo = to;
+    this->dateFrom = from.toTime_t();
+    this->dateTo = to.toTime_t();
     invalidateFilter();
 }
 

src/qt/transactionfilterproxy.h

@@ -65,8 +65,8 @@ class TransactionFilterProxy : public QSortFilterProxyModel
     bool filterAcceptsRow(int source_row, const QModelIndex & source_parent) const;
 
 private:
-    QDateTime dateFrom;
-    QDateTime dateTo;
+    qint64 dateFrom;
+    qint64 dateTo;
     QString addrPrefix;
     quint32 typeFilter;
     WatchOnlyFilter watchOnlyFilter;

src/qt/transactionrecord.cpp

@@ -58,20 +58,22 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
                 {
                     // Received by Dash Address
                     sub.type = TransactionRecord::RecvWithAddress;
-                    sub.address = CBitcoinAddress(address).ToString();
+                    sub.strAddress = CBitcoinAddress(address).ToString();
                 }
                 else
                 {
                     // Received by IP connection (deprecated features), or a multisignature or other non-simple transaction
                     sub.type = TransactionRecord::RecvFromOther;
-                    sub.address = mapValue["from"];
+                    sub.strAddress = mapValue["from"];
                 }
                 if (wtx.IsCoinBase())
                 {
                     // Generated
                     sub.type = TransactionRecord::Generated;
                 }
 
+                sub.address.SetString(sub.strAddress);
+                sub.txDest = sub.address.Get();
                 parts.append(sub);
             }
         }
@@ -119,7 +121,7 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
             TransactionRecord sub(hash, nTime);
             // Payment to self by default
             sub.type = TransactionRecord::SendToSelf;
-            sub.address = "";
+            sub.strAddress = "";
 
             if(mapValue["DS"] == "1")
             {
@@ -128,12 +130,12 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
                 if (ExtractDestination(wtx.tx->vout[0].scriptPubKey, address))
                 {
                     // Sent to Dash Address
-                    sub.address = CBitcoinAddress(address).ToString();
+                    sub.strAddress = CBitcoinAddress(address).ToString();
                 }
                 else
                 {
                     // Sent to IP, or other non-address transaction like OP_EVAL
-                    sub.address = mapValue["to"];
+                    sub.strAddress = mapValue["to"];
                 }
             }
             else
@@ -162,6 +164,8 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
 
             sub.debit = -(nDebit - nChange);
             sub.credit = nCredit - nChange;
+            sub.address.SetString(sub.strAddress);
+            sub.txDest = sub.address.Get();
             parts.append(sub);
             parts.last().involvesWatchAddress = involvesWatchAddress;   // maybe pass to TransactionRecord as constructor argument
         }
@@ -205,13 +209,13 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
                 {
                     // Sent to Dash Address
                     sub.type = TransactionRecord::SendToAddress;
-                    sub.address = CBitcoinAddress(address).ToString();
+                    sub.strAddress = CBitcoinAddress(address).ToString();
                 }
                 else
                 {
                     // Sent to IP, or other non-address transaction like OP_EVAL
                     sub.type = TransactionRecord::SendToOther;
-                    sub.address = mapValue["to"];
+                    sub.strAddress = mapValue["to"];
                 }
 
                 if(mapValue["DS"] == "1")
@@ -228,6 +232,9 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
                 }
                 sub.debit = -nValue;
 
+                sub.address.SetString(sub.strAddress);
+                sub.txDest = sub.address.Get();
+
                 parts.append(sub);
             }
         }
@@ -244,9 +251,10 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
     return parts;
 }
 
-void TransactionRecord::updateStatus(const CWalletTx &wtx, int numISLocks, int chainLockHeight)
+void TransactionRecord::updateStatus(const CWalletTx &wtx, int chainLockHeight)
 {
     AssertLockHeld(cs_main);
+    AssertLockHeld(wtx.GetWallet()->cs_wallet);
     // Determine transaction status
 
     // Find the block the tx is in
@@ -264,9 +272,20 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int numISLocks, int c
     status.countsForBalance = wtx.IsTrusted() && !(wtx.GetBlocksToMaturity() > 0);
     status.depth = wtx.GetDepthInMainChain();
     status.cur_num_blocks = chainActive.Height();
-    status.cachedNumISLocks = numISLocks;
     status.cachedChainLockHeight = chainLockHeight;
 
+    bool oldLockedByChainLocks = status.lockedByChainLocks;
+    if (!status.lockedByChainLocks) {
+        status.lockedByChainLocks = wtx.IsChainLocked();
+    }
+
+    auto addrBookIt = wtx.GetWallet()->mapAddressBook.find(this->txDest);
+    if (addrBookIt == wtx.GetWallet()->mapAddressBook.end()) {
+        status.label = "";
+    } else {
+        status.label = QString::fromStdString(addrBookIt->second.name);
+    }
+
     if (!CheckFinalTx(wtx))
     {
         if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)
@@ -307,7 +326,17 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int numISLocks, int c
     }
     else
     {
-        status.lockedByInstantSend = wtx.IsLockedByInstantSend();
+        // The IsLockedByInstantSend call is quite expensive, so we only do it when a state change is actually possible.
+        if (status.lockedByChainLocks) {
+            if (oldLockedByChainLocks != status.lockedByChainLocks) {
+                status.lockedByInstantSend = wtx.IsLockedByInstantSend();
+            } else {
+                status.lockedByInstantSend = false;
+            }
+        } else if (!status.lockedByInstantSend) {
+            status.lockedByInstantSend = wtx.IsLockedByInstantSend();
+        }
+
         if (status.depth < 0)
         {
             status.status = TransactionStatus::Conflicted;
@@ -322,7 +351,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int numISLocks, int c
             if (wtx.isAbandoned())
                 status.status = TransactionStatus::Abandoned;
         }
-        else if (status.depth < RecommendedNumConfirmations && !wtx.IsChainLocked())
+        else if (status.depth < RecommendedNumConfirmations && !status.lockedByChainLocks)
         {
             status.status = TransactionStatus::Confirming;
         }
@@ -331,15 +360,14 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int numISLocks, int c
             status.status = TransactionStatus::Confirmed;
         }
     }
-
+    status.needsUpdate = false;
 }
 
-bool TransactionRecord::statusUpdateNeeded(int numISLocks, int chainLockHeight)
+bool TransactionRecord::statusUpdateNeeded(int chainLockHeight)
 {
     AssertLockHeld(cs_main);
-    return status.cur_num_blocks != chainActive.Height()
-        || status.cachedNumISLocks != numISLocks
-        || status.cachedChainLockHeight != chainLockHeight;
+    return status.cur_num_blocks != chainActive.Height() || status.needsUpdate
+        || (!status.lockedByChainLocks && status.cachedChainLockHeight != chainLockHeight);
 }
 
 QString TransactionRecord::getTxID() const

src/qt/transactionrecord.h

@@ -7,6 +7,7 @@
 
 #include "amount.h"
 #include "uint256.h"
+#include "base58.h"
 
 #include <QList>
 #include <QString>
@@ -20,9 +21,9 @@ class TransactionStatus
 {
 public:
     TransactionStatus():
-        countsForBalance(false), lockedByInstantSend(false), sortKey(""),
+        countsForBalance(false), lockedByInstantSend(false), lockedByChainLocks(false), sortKey(""),
         matures_in(0), status(Offline), depth(0), open_for(0), cur_num_blocks(-1),
-        cachedNumISLocks(-1), cachedChainLockHeight(-1)
+        cachedChainLockHeight(-1), needsUpdate(false)
     { }
 
     enum Status {
@@ -45,8 +46,12 @@ class TransactionStatus
     bool countsForBalance;
     /// Transaction was locked via InstantSend
     bool lockedByInstantSend;
+    /// Transaction was locked via ChainLocks
+    bool lockedByChainLocks;
     /// Sorting key based on status
     std::string sortKey;
+    /// Label
+    QString label;
 
     /** @name Generated (mined) transactions
        @{*/
@@ -65,10 +70,10 @@ class TransactionStatus
     /** Current number of blocks (to know whether cached status is still valid) */
     int cur_num_blocks;
 
-    //** Know when to update transaction for IS-locks **/
-    int cachedNumISLocks;
     //** Know when to update transaction for chainlocks **/
     int cachedChainLockHeight;
+
+    bool needsUpdate;
 };
 
 /** UI model for a transaction. A core transaction can be represented by multiple UI transactions if it has
@@ -98,22 +103,28 @@ class TransactionRecord
     static const int RecommendedNumConfirmations = 6;
 
     TransactionRecord():
-            hash(), time(0), type(Other), address(""), debit(0), credit(0), idx(0)
+            hash(), time(0), type(Other), strAddress(""), debit(0), credit(0), idx(0)
     {
+        address = CBitcoinAddress(strAddress);
+        txDest = address.Get();
     }
 
     TransactionRecord(uint256 _hash, qint64 _time):
-            hash(_hash), time(_time), type(Other), address(""), debit(0),
+            hash(_hash), time(_time), type(Other), strAddress(""), debit(0),
             credit(0), idx(0)
     {
+        address = CBitcoinAddress(strAddress);
+        txDest = address.Get();
     }
 
     TransactionRecord(uint256 _hash, qint64 _time,
-                Type _type, const std::string &_address,
+                Type _type, const std::string &_strAddress,
                 const CAmount& _debit, const CAmount& _credit):
-            hash(_hash), time(_time), type(_type), address(_address), debit(_debit), credit(_credit),
+            hash(_hash), time(_time), type(_type), strAddress(_strAddress), debit(_debit), credit(_credit),
             idx(0)
     {
+        address = CBitcoinAddress(strAddress);
+        txDest = address.Get();
     }
 
     /** Decompose CWallet transaction to model transaction records.
@@ -126,7 +137,10 @@ class TransactionRecord
     uint256 hash;
     qint64 time;
     Type type;
-    std::string address;
+    std::string strAddress;
+    CBitcoinAddress address;
+    CTxDestination txDest;
+
     CAmount debit;
     CAmount credit;
     /**@}*/
@@ -148,11 +162,11 @@ class TransactionRecord
 
     /** Update status from core wallet tx.
      */
-    void updateStatus(const CWalletTx &wtx, int numISLocks, int chainLockHeight);
+    void updateStatus(const CWalletTx &wtx, int chainLockHeight);
 
     /** Return whether a status update is needed.
      */
-    bool statusUpdateNeeded(int numISLocks, int chainLockHeight);
+    bool statusUpdateNeeded(int chainLockHeight);
 };
 
 #endif // BITCOIN_QT_TRANSACTIONRECORD_H