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

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/ipc-bal changing 6 files +32 −20
  1. 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.

  2. ryanofsky added this to the "In progress" column in a project
  3. 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.
  4. 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! 🤦

  5. promag commented at 10:36 pm on April 10, 2020: member
    Code review ACK 6dbc52bae9f6f7592da457410f63c52572c49fec.
  6. DrahtBot added the label GUI on Apr 10, 2020
  7. DrahtBot added the label Wallet on Apr 10, 2020
  8. ryanofsky force-pushed on Apr 15, 2020
  9. 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
  10. 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

  11. meshcollider commented at 11:21 am on April 17, 2020: contributor
    Concept ACK
  12. 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.

  13. ryanofsky force-pushed on Apr 25, 2020
  14. 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)
  15. 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
  16. 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
  17. 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
  18. 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
  19. ryanofsky force-pushed on May 1, 2020
  20. 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
  21. jonasschnelli commented at 8:59 am on May 20, 2020: contributor
    utACK d3a56be77a9d112cde4baef4314882170b9f228f
  22. jonasschnelli merged this on May 20, 2020
  23. jonasschnelli closed this on May 20, 2020
  24. sidhujag referenced this in commit 381c70f27b on May 20, 2020
  25. jonasschnelli referenced this in commit f4b603cff6 on May 29, 2020
  26. sidhujag referenced this in commit 5b92706601 on May 31, 2020
  27. ryanofsky moved this from the "In progress" to the "Done" column in a project
  28. Fabcien referenced this in commit 94640cfde3 on Feb 5, 2021
  29. DrahtBot locked this on Feb 15, 2022


ryanofsky promag meshcollider DrahtBot jonasschnelli

Labels
GUI Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me