Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

ariard · 2019-07-20T06:25:04Z

This change is intended to make the bitcoin node and its rpc, network and gui interfaces more responsive while the wallet is in use. Currently, because the node's cs_main mutex is always locked before the wallet's cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked while the wallet does relatively slow things like creating and listing transactions.

Switching the lock order so cs_main is acquired after cs_wallet allows cs_main to be only locked intermittently while the wallet is doing slow operations, so the node is not blocked waiting for the wallet.

To review the present PR, most of getting right the move is ensuring any LockAssertion in Chain::Lock method is amended as a LOCK(cs_main). And in final commit, check that any wallet code which was previously locking the chain is now calling a method, enforcing the lock taking job. So far the only exception I found is handleNotifications, which should be corrected.

DrahtBot · 2019-07-20T06:53:23Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Avoid translating RPC errors #18699 (wallet: Avoid translating RPC errors by MarcoFalke)
Make g_chainman internal to validation #18698 (Make g_chainman internal to validation by MarcoFalke)
rpc: replace raw pointers with shared_ptrs #18592 (rpc: replace raw pointers with shared_ptrs by brakmic)
wallet: ensure wallet files are not reused across chains #18554 (wallet: ensure wallet files are not reused across chains by mrwhythat)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
Use shared pointers only in validation interface #18354 (Protect wallet by using shared pointers by bvbfan)
rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins #18244 (rpc: fundrawtransaction and walletcreatefundedpsbt respect locks even with manual coin selection by Sjors)
refactor: consolidate sendmany and sendtoaddress code #18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
UI external signer support (e.g. hardware wallet) #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)
gui: Bilingual GUI error messages #16224 (gui: Bilingual GUI error messages by hebasto)
Wallet passive startup #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
[RPC] Include coinbase transactions in receivedby RPCs #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
wallet: always do avoid partial spends if fees are within a specified range #14582 (wallet: always do avoid partial spends if fees are within a specified range by kallewoof)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)
Remove CWalletTx merging logic from AddToWallet #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

ryanofsky · 2019-07-20T11:17:38Z

Great work! I'd suggest changing the PR title to something like "Reverse cs_main, cs_wallet lock order and reduce cs_main locking" to motivate it better and describe the change.

The current title "Remove Chain::Lock interface" and starting sentence "Follow-up in the set of Chain interface refactoring" make this sound mostly like a refactoring. But this is more of a locking change, and a change to make the wallet more asynchronous and event driven than a refactoring change.

practicalswift · 2019-07-22T11:00:58Z

Concept ACK

Excellent work!

maflcko · 2019-07-22T20:43:47Z

Concept ACK. This might help IBD, because cs_main had to be acquired due to the lock order requirement. If it doesn't help IBD, the change will hopefully still speed up the msghand thread because the wallets take the main lock less.

jnewbery · 2019-07-22T21:32:58Z

Big concept ACK! Thanks @ariard

promag · 2019-07-25T02:58:15Z

Concept ACK, mother of god, not locking cs_main!

ryanofsky · 2019-11-05T18:18:14Z

src/wallet/wallet.cpp

Instead of adding an interfaces::Chain::checkFinalTx method, it might be better to call IsFinalTx directly with the wallet's height and time, avoiding going through CheckFinalTx.

This could perform better since it wouldn't require locking cs_main, and could make wallet calls return more internally consistent information when the wallet is catching up to the chain.

re: #16426 (comment)

Instead of adding an interfaces::Chain::checkFinalTx method, it might be better to call IsFinalTx

#17443 is a step in this direction (#17443 (review))

ariard · 2019-11-11T19:33:32Z

Finally rebased after merge of #15931, PR should be ready for review.

Apart of 3efe38d which use the new m_last_block_processed_height to avoid querying the chainstate and introduce some modifications, other commits are pretty straight-forward. It's just taking cs_main inside the Chain implementation instead of using Chain::lock. Lock order is reversed only in last commit f057aed to avoid any deadlock issue.

If you still feel PR is hard to review, I can subsplit easily in another PR.

ryanofsky · 2019-11-11T20:41:37Z

Approach ACK. Code changes here are very simple after #15931.

All the changes before the last commit f057aedf80d8bd6083c2227e42d4887be4c2933b seem straightforward and don't change behavior other than locking cs_main in new places recursively, so all the new locks are no-ops.

Only the last commit f057aedf80d8bd6083c2227e42d4887be4c2933b is the big scary change that removes cs_main locks all over the wallet, leaving us to hope that remaining locking is sufficient and that stretches of wallet code that used to run under cs_main aren't making assumptions about the tip not changing and other wallet threads not running.

I'll review this PR more closely this week. It might be nice to simplify the PR description now that #15931 is merged. I think the description just basically needs to say that interfaces::Chain::Lock is a wrapper around around cs_main, and that this PR changes wallet code to not lock and unlock cs_main directly anymore, and not keep cs_main locked while it locks cs_wallet and does other work. Instead, after this PR, wallet code only locks cs_main indirectly and intermittently when it needs to look up bits of chain information, and never holds onto cs_main while it does other work.

jnewbery · 2019-11-11T21:01:13Z

Restating my concept ACK. I plan to review this fully soon.

Thanks for rebasing this so quickly

ariard · 2019-11-11T22:03:16Z

Only the last commit f057aed is the big scary change that removes cs_main locks all over the wallet, leaving us to hope that remaining locking is sufficient and that stretches of wallet code that used to run under cs_main aren't making assumptions about the tip not changing and other wallet threads not running.

Most of the code making assumptions about the tip is confined in the rescan logic, so I think this one should get the focus, you can grep for all methods fetching tip like getHeight, getBlockHeight getX and check no decision is made on return value of 2 different calls of them. Concerning other wallet threads, it should be cover by wallet lock in itself.

meshcollider · 2019-11-22T19:56:50Z

Concept ACK

src/wallet/rpcdump.cpp

Summary: Remove findPruned and findFork, no more used after 17954. Backport of Core [[bitcoin/bitcoin#16426 | PR16426]] part [4/5] : bitcoin/bitcoin@8411788 Depends on D7936 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta, Fabien Reviewed By: #bitcoin_abc, majcosta, Fabien Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D7937

Summary: This change is intended to make the bitcoin node and its rpc, network and gui interfaces more responsive while the wallet is in use. Currently because the node's cs_main mutex is always locked before the wallet's cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked while the wallet does relatively slow things like creating and listing transactions. This commit only remmove chain lock tacking in wallet code, and invert lock order from cs_main, cs_wallet to cs_wallet, cs_main. must happen at once to avoid any deadlock. Previous commit were only removing Chain::Lock methods to Chain interface and enforcing they take cs_main. Remove LockChain method from CWallet and Chain::Lock interface. Backport of Core [[bitcoin/bitcoin#16426 | PR16426]] part [5/5] : bitcoin/bitcoin@6a72f26 Depends on D7937 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D7938

Add unit test calling CreateWalletFromFile, which isn't currently called from other unit tests, with some basic checks to make sure it rescans and registers for notifications correctly. Motivation for this change was to try to write a test that would fail without the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8 from bitcoin/bitcoin#16426, but succeed with it: https://github.com/bitcoin/bitcoin/blob/ef8c6ca60767cac589d98ca57ee33179608ccda8/src/wallet/wallet.cpp#L3978-L3986 However, writing a full test for the race condition that call prevents isn't possible without the locking changes from #16426. So this PR just adds as much test coverage as is possible now. This new test is also useful for bitcoin/bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates.

This just drops three interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for bitcoin#10102, which doesn't support overloaded methods - Followup from bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin#19195 (comment)), not because it was implemented.

5baa88f test: Remove no longer needed MakeChain calls (Russell Yanofsky) 6965f13 refactor: Replace uses ChainActive() in interfaces/chain.cpp (Russell Yanofsky) 3fbbb9a refactor: Get rid of more redundant chain methods (Russell Yanofsky) Pull request description: This just drops three interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for #10102, which doesn't support overloaded methods - Followup from bitcoin/bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin/bitcoin#19195 (comment)), not because it was implemented. ACKs for top commit: MarcoFalke: re-ACK 5baa88f 🕶 dongcarl: ACK 5baa88f Tree-SHA512: d20a2a8cf8742e6100c6d3a4871ed67f184925712cf9e55d301abee60353bf3f74cd0d46a13be1715d1c2faef72102bb321654d08a128c2ef6880f72b72a6687

…eWallet e1e68b6 test: Fix inconsistent lock order in wallet_tests/CreateWallet (Hennadii Stepanov) cb23fe0 [skip ci] sync: Check precondition in LEAVE_CRITICAL_SECTION() macro (Hennadii Stepanov) c5e3e74 sync: Improve CheckLastCritical() (Hennadii Stepanov) Pull request description: This PR: - fixes #19049 that was caused by #16426 - removes `wallet_tests::CreateWallet` suppression from the `test/sanitizer_suppressions/tsan` The example of the improved `CheckLastCritical()`/`LEAVE_CRITICAL_SECTION()` log (could be got when compiled without the last commit): ``` 2020-09-20T08:34:28.429485Z [test] INCONSISTENT LOCK ORDER DETECTED 2020-09-20T08:34:28.429493Z [test] Current lock order (least recent first) is: 2020-09-20T08:34:28.429501Z [test] 'walletInstance->cs_wallet' in wallet/wallet.cpp:4007 (in thread 'test') 2020-09-20T08:34:28.429508Z [test] 'cs_wallets' in wallet/wallet.cpp:4089 (in thread 'test') ``` Currently, there are other "naked" `LEAVE_CRITICAL_SECTION()` in the code base: https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/rpc/mining.cpp#L698 https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/checkqueue.h#L208 ACKs for top commit: MarcoFalke: review ACK e1e68b6 💂 ryanofsky: Code review ACK e1e68b6. Just trivial rebase and suggested switch to BOOST_CHECK_EXCEPTION since last review vasild: ACK e1e68b6 Tree-SHA512: a627680eac3af4b4c02772473d68322ce8d3811bf6b035d3485ccc97d35755bef933cffabd3f20b126f89e3301eccecec3f769df34415fb7c426c967b6ce36e6

…s/CreateWallet e1e68b6 test: Fix inconsistent lock order in wallet_tests/CreateWallet (Hennadii Stepanov) cb23fe0 [skip ci] sync: Check precondition in LEAVE_CRITICAL_SECTION() macro (Hennadii Stepanov) c5e3e74 sync: Improve CheckLastCritical() (Hennadii Stepanov) Pull request description: This PR: - fixes bitcoin#19049 that was caused by bitcoin#16426 - removes `wallet_tests::CreateWallet` suppression from the `test/sanitizer_suppressions/tsan` The example of the improved `CheckLastCritical()`/`LEAVE_CRITICAL_SECTION()` log (could be got when compiled without the last commit): ``` 2020-09-20T08:34:28.429485Z [test] INCONSISTENT LOCK ORDER DETECTED 2020-09-20T08:34:28.429493Z [test] Current lock order (least recent first) is: 2020-09-20T08:34:28.429501Z [test] 'walletInstance->cs_wallet' in wallet/wallet.cpp:4007 (in thread 'test') 2020-09-20T08:34:28.429508Z [test] 'cs_wallets' in wallet/wallet.cpp:4089 (in thread 'test') ``` Currently, there are other "naked" `LEAVE_CRITICAL_SECTION()` in the code base: https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/rpc/mining.cpp#L698 https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/checkqueue.h#L208 ACKs for top commit: MarcoFalke: review ACK e1e68b6 💂 ryanofsky: Code review ACK e1e68b6. Just trivial rebase and suggested switch to BOOST_CHECK_EXCEPTION since last review vasild: ACK e1e68b6 Tree-SHA512: a627680eac3af4b4c02772473d68322ce8d3811bf6b035d3485ccc97d35755bef933cffabd3f20b126f89e3301eccecec3f769df34415fb7c426c967b6ce36e6

Summary: 7918c1b019a36a8f9aa55daae422c6b6723b2a39 test: Add CreateWalletFromFile test (Russell Yanofsky) Pull request description: Add unit test calling CreateWalletFromFile, which isn't currently called from other unit tests, with some basic checks to make sure it rescans and registers for notifications correctly. Motivation for this change was to try to write a test that would fail without the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8 from bitcoin/bitcoin#16426, but succeed with it: https://github.com/bitcoin/bitcoin/blob/ef8c6ca60767cac589d98ca57ee33179608ccda8/src/wallet/wallet.cpp#L3978-L3986 However, writing a full test for the race condition that call prevents isn't possible without the locking changes from #16426. So this PR just adds as much test coverage as is possible now. This new test is also useful for bitcoin/bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates. --- Backport of Core [[bitcoin/bitcoin#18727 | PR18727]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9066

Move wallet startup code closer to a simple model where the wallet attaches to the chain with a single chain.handleNotifications() call, and just starts passively receiving blocks and mempool notifications from the last update point, instead having to actively rescan blocks and request a mempool snapshot, and deal with the tip changing, and deal with early or stale notifications. Also, stop locking the cs_wallet mutex and registering for validationinterface notifications before the rescan. This was new behavior since 6a72f26 bitcoin#16426 and is not ideal because it stops other wallets and rpcs and the gui from receiving new notifications until after the scan completes. This change is a half-step towards implementing multiwallet parallel scans (bitcoin#11756), since it provides needed locator and birthday timestamp information to the Chain interface, and it rationalizes locking and event ordering in the startup code. The second half of implementing parallel rescans requires moving the ScanForWalletTransactions implementation (which this PR does not touch) from the wallet to the node.

fanquake added Refactoring Wallet labels Jul 20, 2019

ariard changed the title ~~Remove Chain::Lock interface~~ Reverse cs_main, cs_wallet lock order and reduce cs_main locking Jul 20, 2019

DrahtBot added the Needs rebase label Jul 24, 2019

ariard mentioned this pull request Aug 29, 2019

wallet: encapsulate transactions state #16624

Merged

ryanofsky mentioned this pull request Sep 30, 2019

fix: GuessVerificationProgress requires cs_main lock #15997

Closed

ryanofsky mentioned this pull request Oct 10, 2019

Remove GetDepthInMainChain dependency on locked chain interface #15931

Merged

ryanofsky reviewed Nov 5, 2019

View reviewed changes

ryanofsky mentioned this pull request Nov 5, 2019

wallet: Make IsTrusted scan parents recursively #16766

Merged

ariard force-pushed the 2019-07-remove-more-locking-chain branch from a052c37 to f057aed Compare November 11, 2019 19:21

DrahtBot removed the Needs rebase label Nov 11, 2019

ariard force-pushed the 2019-07-remove-more-locking-chain branch from f057aed to 8aea39a Compare November 11, 2019 21:50

ariard force-pushed the 2019-07-remove-more-locking-chain branch 5 times, most recently from 7ef1044 to 0bdbc43 Compare November 12, 2019 18:58

ariard force-pushed the 2019-07-remove-more-locking-chain branch from 0bdbc43 to f59c5c2 Compare November 21, 2019 19:42

ryanofsky reviewed Dec 2, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

maflcko mentioned this pull request Jul 19, 2021

wallet: Reorder locks in dumpwallet to avoid lock order assertion #22492

Merged

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

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Uh oh!

Conversation

ariard commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jul 22, 2019

Uh oh!

maflcko commented Jul 22, 2019

Uh oh!

jnewbery commented Jul 22, 2019

Uh oh!

promag commented Jul 25, 2019

Uh oh!

ryanofsky Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

ariard commented Nov 11, 2019

Uh oh!

ryanofsky commented Nov 11, 2019

Uh oh!

jnewbery commented Nov 11, 2019

Uh oh!

ariard commented Nov 11, 2019

Uh oh!

meshcollider commented Nov 22, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

ariard commented Jul 20, 2019 •

edited

Loading

DrahtBot commented Jul 20, 2019 •

edited

Loading

ryanofsky commented Jul 20, 2019 •

edited

Loading