Skip to content

Commit

Permalink
Merge bitcoin#18123: gui: Fix race in WalletModel::pollBalanceChanged
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jonasschnelli authored and xdustinface committed Sep 29, 2020
1 parent d4c67d1 commit 57dbdf3
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/qt/walletmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ void WalletModel::pollBalanceChanged()
return;
}

if(fForceCheckBalanceChanged || m_node.getNumBlocks() != cachedNumBlocks || node().privateSendOptions().getRounds() != cachedPrivateSendRounds)
if(fForceCheckBalanceChanged || numBlocks != cachedNumBlocks || node().privateSendOptions().getRounds() != cachedPrivateSendRounds)
{
fForceCheckBalanceChanged = false;

// Balance and number of transactions might have changed
cachedNumBlocks = m_node.getNumBlocks();
cachedNumBlocks = numBlocks;
cachedPrivateSendRounds = node().privateSendOptions().getRounds();

checkBalanceChanged(new_balances);
Expand Down

0 comments on commit 57dbdf3

Please sign in to comment.