Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BugFix] fix "not updating GUI balance" race condition. #2379

Merged

Conversation

furszy
Copy link

@furszy furszy commented May 11, 2021

The problem arises when the model triggers the balance update task and the chain tip changes (due a reorg) before one of the threadpool's threads starts executing the job. As the tip, in this situation, has the same height that the one cached inside the model, the processBalanceChangeInternal returns true even when the balance update wasn't executed, which never sets the processing flag to false again, blocking every future balance update.

Solves #2371.

@furszy furszy self-assigned this May 11, 2021
@furszy furszy added this to the 6.0.0 milestone May 11, 2021
@furszy furszy added the GUI label May 11, 2021
@furszy furszy changed the title [BugFix] fix not updating GUI balance due a race condition. [BugFix] fix "not updating GUI balance" due a race condition. May 11, 2021
@furszy furszy added the Bug label May 11, 2021
@furszy furszy changed the title [BugFix] fix "not updating GUI balance" due a race condition. [BugFix] fix "not updating GUI balance" race condition. May 11, 2021
random-zebra
random-zebra previously approved these changes May 12, 2021
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 🍺 ACK 8e0b4c3

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved
The problem arises when we trigger the balance update worker task and the chain tip changes (due a reorg) before one of the threadpool's threads start executing the update task. As the tip in this situation has the same height that the one cached inside the model, the `processBalanceChangeInternal` returns true without setting the processing flag to false again. Blocking every future balance update.
@furszy
Copy link
Author

furszy commented May 12, 2021

updated

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fbda4d2 after the update

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't able to ever reproduce the issue of the balance not updating, but can at least confirm that these changes still result in the balance updating as intended.

ACK fbda4d2

@random-zebra random-zebra merged commit ddadcde into PIVX-Project:master May 13, 2021
furszy referenced this pull request in furszy/bitcoin-core Jun 18, 2021
The problem arises when we trigger the balance update worker task and the chain tip changes (due a reorg) before one of the threadpool's threads start executing the update task. As the tip in this situation has the same height that the one cached inside the model, the `processBalanceChangeInternal` returns true without setting the processing flag to false again. Blocking every future balance update.

Github-Pull: bitcoin#2379
Rebased-From: fbda4d2
@random-zebra random-zebra modified the milestones: 6.0.0, 5.2.0 Jun 18, 2021
furszy added a commit that referenced this pull request Jun 19, 2021
3344fb3 [CI] Remove Ubuntu 16.04 from GA workflow, has been deprecated by GA and will be removed on September 20, 2021. (furszy)
b5a8638 [GUI] Cleanup compiler warnings in qtutils.h/cpp (furszy)
73b56e9 [GUI] Fix invisible text due an invalid transparent selection color. (furszy)
df5d820 budget: fixing a possible race condition that could cause a good peer being banned. (furszy)
a5fd073 Fix minimize and close bugs (furszy)
b516e36 depends: update Qt 5.9 source url (Kittywhiskers Van Gogh)
18f4b4a BugFix: fix not updating GUI balance race condition. (furszy)
2c923ed scripted-diff: Replace 'NULL' with 'nullptr' in guiutil.cpp (random-zebra)
7b8b23a Fix memory leaks in qt/guiutil.cpp (Dan Raviv)
cbd5c78 Add search option to My Addresses list in receive widget (Volodia)
0dcbea2 [build] depends macOS: point --sysroot to SDK (Sjors Provoost)
3b542fc [Doc] remove old gitian keys. (furszy)
0f099e8 [GUI] Generate FAQ answer content programmatically (Fuzzbawls)
8483861 qt:Show the entire Window when double clicking on taskbar (ken2812221)
998c7e8 [GUI] fix QT 5.15 `currentIndexChanged(QString)` deprecated method call. (furszy)

Pull request description:

  List of straightforward PRs back ported from v6.0 into the v5.2 branch (ordered by merge date).

  * #2259.
  * #2260.
  * #2348
  * #2350
  * #2353
  * #2305
  * #2374
  * #2379
  * #2384
  * #2377
  * #2395
  * #2401
  * #2413

ACKs for top commit:
  random-zebra:
    utACK 3344fb3
  Fuzzbawls:
    utACK 3344fb3

Tree-SHA512: 4317e83d4c1228b8ae20dc1bc5c8e43ac87598d4d9d9244fdd032f2a0c5eccd1a7ed27bc29094c7411dd653b187c728ab31d732ea686abda15228920a390e4e1
@furszy furszy deleted the 2021_GUI_fix_balance_update branch November 29, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants