From 83f69fab3a1ae97c5cff8ba1e6fd191b0fa264bb Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 24 Apr 2020 17:40:33 -0400 Subject: [PATCH 1/4] Switch transaction table to use wallet height not node height Tweak of #17905 to make gui display of transactions and balances more consistent. This change shouldn't cause visible effects in normal cases, just make GUI wallet code more internally correct and consistent. --- src/qt/transactiontablemodel.cpp | 2 +- src/qt/walletmodel.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 18554aef1fe..7a155032284 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -664,7 +664,7 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex &parent) const { Q_UNUSED(parent); - TransactionRecord *data = priv->index(walletModel->wallet(), walletModel->clientModel().getNumBlocks(), row); + TransactionRecord *data = priv->index(walletModel->wallet(), walletModel->getNumBlocks(), row); if(data) { return createIndex(row, column, data); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 07004b7c6b8..5521565f7c7 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -144,7 +144,7 @@ public: interfaces::Node& node() const { return m_node; } interfaces::Wallet& wallet() const { return *m_wallet; } - ClientModel& clientModel() const { return m_client_model; } + int getNumBlocks() const { return cachedNumBlocks; } QString getWalletName() const; QString getDisplayName() const; From 2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 24 Apr 2020 18:06:26 -0400 Subject: [PATCH 2/4] Cancel wallet balance timer when shutdown requested This doesn't fix any current problem, but it makes balance checking code less fragile, and prevents use-after free travis error in next commit: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240 --- src/qt/walletmodel.cpp | 12 +++++++++--- src/qt/walletmodel.h | 4 +++- src/qt/walletview.cpp | 1 + 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index e7581cd64e2..e751f402cf7 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -38,14 +38,15 @@ WalletModel::WalletModel(std::unique_ptr wallet, ClientModel& client_model, const PlatformStyle *platformStyle, QObject *parent) : QObject(parent), m_wallet(std::move(wallet)), - m_client_model(client_model), + m_client_model(&client_model), m_node(client_model.node()), optionsModel(client_model.getOptionsModel()), addressTableModel(nullptr), transactionTableModel(nullptr), recentRequestsTableModel(nullptr), cachedEncryptionStatus(Unencrypted), - cachedNumBlocks(0) + cachedNumBlocks(0), + timer(new QTimer(this)) { fHaveWatchOnly = m_wallet->haveWatchOnly(); addressTableModel = new AddressTableModel(this); @@ -63,11 +64,16 @@ WalletModel::~WalletModel() void WalletModel::startPollBalance() { // This timer will be fired repeatedly to update the balance - QTimer* timer = new QTimer(this); connect(timer, &QTimer::timeout, this, &WalletModel::pollBalanceChanged); timer->start(MODEL_UPDATE_DELAY); } +void WalletModel::setClientModel(ClientModel* client_model) +{ + m_client_model = client_model; + if (!m_client_model) timer->stop(); +} + void WalletModel::updateStatus() { EncryptionStatus newEncryptionStatus = getEncryptionStatus(); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 5521565f7c7..23232ec66b2 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -144,6 +144,7 @@ public: interfaces::Node& node() const { return m_node; } interfaces::Wallet& wallet() const { return *m_wallet; } + void setClientModel(ClientModel* client_model); int getNumBlocks() const { return cachedNumBlocks; } QString getWalletName() const; @@ -161,7 +162,7 @@ private: std::unique_ptr m_handler_show_progress; std::unique_ptr m_handler_watch_only_changed; std::unique_ptr m_handler_can_get_addrs_changed; - ClientModel& m_client_model; + ClientModel* m_client_model; interfaces::Node& m_node; bool fHaveWatchOnly; @@ -179,6 +180,7 @@ private: interfaces::WalletBalances m_cached_balances; EncryptionStatus cachedEncryptionStatus; int cachedNumBlocks; + QTimer* timer; void subscribeToCoreSignals(); void unsubscribeFromCoreSignals(); diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 5d9b420df7a..c4e607990a2 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -97,6 +97,7 @@ void WalletView::setClientModel(ClientModel *_clientModel) overviewPage->setClientModel(_clientModel); sendCoinsPage->setClientModel(_clientModel); + if (walletModel) walletModel->setClientModel(_clientModel); } void WalletView::setWalletModel(WalletModel *_walletModel) From bf0a510981ddc28c754881ca21c50ab18e5f2b59 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 2 Apr 2020 07:42:01 -0400 Subject: [PATCH 3/4] gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications interfaces::Wallet::tryGetBalances was recently updated in https://github.com/bitcoin/bitcoin/pull/18160 to avoid computing balances internally, but this not efficient as it could be with #10102 because tryGetBalances is an interprocess call. Implementing the TransactionChanged / BlockTip check outside of tryGetBalances also allows tryGetBalances to be simplified in next commit 'Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged"'. --- src/qt/walletmodel.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index e751f402cf7..1789fefeadb 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -85,6 +85,10 @@ void WalletModel::updateStatus() void WalletModel::pollBalanceChanged() { + // Avoid recomputing wallet balances unless a TransactionChanged or + // BlockTip notification was received. + if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model->getNumBlocks()) return; + // 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 From d3a56be77a9d112cde4baef4314882170b9f228f Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 2 Apr 2020 07:55:14 -0400 Subject: [PATCH 4/4] Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" This reverts commit 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 from https://github.com/bitcoin/bitcoin/pull/18160 which no longer an optimization since commit "gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications". --- src/interfaces/wallet.cpp | 3 +-- src/interfaces/wallet.h | 7 ++----- src/qt/walletmodel.cpp | 17 ++++++++++------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 752448aac79..3521ca8e38e 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -351,14 +351,13 @@ public: } return result; } - bool tryGetBalances(WalletBalances& balances, int& num_blocks, bool force, int cached_num_blocks) override + bool tryGetBalances(WalletBalances& balances, int& num_blocks) override { TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; } num_blocks = m_wallet->GetLastBlockHeight(); - if (!force && num_blocks == cached_num_blocks) return false; balances = getBalances(); return true; } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index e5e3b806638..0a437e53b91 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -201,11 +201,8 @@ public: //! Get balances. virtual WalletBalances getBalances() = 0; - //! Get balances if possible without waiting for chain and wallet locks. - virtual bool tryGetBalances(WalletBalances& balances, - int& num_blocks, - bool force, - int cached_num_blocks) = 0; + //! Get balances if possible without blocking. + virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0; //! Get balance. virtual CAmount getBalance() = 0; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 1789fefeadb..28489cd2107 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -95,18 +95,21 @@ void WalletModel::pollBalanceChanged() // rescan. interfaces::WalletBalances new_balances; int numBlocks = -1; - if (!m_wallet->tryGetBalances(new_balances, numBlocks, fForceCheckBalanceChanged, cachedNumBlocks)) { + if (!m_wallet->tryGetBalances(new_balances, numBlocks)) { return; } - fForceCheckBalanceChanged = false; + if(fForceCheckBalanceChanged || numBlocks != cachedNumBlocks) + { + fForceCheckBalanceChanged = false; - // Balance and number of transactions might have changed - cachedNumBlocks = numBlocks; + // Balance and number of transactions might have changed + cachedNumBlocks = numBlocks; - checkBalanceChanged(new_balances); - if(transactionTableModel) - transactionTableModel->updateConfirmations(); + checkBalanceChanged(new_balances); + if(transactionTableModel) + transactionTableModel->updateConfirmations(); + } } void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances)