gui: Fix race in WalletModel::pollBalanceChanged #18123

ryanofsky · 2020-02-11T22:28:32Z

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 a0704a8#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 8937d99 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 a0704a8 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.

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 bitcoin@a0704a8#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 8937d99 from bitcoin#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 a0704a8 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 bitcoin#17954.

maflcko · 2020-02-11T22:52:28Z

This might be another reason that users are seeing issues like #17112 . We might want to consider this for backport

luke-jr · 2020-02-12T01:37:17Z

utACK

DrahtBot · 2020-02-12T02:29:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

gui: Balance/TxStatus polling update based on last block hash. #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.

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 bitcoin@a0704a8#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 8937d99 from bitcoin#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. Thanks to John Newbery <john@johnnewbery.com> for catching this while reviewing bitcoin#17954. Github-Pull: bitcoin#18123 Rebased-From: 37d27bc

jonasschnelli · 2020-02-12T14:47:33Z

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

utACK 37d27bc

ryanofsky · 2020-02-12T19:17:03Z

Updated 37d27bc -> bf36a3c (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

Empact · 2020-02-12T20:08:38Z

utACK bf36a3c

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 bitcoin@a0704a8#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 8937d99 from bitcoin#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 a0704a8 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 bitcoin#17954. Github-Pull: bitcoin#18123 Rebased-From: bf36a3c

jonasschnelli · 2020-02-13T07:43:56Z

utACK bf36a3c

bf36a3c gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky) Pull request description: 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 a0704a8#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 8937d99 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 a0704a8 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. ACKs for top commit: Empact: utACK bf36a3c jonasschnelli: utACK bf36a3c Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4

- [0.19] wallet: Reset reused transactions cache bitcoin#18083 - 0.19: Backports bitcoin#17792 - psbt: handle unspendable psbts bitcoin#17524 - qt: Fix comparison function signature bitcoin#17634 - psbt: check that various indexes and amounts are within bounds bitcoin#17156 - [0.19] psbt: check that various indexes and amounts are within bounds bitcoin#18079 - [0.19] Final backports for 0.19.1 bitcoin#17988 - Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash bitcoin#17924 - qt: Fix deprecated QCharRef usage bitcoin#18101 - gui: Throttle GUI update pace when -reindex bitcoin#18121 - gui: Fix race in WalletModel::pollBalanceChanged bitcoin#18123 - gui: Fix unintialized WalletView::progressDialog bitcoin#18062 - Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open bitcoin#18007 - bug-fix macos: give free bytes to F_PREALLOCATE bitcoin#17887 - test: add missing #include to fix compiler errors bitcoin#17980 - zmq: Fix due to invalid argument and multiple notifiers bitcoin#17445

promag · 2020-02-16T10:34:56Z

ACK bf36a3c.

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.

ryanofsky · 2020-02-25T23:08:34Z

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

promag · 2020-02-25T23:22:40Z

@ryanofsky ah! thanks for explaining. Nice catch @jnewbery.

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 bitcoin@a0704a8#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 8937d99 from bitcoin#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 a0704a8 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 bitcoin#17954. Github-Pull: bitcoin#18123 Rebased-From: bf36a3c

fanquake · 2020-02-28T04:15:12Z

Being backported in 18218.

48fef5e gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky) 1964561 build: don't embed a build-id when building libdmg-hfsplus (fanquake) Pull request description: These are not blockers for the 0.19.1 release, as per [IRC discussion this morning](http://www.erisian.com.au/bitcoin-core-dev/log-2020-02-27.html#l-331), doesn't look like there will be an rc3. This PR can collect further backports for the 0.19 branch. Currently backports: * #18004 - build: don't embed a build-id when building libdmg-hfsplus * #18123 - gui: Fix race in WalletModel::pollBalanceChanged ACKs for top commit: promag: ACK 48fef5e. laanwj: ACK 48fef5e luke-jr: utACK 48fef5e Tree-SHA512: c7e7ddda9ee7b8015f16d39aab000e0595f85fe073f79abc1a57b3e2adb0dedc4e07e5fd918e1df5e88b7f3fbc39b57ab3382233c4354b9c2196f65fa1fa6c04

bf36a3c gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky) Pull request description: 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 bitcoin@a0704a8#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 8937d99 from bitcoin#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 a0704a8 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 bitcoin#17954. ACKs for top commit: Empact: utACK bitcoin@bf36a3c jonasschnelli: utACK bf36a3c Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403

Summary: 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 bitcoin/bitcoin@a0704a8#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 [[bitcoin/bitcoin#17954 | PR17954]] 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 a0704a8 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 [[bitcoin/bitcoin#17954 | PR17954]]. This is a backport of Core [[bitcoin/bitcoin#18123 | PR18123]] Test Plan: `ninja && src/qt/bitcoin-qt` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8764

fanquake added the GUI label Feb 11, 2020

DrahtBot mentioned this pull request Feb 12, 2020

gui: Balance/TxStatus polling update based on last block hash. #17993

Merged

ryanofsky mentioned this pull request Feb 12, 2020

Refactor: separate gui from wallet and node #10244

Merged

ryanofsky force-pushed the pr/pollbug branch from 37d27bc to bf36a3c Compare February 12, 2020 19:11

jonasschnelli merged commit bf36a3c into bitcoin:master Feb 13, 2020

ryanofsky mentioned this pull request Feb 13, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

maflcko added this to the 0.19.2 milestone Feb 13, 2020

maflcko added the Needs backport (0.19) label Feb 13, 2020

promag mentioned this pull request Feb 16, 2020

gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18160

Merged

fanquake mentioned this pull request Feb 28, 2020

[0.19] Further 0.19 backports #18218

Merged

fanquake removed the Needs backport (0.19) label Feb 28, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

xdustinface mentioned this pull request Aug 30, 2020

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403 dashpay/dash#3682

Merged

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 14, 2020

Merge pull request #3682 from xdustinface/backport-10244-18123

74f4d2a

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gui: Fix race in WalletModel::pollBalanceChanged #18123

gui: Fix race in WalletModel::pollBalanceChanged #18123

Uh oh!

ryanofsky commented Feb 11, 2020 •

edited

Loading

Uh oh!

maflcko commented Feb 11, 2020

Uh oh!

luke-jr commented Feb 12, 2020

Uh oh!

DrahtBot commented Feb 12, 2020

Uh oh!

jonasschnelli commented Feb 12, 2020

Uh oh!

ryanofsky commented Feb 12, 2020 •

edited

Loading

Uh oh!

Empact commented Feb 12, 2020 •

edited

Loading

Uh oh!

jonasschnelli commented Feb 13, 2020

Uh oh!

promag commented Feb 16, 2020

Uh oh!

ryanofsky commented Feb 25, 2020

Uh oh!

promag commented Feb 25, 2020

Uh oh!

fanquake commented Feb 28, 2020

Uh oh!

Uh oh!

gui: Fix race in WalletModel::pollBalanceChanged #18123

gui: Fix race in WalletModel::pollBalanceChanged #18123

Uh oh!

Conversation

ryanofsky commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 11, 2020

Uh oh!

luke-jr commented Feb 12, 2020

Uh oh!

DrahtBot commented Feb 12, 2020

Conflicts

Uh oh!

jonasschnelli commented Feb 12, 2020

Uh oh!

ryanofsky commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Feb 13, 2020

Uh oh!

promag commented Feb 16, 2020

Uh oh!

ryanofsky commented Feb 25, 2020

Uh oh!

promag commented Feb 25, 2020

Uh oh!

fanquake commented Feb 28, 2020

Uh oh!

Uh oh!

ryanofsky commented Feb 11, 2020 •

edited

Loading

ryanofsky commented Feb 12, 2020 •

edited

Loading

Empact commented Feb 12, 2020 •

edited

Loading