From e62958dc81d215a1c56318d0914dfd9a33d45973 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 7 May 2022 10:54:11 -0300 Subject: [PATCH 1/5] GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call Inside setModel, we call 'wallet().getBalances()', to set the view balance, right before calling 'updateDisplayUnit' which calls 'wallet().getBalances()' internally and re-sets the view balance again. --- src/qt/sendcoinsdialog.cpp | 8 +++----- src/qt/sendcoinsdialog.h | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index fd8eccb86d..91d2233ff5 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -164,11 +164,9 @@ void SendCoinsDialog::setModel(WalletModel *_model) } } - interfaces::WalletBalances balances = _model->wallet().getBalances(); - setBalance(balances); connect(_model, &WalletModel::balanceChanged, this, &SendCoinsDialog::setBalance); - connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::updateDisplayUnit); - updateDisplayUnit(); + connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::refreshBalance); + refreshBalance(); // Coin Control connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::coinControlUpdateLabels); @@ -718,7 +716,7 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances) } } -void SendCoinsDialog::updateDisplayUnit() +void SendCoinsDialog::refreshBalance() { setBalance(model->wallet().getBalances()); ui->customFee->setDisplayUnit(model->getOptionsModel()->getDisplayUnit()); diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 400503d0c0..b58d4690a0 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -97,7 +97,7 @@ private Q_SLOTS: void on_buttonMinimizeFee_clicked(); void removeEntry(SendCoinsEntry* entry); void useAvailableBalance(SendCoinsEntry* entry); - void updateDisplayUnit(); + void refreshBalance(); void coinControlFeatureChanged(bool); void coinControlButtonClicked(); void coinControlChangeChecked(int); From 321335bf0292034d79afa6c44f7f072942b6cc3c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 9 May 2022 11:24:20 -0300 Subject: [PATCH 2/5] GUI: add getter for WalletModel::m_cached_balances field No need to guard it as it is/will only be accessed from the main thread for now --- src/qt/walletmodel.cpp | 7 ++++++- src/qt/walletmodel.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 5ee32e79d5..2adf1a11f1 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -120,12 +120,17 @@ void WalletModel::pollBalanceChanged() void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances) { - if(new_balances.balanceChanged(m_cached_balances)) { + if (new_balances.balanceChanged(m_cached_balances)) { m_cached_balances = new_balances; Q_EMIT balanceChanged(new_balances); } } +interfaces::WalletBalances WalletModel::getCachedBalance() const +{ + return m_cached_balances; +} + void WalletModel::updateTransaction() { // Balance and number of transactions might have changed diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index ad1239ccdc..18951c2858 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -157,6 +157,9 @@ public: uint256 getLastBlockProcessed() const; + // Retrieve the cached wallet balance + interfaces::WalletBalances getCachedBalance() const; + private: std::unique_ptr m_wallet; std::unique_ptr m_handler_unload; From 96e3264a82c51b456703f500bd98e8cb98115697 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 9 May 2022 10:59:57 -0300 Subject: [PATCH 3/5] GUI: use cached balance in overviewpage and sendcoinsdialog Plus, calculate the cached balance right when the wallet model, so the wallet widgets don't need to redo the same balance calculation multiple times when they are waiting for the model balance polling timer. ---------------------------------------------------------------------- test wise: `WalletTests` now need to trigger the walletModel balance changed manually. So the model updates its internal state and can be used by the widgets. This is because the test does not start the balance polling timer, in the same way as does not initialize several parts of the GUI workflow. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the `WalletController` class flow. Rationale is that this unit test is focused on verifying the GUI widgets/views behavior only: update the presented information, etc. when they receive different signals and/or function calls from outside (in other words, focus is on the signal slots/receiver side). It's not about whether the wallet balance polling timer is functioning as expected or not (which we definitely create a new test case for it in a follow-up work). --- src/qt/overviewpage.cpp | 7 +++---- src/qt/sendcoinsdialog.cpp | 2 +- src/qt/test/wallettests.cpp | 28 +++++++++++++--------------- src/qt/walletmodel.cpp | 4 ++++ 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 820bcbf3cd..900985a108 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -277,14 +277,13 @@ void OverviewPage::setWalletModel(WalletModel *model) ui->listTransactions->setModelColumn(TransactionTableModel::ToAddress); // Keep up to date with wallet - interfaces::Wallet& wallet = model->wallet(); - interfaces::WalletBalances balances = wallet.getBalances(); - setBalance(balances); + setBalance(model->getCachedBalance()); connect(model, &WalletModel::balanceChanged, this, &OverviewPage::setBalance); connect(model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &OverviewPage::updateDisplayUnit); - updateWatchOnlyLabels(wallet.haveWatchOnly() && !model->wallet().privateKeysDisabled()); + interfaces::Wallet& wallet = model->wallet(); + updateWatchOnlyLabels(wallet.haveWatchOnly() && !wallet.privateKeysDisabled()); connect(model, &WalletModel::notifyWatchonlyChanged, [this](bool showWatchOnly) { updateWatchOnlyLabels(showWatchOnly && !walletModel->wallet().privateKeysDisabled()); }); diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 91d2233ff5..8960045d25 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -718,7 +718,7 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances) void SendCoinsDialog::refreshBalance() { - setBalance(model->wallet().getBalances()); + setBalance(model->getCachedBalance()); ui->customFee->setDisplayUnit(model->getOptionsModel()->getDisplayUnit()); updateSmartFeeLabel(); } diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index c4cd0f4cd1..04eeea0eb9 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -129,6 +129,13 @@ void BumpFee(TransactionView& view, const uint256& txid, bool expectDisabled, st QVERIFY(text.indexOf(QString::fromStdString(expectError)) != -1); } +void CompareBalance(WalletModel& walletModel, CAmount expected_balance, QLabel* balance_label_to_check) +{ + BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); + QString balanceComparison = BitcoinUnits::formatWithUnit(unit, expected_balance, false, BitcoinUnits::SeparatorStyle::ALWAYS); + QCOMPARE(balance_label_to_check->text().trimmed(), balanceComparison); +} + //! Simple qt wallet tests. // // Test widgets can be debugged interactively calling show() on them and @@ -193,15 +200,10 @@ void TestGUI(interfaces::Node& node) sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); - { - // Check balance in send dialog - QLabel* balanceLabel = sendCoinsDialog.findChild("labelBalance"); - QString balanceText = balanceLabel->text(); - BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); - CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false, BitcoinUnits::SeparatorStyle::ALWAYS); - QCOMPARE(balanceText, balanceComparison); - } + // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel. + walletModel.pollBalanceChanged(); + // Check balance in send dialog + CompareBalance(walletModel, walletModel.wallet().getBalance(), sendCoinsDialog.findChild("labelBalance")); // Send two transactions, and verify they are added to transaction list. TransactionTableModel* transactionTableModel = walletModel.getTransactionTableModel(); @@ -221,12 +223,8 @@ void TestGUI(interfaces::Node& node) // Check current balance on OverviewPage OverviewPage overviewPage(platformStyle.get()); overviewPage.setWalletModel(&walletModel); - QLabel* balanceLabel = overviewPage.findChild("labelBalance"); - QString balanceText = balanceLabel->text().trimmed(); - BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); - CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false, BitcoinUnits::SeparatorStyle::ALWAYS); - QCOMPARE(balanceText, balanceComparison); + walletModel.pollBalanceChanged(); // Manual balance polling update + CompareBalance(walletModel, walletModel.wallet().getBalance(), overviewPage.findChild("labelBalance")); // Check Request Payment button ReceiveCoinsDialog receiveCoinsDialog(platformStyle.get()); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 2adf1a11f1..a7f290ba92 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -67,6 +67,10 @@ WalletModel::~WalletModel() void WalletModel::startPollBalance() { + // Update the cached balance right away, so every view can make use of it, + // so them don't need to waste resources recalculating it. + pollBalanceChanged(); + // This timer will be fired repeatedly to update the balance // Since the QTimer::timeout is a private signal, it cannot be used // in the GUIUtil::ExceptionSafeConnect directly. From 050e8b139145d6991e740b0e5f2b3364663dd348 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 27 Apr 2022 11:26:15 -0300 Subject: [PATCH 4/5] GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually No need to walk through the entire wallet's tx map. Used for 'walletModel::prepareTransaction' and 'useAvailable' flow in sendcoinsdialog. --- src/qt/sendcoinsdialog.cpp | 2 +- src/qt/walletmodel.cpp | 9 ++++++++- src/qt/walletmodel.h | 4 ++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 8960045d25..5f6f35e9c0 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -795,7 +795,7 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry) m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); // Calculate available amount to send. - CAmount amount = model->wallet().getAvailableBalance(*m_coin_control); + CAmount amount = model->getAvailableBalance(m_coin_control.get()); for (int i = 0; i < ui->entries->count(); ++i) { SendCoinsEntry* e = qobject_cast(ui->entries->itemAt(i)->widget()); if (e && !e->isHidden() && e != entry) { diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index a7f290ba92..b5695cda7e 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -203,7 +203,9 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact return DuplicateAddress; } - CAmount nBalance = m_wallet->getAvailableBalance(coinControl); + // If no coin was manually selected, use the cached balance + // Future: can merge this call with 'createTransaction'. + CAmount nBalance = getAvailableBalance(&coinControl); if(total > nBalance) { @@ -608,3 +610,8 @@ uint256 WalletModel::getLastBlockProcessed() const { return m_client_model ? m_client_model->getBestBlockHash() : uint256{}; } + +CAmount WalletModel::getAvailableBalance(const CCoinControl* control) +{ + return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance; +} diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 18951c2858..afe93c43e0 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -160,6 +160,10 @@ public: // Retrieve the cached wallet balance interfaces::WalletBalances getCachedBalance() const; + // If coin control has selected outputs, searches the total amount inside the wallet. + // Otherwise, uses the wallet's cached available balance. + CAmount getAvailableBalance(const wallet::CCoinControl* control); + private: std::unique_ptr m_wallet; std::unique_ptr m_handler_unload; From 4584d300a40bfd84517072f7a6eee114fb7cab08 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 10 May 2022 09:43:16 -0300 Subject: [PATCH 5/5] GUI: remove now unneeded 'm_balances' field from overviewpage --- src/qt/overviewpage.cpp | 16 +++++++--------- src/qt/overviewpage.h | 1 - 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 900985a108..c09d1b05f8 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -148,8 +148,6 @@ OverviewPage::OverviewPage(const PlatformStyle *platformStyle, QWidget *parent) { ui->setupUi(this); - m_balances.balance = -1; - // use a SingleColorIcon for the "out of sync warning" icon QIcon icon = m_platform_style->SingleColorIcon(QStringLiteral(":/icons/warning")); ui->labelTransactionsStatus->setIcon(icon); @@ -178,8 +176,9 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index) void OverviewPage::setPrivacy(bool privacy) { m_privacy = privacy; - if (m_balances.balance != -1) { - setBalance(m_balances); + const auto& balances = walletModel->getCachedBalance(); + if (balances.balance != -1) { + setBalance(balances); } ui->listTransactions->setVisible(!m_privacy); @@ -198,7 +197,6 @@ OverviewPage::~OverviewPage() void OverviewPage::setBalance(const interfaces::WalletBalances& balances) { BitcoinUnit unit = walletModel->getOptionsModel()->getDisplayUnit(); - m_balances = balances; if (walletModel->wallet().isLegacy()) { if (walletModel->wallet().privateKeysDisabled()) { ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); @@ -306,10 +304,10 @@ void OverviewPage::changeEvent(QEvent* e) void OverviewPage::updateDisplayUnit() { - if(walletModel && walletModel->getOptionsModel()) - { - if (m_balances.balance != -1) { - setBalance(m_balances); + if (walletModel && walletModel->getOptionsModel()) { + const auto& balances = walletModel->getCachedBalance(); + if (balances.balance != -1) { + setBalance(balances); } // Update txdelegate->unit with the current unit diff --git a/src/qt/overviewpage.h b/src/qt/overviewpage.h index 058df1a8c5..56f45907db 100644 --- a/src/qt/overviewpage.h +++ b/src/qt/overviewpage.h @@ -52,7 +52,6 @@ private: Ui::OverviewPage *ui; ClientModel *clientModel; WalletModel *walletModel; - interfaces::WalletBalances m_balances; bool m_privacy{false}; const PlatformStyle* m_platform_style;