Skip to content

Commit

Permalink
Fix largest part of GUI lockups with large wallets (#3155)
Browse files Browse the repository at this point in the history 
* [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
  • Loading branch information
@codablock @UdjinM6
codablock authored and UdjinM6 committed Oct 19, 2019
1 parent 3c6b5f9 commit da2f503
Show file tree
Hide file tree
Showing 11 changed files with 155 additions and 66 deletions.
14 changes: 12 additions & 2 deletions src/qt/addresstablemodel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions src/qt/addresstablemodel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#ifndef BITCOIN_QT_ADDRESSTABLEMODEL_H
#define BITCOIN_QT_ADDRESSTABLEMODEL_H

#include "base58.h"

#include <QAbstractTableModel>
#include <QStringList>

Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions src/qt/transactiondesc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) + ")";
Expand Down
10 changes: 5 additions & 5 deletions src/qt/transactionfilterproxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand 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();
Expand Down Expand Up @@ -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();
}

Expand Down
4 changes: 2 additions & 2 deletions src/qt/transactionfilterproxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
60 changes: 44 additions & 16 deletions src/qt/transactionrecord.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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")
{
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
Expand All @@ -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);
}
}
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -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
Expand Down
36 changes: 25 additions & 11 deletions src/qt/transactionrecord.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "amount.h"
#include "uint256.h"
#include "base58.h"

#include <QList>
#include <QString>
Expand All @@ -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 {
Expand All @@ -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
@{*/
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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;
/**@}*/
Expand All @@ -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
Loading

0 comments on commit da2f503

Please sign in to comment.