Call wallet/validation notify callbacks in scheduler thread (without cs_main) #2203
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
furszy
force-pushed
the
2020_move_wallet_signals_to_background
branch
from
d3e4df9 to
e1879e4
Compare
furszy
force-pushed
the
2020_move_wallet_signals_to_background
branch
from
1086f58 to
472020c
Compare
Author
|
Rebased on master after #2192 merge, ready for review. |
furszy
force-pushed
the
2020_move_wallet_signals_to_background
branch
4 times, most recently
from
4b19905 to
4c3ab41
Compare
This is currently unused, but will by used by wallet to cache when transactions are in the mempool, obviating the need for calls to mempool from CWalletTx::InMempool()
This is both good practice (we want to move all such callbacks into a background thread eventually) and prevents a lock inversion when we go to use this in wallet (mempool.cs->cs_wallet and cs_wallet->mempool.cs would otherwise both be used).
This blocks until the wallet has synced up to the current height.
Adaptation from btc@5d67a7868db188f7e43ce9028f215034d057790d This prevents the wallet-RPCs-return-stale-info issue from being re-introduced when new-block callbacks no longer happen in the block-connection cs_main lock
This avoid calling out to mempool state during coin selection, balance calculation, etc. In the next commit we ensure all wallet callbacks from CValidationInterface happen in the same queue, serialized with each other. This helps to avoid re-introducing one of the issues described in bitcoin#9584 [1] by further disconnecting wallet from current chain/mempool state. Thanks to @morcos for the suggestion to do this.
This runs Block{Connected,Disconnected}, SetBestChain, Inventory,
and TransactionAddedToMempool on the background scheduler thread.
Of those, only BlockConnected is used outside of Wallet/ZMQ, and
is used only for orphan transaction removal in net_processing,
something which does not need to be synchronous with anything
else.
This partially reverts bitcoin#9583, re-enabling some of the gains from
bitcoin#7946. This does not, however, re-enable the gains achieved by
repeatedly releasing cs_main between each transaction processed.
Note that UpdatedBlockTip is also used in net_processing to announce new blocks to peers. As this may need additional review, this change is included in its own commit.
…e of pushing the message.
Base work inspiration btc@f69c4370d0a43f798af0c7b3c4c5e4b1929d92a3.
they are networkely disabled and checkpoints are preventing the chain for any large reorganization.
Author
|
update: just in case, there is currently here a lock held assertion failing that is solved down the commits line in #2209, commit be1780316184b067242049ea34698ee96993ced2. |
furszy
force-pushed
the
2020_move_wallet_signals_to_background
branch
from
c346397 to
6dedb2e
Compare
Coming from btc@fa3528a85b05ea9507077f3eb340c9fb189251a6
This is highly slowing down the invalidate block/chain process for no reason.
…d of only at the end.
…we currently don't support.
GetNullifiersForAddresses() asserts for cs_wallet lock held.
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
force-pushed
the
2020_move_wallet_signals_to_background
branch
from
5e7b6ce to
3d11027
Compare
random-zebra
approved these changes
Feb 24, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Great job! 🍻 ACK 3d11027
Left a few minor nits, that can be ignored and/or addressed later.
Comment on lines
934
to
-930
| { | ||
| LOCK(cs_main); |
There was a problem hiding this comment.
nit: can remove the code-block now
| RPCs whose behavior does *not* depend on the current chainstate may omit this | ||
| call. | ||
| - *Rationale*: In previous versions of Bitcoin Core, the wallet was always |
There was a problem hiding this comment.
nit:
Suggested change
| - *Rationale*: In previous versions of Bitcoin Core, the wallet was always | |
| - *Rationale*: In previous versions of PIVX Core, the wallet was always |
Comment on lines
+57
to
63
| // Start the lightweight task scheduler thread | ||
| CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, &scheduler); | ||
| threadGroup.create_thread(std::bind(&TraceThread<CScheduler::Function>, "scheduler", serviceLoop)); | ||
|
|
||
| // Note that because we don't bother running a scheduler thread here, | ||
| // callbacks via CValidationInterface are unreliable, but that's OK, | ||
| // our unit tests aren't testing multiple parts of the code at once. |
There was a problem hiding this comment.
Nit.
This comment doesn't make sense now.
Better remove it and replace the one at line 57 with
// We have to run a scheduler thread to prevent ActivateBestChain
// from blocking due to queue overrun.
| bool SaplingScriptPubKeyMan::IsNoteSaplingChange(const SaplingOutPoint& op, libzcash::SaplingPaymentAddress address) const | ||
| { | ||
| LOCK(wallet->cs_KeyStore); | ||
| LOCK2(wallet->cs_wallet, wallet->cs_KeyStore); |
There was a problem hiding this comment.
can remove the cs_KeyStore lock here, as GetNullifiersForAddresses takes care of it via GetSaplingIncomingViewingKey.
Fuzzbawls
approved these changes
Feb 26, 2021
|
|
||
| AssertLockNotHeld(cs_main); | ||
| { // cs_main scope | ||
| LOCK(cs_main); |
Collaborator
There was a problem hiding this comment.
nit: should indent everything by 4 spaces after the new opening scope brace
random-zebra
added a commit
that referenced
this pull request
Mar 12, 2021
ca3edc5 GUI: Update worker type if task already exist. (furszy) a1390c3 wallet balances, cache total delegated balance and calculate it only once. (furszy) 02eb781 GUI: settings information, do not update block num and block hash if the screen is not visible. (furszy) cbc5021 Masternode-sync read only function to get the "IsBlockchainSynced" state. (furszy) ef77fdf GUI: topbar, removing not used block source. (furszy) e9c160a wallet: single loop to calculate the currently required balances. (furszy) 32538ae wallet: Disallow abandon of conflicted txes (furszy) 62cd35f GUI: MN model, remove now unneeded cs_main locks. (furszy) 5658844 wallet: removing unnecessary `mempool.cs` lock from ReacceptWalletTransactions() (furszy) 63f3fa7 GUI: add missing qt metatype declarations. (furszy) bc76186 wallet model: update delay increased to 5 seconds. (furszy) 3fb3f34 GUI dashboard: do not update the staking filter if it's not needed. (furszy) 7cb8276 init: move "Done loading" message and rpc warm up finished after the wallet post initialization process (furszy) 8762e81 Refactoring with QString::toNSString (Hennadii Stepanov) 69b3330 GUI: txmodel, do not lock cs_wallet if no wtx status update is needed. (furszy) b4ab286 GUI: dashboard, charts update delay time increased for IBD. (furszy) 0f197ca Wallet interface: do not load not used cold staking balances. (furszy) dee4224 Wallet:GetLockedCoins() loop only over the locked coins, not over the entire wallet txes map. (furszy) 0a61f7f GUI: cold staking, do not refresh delegations if the screen is not visible. (furszy) c2d66d0 GUI: cold staking screen, do not initialize coin control dialog at startup. (furszy) 44d9cbe GUI: dashboard screen, remove stakes filter source model if chart is not being presented. (furszy) 6e49a11 GUI: send screen, initialize coinControlDialog view only when it's being called. (furszy) 379e5f2 GUI: send screen, move refresh amounts calculation to a background worker thread. (furszy) 76bc08b GUI: balance polling update moved to a background worker thread. (furszy) b73500a wallet:BlockConnected do not lock cs_wallet for its entire process. (furszy) 208a292 wallet: remove last cs_main locks from every signal handler function :) . (furszy) 9720579 SSPKM: remove redundant ReadBlockFromDisk from IncrementNoteWitnesses. (furszy) 4ccec74 wallet: Add IsSpent() cs_wallet lock assertion. (furszy) e0a0d2d sapling_wallet_tests: locking order refactor, solving inconsistent orders for cs_main and cs_wallet. (furszy) c69e7e8 Adapt sapling_wallet_listreceived.py to the new wtx confirmation structure. (furszy) c4952a2 Wallet::CreateWalletFromFile guard direct chainActive access (furszy) f889dcb test : updating wallet's last block processed manually. (furszy) 45c9471 wallet: split CheckTXAvailability in two. (furszy) 5109c8d Wallet: remove cs_main from IsSaplingSpent() and GetFilteredNotes() (furszy) a7f6ab1 Wallet: remove cs_main requirement from RelayWalletTransaction and FundTransaction. (furszy) 1e7ffc2 Wallet: remove cs_main requirement from CreateCoinStake (furszy) 2e7cdb2 Wallet: added max value out filter for AvailableCoins. (furszy) b9220f4 Wallet: Do not add P2CS utxo to autocombine flow and discard them later. (furszy) 59ed47d Wallet: remove cs_main lock from AutoCombineDust plus a redundant maturity check. (furszy) fcc4c83 Move AutoCombineDust functionality to the wallet background thread (furszy) fcb20c2 GUI: remove cs_main lock dependency from CWalletModel::isSpent, CWalletModel::isLockedCoin, CWalletModel::lockCoin, CWalletModel::unlockCoin (furszy) 65fbad1 GUI: remove cs_main lock dependency from transaction model update. (furszy) 0b61857 GUI: fix coinstake tx ordering. (furszy) 33588fe GUI: Remove cs_main lock and chain dependency from transaction update status (furszy) 3dede64 GUI: Remove cs_main lock from balance polling timer (furszy) 5e06330 Removed IsFinalTx() cs_main lock requirement. (furszy) 1bd97ca wallet::MarkConflicted remove blockIndex and there by cs_main lock dependency. (furszy) 239d6a2 wallet: remove the now unneeded cs_main locks (furszy) 1386ab7 Use CWallet::m_last_block_processed_height in GetDepthInMainChain (furszy) 9adeb61 Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (furszy) 8aa2d31 Add block_height field in struct Confirmation (furszy) 4405ac0 Replace CWalletTx::SetConf by Confirmation initialization list (furszy) 6320efb Update wallet last processed index in every unit test that needs it (furszy) 4957051 wallet: cache block hash, height and time inside the wallet. (furszy) 33f4788 wallet: refactor GetDepthInMainChain to not return the block index. (furszy) e7c8ca6 wallet: remove unused IsInMainChain method (furszy) 9e51a48 Add a test wallet_reorgsrestore (Antoine Riard) 370c200 wallet: simplifying pindexRescan set + added an AssertLockHeld on FindForkInGlobalIndex (furszy) 46f0e30 Fixing reindex problem, use mapBlockIndex.find() and not mapBlockIndex[] (furszy) da78039 [Wallet] Adapting TransactionAddedToMempool and BlockDisconnected to the new wtx confirmation status (furszy) ff04fa6 Modify wallet tx status if has been reorged out (furszy) f7baeaf Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard) 8d8928e Encapsulate tx status in a Confirmation struct (Antoine Riard) Pull request description: 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. ACKs for top commit: Fuzzbawls: All good now, ACK ca3edc5 random-zebra: ACK ca3edc5 and merging... Tree-SHA512: 6d4268730941822942b0df0aab200683a1fabaf6801618cf34955b43b29bc2beb694567635f731a724abf7b73cec3edfe506e0428de6710c0fcf603613f5614a
random-zebra
added a commit
that referenced
this pull request
Apr 4, 2021
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Concluding with the validation <--> wallet asynchronous signal processing work started in #2082, #2118, #2150, #2192, #2195.
Effectively moving every validation interface callback to a background thread without locking
cs_mainfor its entire process (each handler can now requestcs_mainlock only when/if they need it).This has a direct performance improvement on the synchronization time (which i haven't measured yet because there is one/two more PRs over the wallet and GUI areas, probably large as well, on top of this one and #2201 that should boost the sync time a lot more).
Containing the following changes:
cs_mainlock acquisitions as well.