Qt: Only call tryGetBalances in pollBalanceChanged if the result will be used. #13612

1. 
   tecnovert commented at 6:57 pm on July 7, 2018: contributor

   getBalances takes significant processing if the wallet has many transactions.
2. Avoid calling tryGetBalances in pollBalanceChanged if the result won't be used.

   getBalances takes significant processing if the wallet has many transactions.

   e7ad1768fe
3. 
   sipa commented at 1:32 am on July 8, 2018: member

   Concept ACK, seems reasonable if it’s an issue.
4. fanquake added the label GUI on Jul 8, 2018
5. fanquake added the label Wallet on Jul 8, 2018
6. fanquake requested review from jonasschnelli on Jul 8, 2018
7. 
   MarcoFalke commented at 6:48 am on July 8, 2018: member

   Could do an early return to avoid wrapping the whole function scope in two sets of braces and indenting it by 4 spaces

   On Sat, Jul 7, 2018 at 2:33 PM, Pieter Wuille notifications@github.com wrote:

   Concept ACK, seems reasonable if it’s an issue.

   — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13612#issuecomment-403254845, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv7wlo1WfWRJHWaMjiwiIGZ2r5nfSks5uEWFPgaJpZM4VGdUl .

8. tecnovert force-pushed on Jul 8, 2018
9. Rearranged so TRY_LOCK is called first.

   The LOCK in getNumBlocks shouldn't be run before the TRY_LOCK in
   tryGetBalances
   The neatest fix seems to be to move the height check into tryGetBalances.

   899d8f68a0
10. tecnovert force-pushed on Jul 8, 2018
11. 
    tecnovert commented at 2:11 pm on July 8, 2018: contributor

    It takes a fair amount of blocks and transactions before the issue shows up. CPU usage scales with both the chain height and number of transactions.

    gprof results show most of the time is taken by all the GetDepthInMainChain calls from IsTrusted

    Testing on a regtest node with 10k blocks and 10k txns htop shows: +-6 CPU% before the changes and +-0.7 CPU% after

    20k blocks and 10k txns: +-6.5 CPU% before the changes and +-0.7 CPU% after

    40k blocks and 39.6k txns: +-20 CPU% before the changes and +-0.7 CPU% after

12. 
    promag commented at 9:53 pm on July 8, 2018: member

    Concept ACK.

    A greater improvement would be to avoid pollBalanceChanged each 250ms. Will test.

13. 
    practicalswift commented at 1:13 pm on July 9, 2018: contributor

    Concept ACK

    Nice first-time contribution @tecnovert!

14. 
    jonasschnelli commented at 8:38 pm on July 10, 2018: contributor

    The way how to GUI (and the wallet) calculates the balance has significant performance impacts. I tried in the past to enhance this situation (but failed, see #10251). Concept ACK, will test and perf-compare soon.
15. 
    DrahtBot commented at 8:15 pm on August 3, 2018: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14437 (Refactor: Start to separate wallet from node by ryanofsky)
    • #10973 (Refactor: separate wallet from node by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

16. 
    DrahtBot commented at 9:00 pm on September 7, 2018: member
17. DrahtBot closed this on Sep 7, 2018

    ---

18. DrahtBot reopened this on Sep 7, 2018

    ---

19. DrahtBot added the label Needs rebase on Nov 9, 2018
20. 
    DrahtBot commented at 3:16 pm on November 9, 2018: member
21. in src/interfaces/wallet.cpp:348 in 899d8f68a0

    344@@ -345,16 +345,24 @@ class WalletImpl : public Wallet
    345         }
    346         return result;
    347     }
    348-    bool tryGetBalances(WalletBalances& balances, int& num_blocks) override
    349+    bool tryGetBalances(WalletBalances& balances, bool skip_height_check, int cached_blocks, int& num_blocks) override
    

    ---

    ---

    

    meshcollider commented at 7:42 am on November 10, 2018:

    I feel like this bool name is a little confusing, could be ambiguous on exactly what it is skipping. Perhaps ignore_cached or force_check_balance?
22. 
    meshcollider commented at 11:17 am on November 14, 2018: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/13612/commits/899d8f68a09d57d579b81a7e1d721a51e8c611bf, still needs rebase though
23. cryptozeny referenced this in commit b6f0a9d6c1 on Feb 8, 2019
24. cryptozeny referenced this in commit 2a96f22c92 on Feb 8, 2019
25. cryptozeny referenced this in commit 50403a93c6 on Feb 8, 2019
26. cryptozeny referenced this in commit ea78562840 on Feb 8, 2019
27. cryptozeny referenced this in commit fff78b9b6f on Feb 8, 2019
28. cryptozeny referenced this in commit 72436c90b2 on Feb 8, 2019
29. fanquake added the label Up for grabs on Aug 4, 2019
30. fanquake closed this on Aug 4, 2019

    ---

31. laanwj removed the label Needs rebase on Oct 24, 2019
32. 
    jarolrod commented at 9:28 pm on March 25, 2021: member

    @fanquake This can have the Up For Grabs label removed. A similar fix was done in #18160, then reverted as part of a different fix in https://github.com/bitcoin/bitcoin/pull/18587
33. fanquake removed the label Up for grabs on Mar 25, 2021
34. DrahtBot locked this on Aug 16, 2022

Contributors
tecnovert sipa MarcoFalke promag practicalswift jonasschnelli DrahtBot meshcollider jarolrod

Review Requested
jonasschnelli

Labels
GUI Wallet