gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged

ryanofsky commented at 9:33 pm on April 10, 2020: member

Main commit gui: Avoid wallet tryGetBalances calls is one-line change to WalletModel::pollBalanceChanged that returns early if there hasn’t been a new TransactionChanged or BlockTip notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way.

The other commits are a straight revert of #18160, and two tweaks to avoid relying on WalletModel::m_client_model lifetime which were causing travis failures with earlier versions of this PR.

Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102

This PR is part of the process separation project.

ryanofsky added this to the "In progress" column in a project

promag commented at 9:52 pm on April 10, 2020: member

Worth mentioning that #17905 made this easy (https://github.com/bitcoin/bitcoin/pull/18160#pullrequestreview-364552928) and that #18160 was merged to fix #15015 in 0.19 and 0.20.

in src/qt/walletmodel.cpp:84 in 8928ebd6eb outdated

78@@ -79,6 +79,10 @@ void WalletModel::updateStatus()
79 
80 void WalletModel::pollBalanceChanged()
81 {
82+    // Avoid recomputing wallet balances unless a TransactionChanged or
83+    // BlockTip notification was received.
84+    if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;

promag commented at 10:00 pm on April 10, 2020:

Maybe this should be >=? Can’t tryGetBalances return a block ahead of m_client_model.getNumBlocks()?

Alternatively drop 2nd arg from tryGetBalances and use numBlocks = m_client_model.getNumBlocks()?

ryanofsky commented at 10:30 pm on April 10, 2020:

Maybe this should be >=? Can’t tryGetBalances return a block ahead of m_client_model.getNumBlocks()?

Not sure what goal of this suggestion is. >= would make the check less sensitive in case of a rollback. This code is already not sensitive enough because it is assuming chains the same height are identical.

Alternatively drop 2nd arg from tryGetBalances and use numBlocks = m_client_model.getNumBlocks()?

Again not sure reason for this suggestion. It would make the PR no longer a direct revert of 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 and also no longer be correct when combined with #17954 and when the node tip runs ahead of the wallet last block processed.

Goal of this PR is just to make a one-line change that prevents unnecessary cross-process calls and allows #18160 to be reverted. There’s room for improvement on top of PR, including removing the polling logic altogether, but by itself this PR should be an improvement on a few fronts

promag commented at 10:35 pm on April 10, 2020:

This code is already not sensitive enough because it is assuming chains the same height are identical

Indeed! 🤦

promag commented at 10:36 pm on April 10, 2020: member

Code review ACK 6dbc52bae9f6f7592da457410f63c52572c49fec.

DrahtBot added the label GUI on Apr 10, 2020

DrahtBot added the label Wallet on Apr 10, 2020

ryanofsky force-pushed on Apr 15, 2020

ryanofsky commented at 3:50 pm on April 15, 2020: member

Rebased 6dbc52bae9f6f7592da457410f63c52572c49fec -> 605c3f19be5f7a0f1d2651d00b4db9ba009ac1c7 (pr/ipc-bal.1 -> pr/ipc-bal.2, compare) due to silent conflict with #17954

ryanofsky commented at 9:04 pm on April 15, 2020: member

Travis failure in wallet_resendwallettransactions seems like it must be caused by a problem in master because this PR is just a gui change which shouldn’t be affecting that test at all: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367626#L13955

Travis failure in address sanitizer build is probably caused this PR, and I’m going to try to debug and fix it locally. https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240. Hopefully it’s just a test bug not a code bug

meshcollider commented at 11:21 am on April 17, 2020: contributor

Concept ACK

DrahtBot commented at 5:25 am on April 23, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

ryanofsky force-pushed on Apr 25, 2020

ryanofsky commented at 8:23 pm on April 27, 2020: member

Updated 605c3f19be5f7a0f1d2651d00b4db9ba009ac1c7 -> 6de7f9efc1f18003375997051c3274ecdd4b5860 (pr/ipc-bal.2 -> pr/ipc-bal.3, compare) to fix ClientModel use-after-free when shutdown requested https://travis-ci.org/github/bitcoin/bitcoin/jobs/675367629#L4240 mentioned #18587 (comment)

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.

83f69fab3a

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

2bc9b92ed8

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"'.

bf0a510981

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".

d3a56be77a

ryanofsky force-pushed on May 1, 2020

ryanofsky commented at 3:49 pm on May 1, 2020: member

Rebased 6de7f9efc1f18003375997051c3274ecdd4b5860 -> d3a56be77a9d112cde4baef4314882170b9f228f (pr/ipc-bal.3 -> pr/ipc-bal.4, compare) due to silent conflict with #16426

jonasschnelli commented at 8:59 am on May 20, 2020: contributor

utACK d3a56be77a9d112cde4baef4314882170b9f228f

jonasschnelli merged this on May 20, 2020

jonasschnelli closed this on May 20, 2020

sidhujag referenced this in commit 381c70f27b on May 20, 2020

jonasschnelli referenced this in commit f4b603cff6 on May 29, 2020

sidhujag referenced this in commit 5b92706601 on May 31, 2020

ryanofsky moved this from the "In progress" to the "Done" column in a project

Fabcien referenced this in commit 94640cfde3 on Feb 5, 2021

DrahtBot locked this on Feb 15, 2022

gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged #18587

Conflicts