gui: Fix race in WalletModel::pollBalanceChanged #18123

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/pollbug changing 1 files +2 −2
  1. ryanofsky commented at 10:28 pm on February 11, 2020: member

    Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling.

    This bug could cause balances to appear out of date, and was first introduced https://github.com/bitcoin/bitcoin/pull/10244/commits/a0704a8996bb950ae3c4d5b5a30e9dfe34cde1d3#diff-2e3836af182cfb375329c3463ffd91f8L117. Before that commit, there wasn’t a problem because cs_main was held during the poll update.

    Currently, the problem should be rare. But if 8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification.

    MarcoFalke also points out that a0704a8996b could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI.

    Thanks to John Newbery for finding this bug this while reviewing #17954.

  2. gui: Fix race in WalletModel::pollBalanceChanged 
    Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
https://github.com/bitcoin/bitcoin/pull/10244/commits/a0704a8996bb950ae3c4d5b5a30e9dfe34cde1d3#r378452145
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

MarcoFalke <falke.marco@gmail.com> also points out that a0704a8996b could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.

Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing https://github.com/bitcoin/bitcoin/pull/17954.
    bf36a3ccc2
  3. fanquake added the label GUI on Feb 11, 2020
  4. MarcoFalke commented at 10:52 pm on February 11, 2020: member
    This might be another reason that users are seeing issues like #17112 . We might want to consider this for backport
  5. luke-jr commented at 1:37 am on February 12, 2020: member
    utACK
  6. DrahtBot commented at 2:29 am on February 12, 2020: 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:

    • #17993 (gui: Avoid redundant cs_main locks in balance polling. by furszy)

    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.

  7. luke-jr referenced this in commit 330a0986e8 on Feb 12, 2020
  8. jonasschnelli commented at 2:47 pm on February 12, 2020: contributor

    Good catch and thanks for the fix. Agree with @MarcoFalke that this should be backported.

    utACK 37d27bc070553505568cda900d22b5a3600fb54c

  9. ryanofsky force-pushed on Feb 12, 2020
  10. ryanofsky commented at 7:17 pm on February 12, 2020: member
    Updated 37d27bc070553505568cda900d22b5a3600fb54c -> bf36a3ccc212ad4d7c5cb8f26d7a22e279fe3cec (pr/pollbug.1 -> pr/pollbug.2, compare). No code changes, just updated the commit description to mention how this could be related to GUI hangs
  12. luke-jr referenced this in commit 5a7833aec0 on Feb 13, 2020
  13. jonasschnelli commented at 7:43 am on February 13, 2020: contributor
    utACK bf36a3c
  14. jonasschnelli referenced this in commit b6a16fa44e on Feb 13, 2020
  15. jonasschnelli merged this on Feb 13, 2020
  16. jonasschnelli closed this on Feb 13, 2020
  17. MarkLTZ referenced this in commit d99bc96496 on Feb 13, 2020
  18. MarcoFalke added this to the milestone 0.19.2 on Feb 13, 2020
  19. MarcoFalke added the label Needs backport (0.19) on Feb 13, 2020
  20. promag commented at 10:34 am on February 16, 2020: member

    ACK bf36a3ccc212ad4d7c5cb8f26d7a22e279fe3cec.

    This might be another reason that users are seeing issues like #17112 . We might want to consider this for backport @MarcoFalke tryGetBalance just avoids waiting for the lock, is doesn’t save you the time spent in CWallet::GetBalance, so I don’t see how this could make it better. See #16874.

  21. ryanofsky commented at 11:08 pm on February 25, 2020: member

    tryGetBalance just avoids waiting for the lock, is doesn’t save you the time spent in CWallet::GetBalance, so I don’t see how this could make it better @promag The case where this could alleviate GUI hangs is the case where tryGetBalance succeeds in getting the locks and balance without blocking, but then before the one of the two m_node.getNumBlocks() something else acquires cs_main, and the GUI thread blocks in the m_node.getNumBlocks() call

  22. promag commented at 11:22 pm on February 25, 2020: member
    @ryanofsky ah! thanks for explaining. Nice catch @jnewbery.
  23. fanquake referenced this in commit 48fef5ebae on Feb 28, 2020
  24. fanquake commented at 4:15 am on February 28, 2020: member
    Being backported in 18218.
  25. fanquake removed the label Needs backport (0.19) on Feb 28, 2020
  26. MarcoFalke referenced this in commit 05f5dd96c7 on Mar 4, 2020
  27. xdustinface referenced this in commit eb72a1275b on Aug 30, 2020
  28. xdustinface referenced this in commit b2ff7243c3 on Aug 30, 2020
  29. xdustinface referenced this in commit 93d6d65a34 on Aug 30, 2020
  30. xdustinface referenced this in commit 590e456bff on Aug 30, 2020
  31. xdustinface referenced this in commit 8f22f0ff7a on Aug 30, 2020
  32. xdustinface referenced this in commit 747c0be07f on Aug 30, 2020
  33. xdustinface referenced this in commit 620e140d9b on Aug 30, 2020
  34. xdustinface referenced this in commit a553b26b13 on Aug 31, 2020
  35. xdustinface referenced this in commit f10d0f8127 on Sep 27, 2020
  36. xdustinface referenced this in commit fee99a16c5 on Sep 29, 2020
  37. xdustinface referenced this in commit 57dbdf304a on Sep 29, 2020
  38. xdustinface referenced this in commit 13bdfff80e on Oct 2, 2020
  39. xdustinface referenced this in commit 7cb8b6bc9d on Oct 2, 2020
  40. xdustinface referenced this in commit 852628ec4e on Oct 5, 2020
  41. xdustinface referenced this in commit ff3104dba3 on Oct 5, 2020
  42. xdustinface referenced this in commit 7c6cecaf29 on Oct 14, 2020
  43. UdjinM6 referenced this in commit 74f4d2a898 on Oct 14, 2020
  44. Fabcien referenced this in commit 527cee36c4 on Jan 2, 2021
  45. DrahtBot locked this on Feb 15, 2022


ryanofsky MarcoFalke luke-jr DrahtBot jonasschnelli Empact promag fanquake

Labels
GUI

Milestone
0.19.2

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-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me