Skip to content

Acquire cs_main lock before cs_wallet during wallet initialization #11126

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 28, 2017
Merged

Acquire cs_main lock before cs_wallet during wallet initialization #11126

merged 1 commit into from
Aug 28, 2017

Conversation

ryanofsky
Copy link
Contributor

CWallet::MarkConflicted may acquire the cs_main lock after CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization. (CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet which calls CWallet::MarkConflicted). This is the opposite order that cs_main and cs_wallet locks are acquired in the rest of the code, and so leads to POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.

This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to acquire both locks in the standard order.

Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

@ryanofsky
Acquire cs_main lock before cs_wallet during wallet initialization
de9a1db 
CWallet::MarkConflicted may acquire the cs_main lock after
CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization.
(CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet
which calls CWallet::MarkConflicted). This is the opposite order that cs_main
and cs_wallet locks are acquired in the rest of the code, and so leads to
POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.

This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to
acquire both locks in the standard order. It also fixes some tests that were
acquiring wallet and main locks out of order and failed with the new locking in
CWallet::LoadWallet.

Error was reported by Luke Dashjr <luke-jr@utopios.org> in
https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/
@laanwj laanwj added the Wallet label Aug 24, 2017
@ryanofsky ryanofsky force-pushed the pr/loadlock2 branch 2 times, most recently from cea5319 to fef5c2c Compare August 24, 2017 21:13
jnewbery
jnewbery reviewed Aug 25, 2017
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

Looks good. Tested ACK fef5c2c.

One nit inline.

src/wallet/wallet.cpp
@@ -3107,6 +3107,8 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c

DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
{
LOCK2(cs_main, cs_wallet);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. You can remove the LOCK(cs_wallet); from further down in this function (currently L3118)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 09ee3fa

ryanofsky
ryanofsky commented Aug 25, 2017
View reviewed changes
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Added commit fef5c2c -> 09ee3fa (pr/loadlock2.3 -> pr/loadlock2.4, compare)

Squashed 09ee3fa -> de9a1db (pr/loadlock2.4 -> pr/loadlock2.5)

src/wallet/wallet.cpp
@@ -3107,6 +3107,8 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c

DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
{
LOCK2(cs_main, cs_wallet);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 09ee3fa

@jnewbery
Copy link
Contributor

Tested ACK de9a1db

@laanwj
Copy link
Member

laanwj commented Aug 28, 2017

utACK de9a1db

@laanwj laanwj merged commit de9a1db into bitcoin:master Aug 28, 2017
laanwj added a commit that referenced this pull request Aug 28, 2017
@laanwj
Merge #11126: Acquire cs_main lock before cs_wallet during wallet ini…
df91e11 
…tialization

de9a1db Acquire cs_main lock before cs_wallet during wallet initialization (Russell Yanofsky)

Pull request description:

  `CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`). This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to `POTENTIAL DEADLOCK DETECTED` errors if bitcoin is built with `-DDEBUG_LOCKORDER`.

  This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.

  Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

Tree-SHA512: 353fe21bc0a4a2828b41876897001a3c414d4b115ee7430925bd391d8bc396fca81661145d00996c1ba1a01516d9acf8b89fb5c3da27092f5f3aa7e37ef26ffa
@maflcko maflcko added this to the 0.15.1 milestone Aug 28, 2017
@NicolasDorier NicolasDorier mentioned this pull request Sep 14, 2017
Closed
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017
@ryanofsky
Acquire cs_main lock before cs_wallet during wallet initialization
2cb720a 
CWallet::MarkConflicted may acquire the cs_main lock after
CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization.
(CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet
which calls CWallet::MarkConflicted). This is the opposite order that cs_main
and cs_wallet locks are acquired in the rest of the code, and so leads to
POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.

This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to
acquire both locks in the standard order. It also fixes some tests that were
acquiring wallet and main locks out of order and failed with the new locking in
CWallet::LoadWallet.

Error was reported by Luke Dashjr <luke-jr@utopios.org> in
https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

Github-Pull: bitcoin#11126
Rebased-From: de9a1db
@ryanofsky ryanofsky mentioned this pull request Dec 14, 2017
Closed
@ryanofsky ryanofsky mentioned this pull request Apr 6, 2018
Merged
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 12, 2019
@laanwj @UdjinM6
Merge bitcoin#11126: Acquire cs_main lock before cs_wallet during wal…
32d709f 
…let initialization

de9a1db Acquire cs_main lock before cs_wallet during wallet initialization (Russell Yanofsky)

Pull request description:

  `CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`). This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to `POTENTIAL DEADLOCK DETECTED` errors if bitcoin is built with `-DDEBUG_LOCKORDER`.

  This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.

  Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

Tree-SHA512: 353fe21bc0a4a2828b41876897001a3c414d4b115ee7430925bd391d8bc396fca81661145d00996c1ba1a01516d9acf8b89fb5c3da27092f5f3aa7e37ef26ffa
@UdjinM6 UdjinM6 mentioned this pull request Jul 12, 2019
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 12, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11126: Acquire cs_main lock before cs_wallet during wal…
2f8512b 
…let initialization

de9a1db Acquire cs_main lock before cs_wallet during wallet initialization (Russell Yanofsky)

Pull request description:

  `CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`). This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to `POTENTIAL DEADLOCK DETECTED` errors if bitcoin is built with `-DDEBUG_LOCKORDER`.

  This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.

  Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

Tree-SHA512: 353fe21bc0a4a2828b41876897001a3c414d4b115ee7430925bd391d8bc396fca81661145d00996c1ba1a01516d9acf8b89fb5c3da27092f5f3aa7e37ef26ffa
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@laanwj
Merge bitcoin#11126: Acquire cs_main lock before cs_wallet during wal…
85a82c9 
…let initialization

de9a1db Acquire cs_main lock before cs_wallet during wallet initialization (Russell Yanofsky)

Pull request description:

  `CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`). This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to `POTENTIAL DEADLOCK DETECTED` errors if bitcoin is built with `-DDEBUG_LOCKORDER`.

  This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.

  Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

Tree-SHA512: 353fe21bc0a4a2828b41876897001a3c414d4b115ee7430925bd391d8bc396fca81661145d00996c1ba1a01516d9acf8b89fb5c3da27092f5f3aa7e37ef26ffa
@random-zebra random-zebra mentioned this pull request Aug 1, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 2, 2020
@random-zebra
Merge #1781: [Wallet] Acquire cs_main lock before cs_wallet during wa…
4715915 
…llet initialization

675ab11 Acquire cs_main lock before cs_wallet during wallet initialization (random-zebra)

Pull request description:

  Fixing
  ```
  2020-08-01 09:53:49 POTENTIAL DEADLOCK DETECTED
  2020-08-01 09:53:49 Previous lock order was:
  2020-08-01 09:53:49  (1) cs_main wallet/wallet.cpp:1617 (in thread )
  2020-08-01 09:53:49  (2) cs_wallet wallet/wallet.cpp:1617 (in thread )
  2020-08-01 09:53:49 Current lock order is:
  2020-08-01 09:53:49  (2) pwallet->cs_wallet wallet/walletdb.cpp:708 (in thread )
  2020-08-01 09:53:49  (1) cs_main wallet/wallet.cpp:1013 (in thread )
  ```

  (backports bitcoin#11126)

  > `CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`).
  > This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.
  >
  > This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.

ACKs for top commit:
  furszy:
    nice finding, ACK 675ab11 .
  Fuzzbawls:
    ACK 675ab11

Tree-SHA512: 8f802101bea4b352b52177d79c4c3d2e22666b6859735e27b40f8adbfeaa21acbf0dc8668c60dd50590c165c77532733fc91630edba4be4a118f00b1946205ae
@LarryRuane LarryRuane mentioned this pull request Mar 3, 2021
Merged
LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 13, 2021
@ryanofsky @LarryRuane
Acquire cs_main lock before cs_wallet during wallet initialization
e0a1266 
CWallet::MarkConflicted may acquire the cs_main lock after
CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization.
(CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet
which calls CWallet::MarkConflicted). This is the opposite order that cs_main
and cs_wallet locks are acquired in the rest of the code, and so leads to
POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.

This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to
acquire both locks in the standard order. It also fixes some tests that were
acquiring wallet and main locks out of order and failed with the new locking in
CWallet::LoadWallet.

Error was reported by Luke Dashjr <luke-jr@utopios.org> in
https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/

zcash: cherry picked from commit de9a1db
zcash: bitcoin/bitcoin#11126
zkbot added a commit to zcash/zcash that referenced this pull request Apr 17, 2021
@zkbot
Auto merge of #5017 - LarryRuane:upstream-locking-0.16, r=str4d
0e36226 
Bitcoin 0.16 locking PRs

These are locking changes from upstream (bitcoin core) release 0.16 (Aug 14, 2017, to Feb 16, 2018), oldest to newest (when merged to the master branch).

Each commit also includes a reference both to the PR and the upstream commit.

- bitcoin/bitcoin#11126
  - Excludes changes to wallet tests that we don't have.
- bitcoin/bitcoin#10916
  - first commit only; second commit already merged by d9fcc2b
- bitcoin/bitcoin#11107
  - Only the last commit.
- bitcoin/bitcoin#11593
- bitcoin/bitcoin#11585
- bitcoin/bitcoin#11618
- bitcoin/bitcoin#10286
  - Only the third and last commits.
- bitcoin/bitcoin#11870
- bitcoin/bitcoin#12330
- bitcoin/bitcoin#12366
- bitcoin/bitcoin#12368
- bitcoin/bitcoin#12333
  - Only the first commit.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@jnewbery jnewbery jnewbery left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
0.15.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanofsky @jnewbery @laanwj @maflcko