Skip to content

Avoid recalculating the wallet balance - use model cache #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 15, 2022
Merged

Avoid recalculating the wallet balance - use model cache #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e62958d
GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call
furszy May 7, 2022
321335b
GUI: add getter for WalletModel::m_cached_balances field
furszy May 9, 2022
96e3264
GUI: use cached balance in overviewpage and sendcoinsdialog
furszy May 9, 2022
050e8b1
GUI: 'getAvailableBalance', use cached balance if the user did not se…
furszy Apr 27, 2022
4584d30
GUI: remove now unneeded 'm_balances' field from overviewpage
furszy May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions src/qt/overviewpage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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));
Expand Down Expand Up @@ -277,14 +275,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());
});
Expand All @@ -307,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
Expand Down
1 change: 0 additions & 1 deletion src/qt/overviewpage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public Q_SLOTS:
Ui::OverviewPage *ui;
ClientModel *clientModel;
WalletModel *walletModel;
interfaces::WalletBalances m_balances;
bool m_privacy{false};

const PlatformStyle* m_platform_style;
Expand Down
12 changes: 5 additions & 7 deletions src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -718,9 +716,9 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances)
}
}

void SendCoinsDialog::updateDisplayUnit()
void SendCoinsDialog::refreshBalance()
{
setBalance(model->wallet().getBalances());
setBalance(model->getCachedBalance());
ui->customFee->setDisplayUnit(model->getOptionsModel()->getDisplayUnit());
updateSmartFeeLabel();
}
Expand Down Expand Up @@ -797,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<SendCoinsEntry*>(ui->entries->itemAt(i)->widget());
if (e && !e->isHidden() && e != entry) {
Expand Down
2 changes: 1 addition & 1 deletion src/qt/sendcoinsdialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 13 additions & 15 deletions src/qt/test/wallettests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -193,15 +200,10 @@ void TestGUI(interfaces::Node& node)
sendCoinsDialog.setModel(&walletModel);
transactionView.setModel(&walletModel);

{
// Check balance in send dialog
QLabel* balanceLabel = sendCoinsDialog.findChild<QLabel*>("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();
Copy link
Member

@achow101 achow101 May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7345a04 "test: GUI WalletTests, trigger walletModel balance changed manually."

It seems potentially bad that the cached balance might be out of sync with the real balance and that pollBalanceChanged needs to be called here in the tests?

Copy link
Member Author

@furszy furszy May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's out of sync merely because we are not starting the polling timer at all in the test. Same as we aren't initializing several parts of the GUI objects. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the WalletController class flow.

Which I think that is actually what we are looking for. It's an unit test that is focused on verifying that GUI widgets/views are behaving as expected: updating 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 can create a new test case for it in a follow-up work).

// Check balance in send dialog
CompareBalance(walletModel, walletModel.wallet().getBalance(), sendCoinsDialog.findChild<QLabel*>("labelBalance"));

// Send two transactions, and verify they are added to transaction list.
TransactionTableModel* transactionTableModel = walletModel.getTransactionTableModel();
Expand All @@ -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<QLabel*>("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<QLabel*>("labelBalance"));

// Check Request Payment button
ReceiveCoinsDialog receiveCoinsDialog(platformStyle.get());
Expand Down
20 changes: 18 additions & 2 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -120,12 +124,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
Expand Down Expand Up @@ -194,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)
{
Expand Down Expand Up @@ -599,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;
}
7 changes: 7 additions & 0 deletions src/qt/walletmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ class WalletModel : public QObject

uint256 getLastBlockProcessed() const;

// 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<interfaces::Wallet> m_wallet;
std::unique_ptr<interfaces::Handler> m_handler_unload;
Expand Down