Killing wallet and GUI cs_main locks #2209

furszy · 2021-02-17T21:23:49Z

This is the conclusion of a deep deep rabbit hole: #1726, #2150, #2082, #2118, #2150, #2179, #2191, #2192, #2195, #2201, #2203..

Effectively removing every cs_main lock from the wallet and GUI processing threads. Completely decoupling the wallet state and the visual interface update procedures from the main message and validation handler thread.

Meaning that the messages & validation handler thread will be largely more active, accepting and verifying way more data in less time, not being affected by several other threads accessing to the main critical section. And, at the same time, the wallet and GUI worker threads will be able to perform and process their tasks concurrently, without waiting for the cs_main mutex acquisition.
Improving the overall software performance, the GUI responsiveness and decreasing the synchronization time.

To make this possible, the wallet is maintaining in memory a view of the chain and updating it only via the validation interface signals. Using the view to perform all of the chain related calculations.

for now, IsSpent() requires cs_main lock due its internal call to GetDepthInMainChain. This dependency will be completely removed moving forward, in bitcoin#2209

furszy · 2021-02-26T14:12:45Z

Rebased on top of master, plus after some testing on a big wallet (+76k transactions), expanded the work with some more improvements:

Moved the wallet model balance polling update and the send screen refresh amounts processes to a background worker thread.
Send and cold staking screens: coin control dialog model initialization only performed on demand and not at startup.
Dashboard and cold staking chart and cs delegations model refresh: do not try to recalculate them if the screen is not visible.
Wallet GetLockedCoins() is not longer looping over the entire wallet map to retrieve the locked balance, only through the locked outputs.
Delegation and cs balances only calculated when are needed on-demand (removing two unneeded for loops over all of the txes set..).
Transaction model: tx update not locking cs_wallet for the entire process, only when the status update is required.
Staking filter not initialized if chart is not being shown.
Fixed tx records state update recalculation that was forcing to update every record with every new arriving block.
Chart update time spacing increased when the wallet is in IBD.
Refactor: MacOS notification handler using `QString::toNSString

Plus some few other small bug fixes that found while was doing all of this.

furszy · 2021-03-01T14:16:22Z

Added few more commits, minimizing the amount of work required for the wallet to calculate the balance and to re-accept transactions at startup (both areas were walking through the entire wallet transactions set several times, looking for slightly different information, instead of doing it just once and grouping the results altogether). Affecting mostly to large wallets smoothness and decreasing their startup time considerably.

The behavior of MacNotificationHandler::showNotification() has not been changed.

…wallet post initialization process

…nsactions()

Coming from btc@fa795cf9c52b82cc3cccd21483360d6e03f767f0

future: add watch-only and CS balances distinction when/if they are needed (at the moment, they aren't). Inspired in btc@fa57411fcba00556ba25d45bca53cc04623da051 .

ClientModel::getBlockSource is locking cs_nodes every second and the result is currently not used.

…ate.

…the screen is not visible.

…once. gui send screen: remove on-demand delegated balance calculation

So it doesn't invalidly re-use the cached one.

furszy · 2021-03-11T20:48:00Z

Force-pushed it due a bug over the delegated balance update in the send screen (reported by fuzz).
Added the following changes:

a1390c3 adding the delegated balance to the balances wrapper. Removing the on-demand delegated balance calculation (another loop over the entire wallet txes map) needs on the send screen at every show event and delegation checkbox check.
ca3edc5 missing worker request type update.
Thanks to (1), was able to unify the two new request types that added in this PR in the send screen into a single REQUEST_REFRESH_BALANCE without decreasing the performance (squashed in 379e5f2 that was the commit who introduced it).

Fuzzbawls

All good now, ACK ca3edc5

random-zebra

ACK ca3edc5 and merging...

…tChain + optimization 50dbec5 Add unit tests for signals generated by ProcessNewBlock() (furszy) f68251d Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy) a51a755 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen) 198f435 Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo) 8d15cf5 Optimize ActivateBestChain for long chains (Pieter Wuille) 8640be1 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo) ef24337 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) f9d2ab3 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) cb9bb25 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 4ea2048 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: Made some back ports and adaptations to validate further the work introduced in #2203 and #2209. Now that the scheduler thread is processing most of the chain events, the callbacks execution order must be verified to dispatch the events serially, ensuring a consistent memory state in each event processing invocation. There are some concurrency issues solved, as well a an optimization for larger chains for the ABC workflow. Each commit details them well. List: bitcoin#7917 (only fb8fad1) bitcoin#12988 bitcoin#13023 bitcoin#13247 bitcoin#13835 This is built on top of #2284 (and the reason that made me notice that problem). So, 2284 comes first, then this one. ACKs for top commit: Fuzzbawls: ACK 50dbec5 random-zebra: ACK 50dbec5 and merging... Tree-SHA512: 573144cc96d2caa49ed2f0887dc8c53b5aca0efd54b6a25626063ccb780da426f56b3c6b7491648e7abefc1960c82b1354a4ff2c51425bfe99adaa4394562608

…fication fix f2734f0 [Tests] Fix policyestimator and validation_block tests (random-zebra) 3448bc8 wallet: Minimal fix to restore conflicted transaction notifications (Antoine Riard) ba9d84d feature_notifications.py mimic upstream is_wallet_compiled() function to make future back ports easier. (furszy) 37c9944 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery) 5cc619f [validation] Remove pool member from ConnectTrace (John Newbery) 1e453b7 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery) e774bfd [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery) 389680a [validation interface] Remove vtxConflicted from BlockConnected (furszy) 19c9383 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery) 139882d Fire TransactionRemovedFromMempool from mempool This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. (furszy) Pull request description: PR built on top of #2209, #2235 and #2240 (don't get scared by the amount of commits, most of them are coming from 2209. Will disappear as soon as that one gets merged). Based on the brief talk originated in #2209 (comment) . Solving the conflicted transactions external listeners notification. Adapting the following PRs: bitcoin#14384, bitcoin#17477 and bitcoin#18982. Without functional test support, for the time being, for the lack of RBF functionality. ACKs for top commit: Fuzzbawls: ACK f2734f0 random-zebra: re-utACK f2734f0 and merging... Tree-SHA512: 07d437e0d5e5d9798b16d982b6f58585d1f0dcd82251c5ac06f47e44233433831243d451ce43068e33c6002a64b76fa4d165167a2cbaeeedc28cde2019853565

…dling 57221af net_processing: Orphans map contemplating large shield transactions. (furszy) 9ec918f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 488a0e3 Select orphan transaction uniformly for eviction (Pieter Wuille) d6e3ebe tests: Add missing locking annotations and locks (practicalswift) 3b7c9e5 Create new mutex for orphans, no cs_main in PLV::BlockConnected (Matt Corallo) 75cd6eb net_processing: Remove single zc transactions message ATMP call. (furszy) ba63837 net_processing: Do not process single zc transactions messages anymore. (furszy) f048869 net_processing: Add missing erase orphan transactions when a block is connected. (furszy) 71fd4b2 Remove mapOrphanTransactionsByPrev from DoS_tests (Pieter Wuille) 408a51a Align struct COrphan definition (Pieter Wuille) b5e650e Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell) a065252 Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell) 59ff590 Adds an expiration time for orphan tx. (Gregory Maxwell) d7461b8 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille) Pull request description: Upgraded the orphan transactions management process, getting us much closer to upstream. Skipped several intermediate PRs because our code is way too far ahead of them in some areas like the validation interface and block processing after #2209. Plus added a net_processing zc cleanup. Important changes: * The elimination of a leak in the orphan map, which was not connected to the block connection process, always growing to its maximum size. * Early detection and discard of zc txs received as single tx message. * Removal of the ATMP check of zc txs received as single tx message. * Removed the need of `cs_main` lock for accessing `mapOrphanTransactions` and `mapOrphanTransactionsByPrev`. ACKs for top commit: random-zebra: utACK 57221af Fuzzbawls: ACK 57221af Tree-SHA512: 816fd059fe0fb02d69f0c5d57ddfe4f569183a16e7a5fc6b643f4a7768674de6fd143232ec82b02c71c852b18b1d6e91a6f1ecc7b2ba576ba6db5d3f1498e657

8afb350 [GUI] Fix Cold Staking address list (Fuzzbawls) Pull request description: #2209 (specifically c2d66d0) introduced a visual bug with the display of cold staking addresses. Instead of showing the address as intended, the label was being duplicated. This restores the intended behavior. Without this patch: ![Image 050](https://user-images.githubusercontent.com/7393257/115131204-2b52c200-9fab-11eb-8d05-f75ffb634a55.png) ![Image 051](https://user-images.githubusercontent.com/7393257/115131207-2beb5880-9fab-11eb-861a-97fa334f192b.png) With this patch: ![Image 053](https://user-images.githubusercontent.com/7393257/115131214-3574c080-9fab-11eb-98a4-a6b3690174c8.png) ![Image 052](https://user-images.githubusercontent.com/7393257/115131217-37d71a80-9fab-11eb-9398-7cddc8f5ec68.png) ACKs for top commit: random-zebra: ACK 8afb350 furszy: ACK 8afb350 Tree-SHA512: c39701f4985926f8763faba060c741e8525b8a4674fa02004d1ec617133f6c65f3829adf5626e2d949b26be8a47a5961312c9fca261e5eaac3f6702dfddef0ee

furszy self-assigned this Feb 17, 2021

furszy added this to the 5.1.0 milestone Feb 18, 2021

furszy added GUI Wallet labels Feb 18, 2021

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch 2 times, most recently from 384252b to 2236d68 Compare February 23, 2021 00:20

furszy mentioned this pull request Feb 23, 2021

Call wallet/validation notify callbacks in scheduler thread (without cs_main) #2203

Merged

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch from 2236d68 to 4f76f13 Compare February 23, 2021 14:08

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch 3 times, most recently from dfdf7df to 9384d4d Compare February 24, 2021 13:07

furszy mentioned this pull request Feb 25, 2021

[GUI][Visual bug] Transaction details shown TWICE. #1480

Closed

furszy dismissed a stale review via 7631e95 February 25, 2021 23:05

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch 4 times, most recently from 23fc5c9 to 3c0a122 Compare February 26, 2021 13:34

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch 2 times, most recently from 5f3f38d to 0df71e5 Compare February 26, 2021 17:08

furszy changed the title ~~[WIP] Killing wallet and GUI cs_main locks~~ Killing wallet and GUI cs_main locks Feb 28, 2021

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch 6 times, most recently from 1484bc8 to 578a28e Compare March 1, 2021 05:08

furszy and others added 16 commits March 11, 2021 17:18

GUI: dashboard, charts update delay time increased for IBD.

b4ab286

GUI: txmodel, do not lock cs_wallet if no wtx status update is needed.

69b3330

Refactoring with QString::toNSString

8762e81

The behavior of MacNotificationHandler::showNotification() has not been changed.

init: move "Done loading" message and rpc warm up finished after the …

7cb8276

…wallet post initialization process

GUI dashboard: do not update the staking filter if it's not needed.

3fb3f34

wallet model: update delay increased to 5 seconds.

bc76186

GUI: add missing qt metatype declarations.

63f3fa7

wallet: removing unnecessary mempool.cs lock from ReacceptWalletTra…

5658844

…nsactions()

GUI: MN model, remove now unneeded cs_main locks.

62cd35f

wallet: Disallow abandon of conflicted txes

32538ae

Coming from btc@fa795cf9c52b82cc3cccd21483360d6e03f767f0

wallet: single loop to calculate the currently required balances.

e9c160a

future: add watch-only and CS balances distinction when/if they are needed (at the moment, they aren't). Inspired in btc@fa57411fcba00556ba25d45bca53cc04623da051 .

GUI: topbar, removing not used block source.

ef77fdf

ClientModel::getBlockSource is locking cs_nodes every second and the result is currently not used.

Masternode-sync read only function to get the "IsBlockchainSynced" st…

cbc5021

…ate.

GUI: settings information, do not update block num and block hash if …

02eb781

…the screen is not visible.

wallet balances, cache total delegated balance and calculate it only …

a1390c3

…once. gui send screen: remove on-demand delegated balance calculation

GUI: Update worker type if task already exist.

ca3edc5

So it doesn't invalidly re-use the cached one.

furszy dismissed random-zebra’s stale review via ca3edc5 March 11, 2021 20:37

furszy force-pushed the 2020_wallet_and_GUI_locks_removed branch from 358a001 to ca3edc5 Compare March 11, 2021 20:37

Fuzzbawls approved these changes Mar 12, 2021

View reviewed changes

random-zebra approved these changes Mar 12, 2021

View reviewed changes

random-zebra merged commit b6d5146 into PIVX-Project:master Mar 12, 2021

furszy mentioned this pull request Apr 2, 2021

[Validation][Test] Fix concurrency issues in ActivateBestChain + optimization #2290

Merged

furszy mentioned this pull request Apr 8, 2021

[net_processing] Improve/upgrade orphan transactions handling #2301

Merged

Fuzzbawls mentioned this pull request Apr 18, 2021

[GUI] Fix Cold Staking address list #2321

Merged

furszy deleted the 2020_wallet_and_GUI_locks_removed branch November 29, 2022 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Killing wallet and GUI cs_main locks #2209

Killing wallet and GUI cs_main locks #2209

Uh oh!

furszy commented Feb 17, 2021 •

edited

Loading

Uh oh!

furszy commented Feb 26, 2021

Uh oh!

furszy commented Mar 1, 2021

Uh oh!

furszy commented Mar 11, 2021 •

edited

Loading

Uh oh!

Fuzzbawls left a comment

Uh oh!

random-zebra left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Killing wallet and GUI cs_main locks #2209

Killing wallet and GUI cs_main locks #2209

Uh oh!

Conversation

furszy commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Feb 26, 2021

Uh oh!

furszy commented Mar 1, 2021

Uh oh!

furszy commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

furszy commented Feb 17, 2021 •

edited

Loading

furszy commented Mar 11, 2021 •

edited

Loading