Acquire cs_main lock before cs_wallet during wallet initialization #11126

1. 
   ryanofsky commented at 6:29 PM on August 24, 2017: 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/

2. Acquire cs_main lock before cs_wallet during wallet initialization

   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/

   de9a1db2ed
3. laanwj added the label Wallet on Aug 24, 2017
4. ryanofsky force-pushed on Aug 24, 2017
5. ryanofsky force-pushed on Aug 24, 2017
6. in src/wallet/wallet.cpp:3110 in fef5c2c433 outdated

   3106 | @@ -3107,6 +3107,8 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c
   3107 |  
   3108 |  DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
   3109 |  {
   3110 | +    LOCK2(cs_main, cs_wallet);
   

   ---

   ---

   

   jnewbery commented at 6:32 PM on August 25, 2017:

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

   ---

   

   ryanofsky commented at 7:00 PM on August 25, 2017:

   Done in 09ee3faaf631fb78150c2725775c65ede66d3a77

7. 
   jnewbery commented at 6:35 PM on August 25, 2017: member

   Looks good. Tested ACK fef5c2c433231bbf6baff8ad6747d7579d35fd7c.

   One nit inline.

8. ryanofsky force-pushed on Aug 25, 2017
9. 
   ryanofsky commented at 7:01 PM on August 25, 2017: contributor

   Added commit fef5c2c433231bbf6baff8ad6747d7579d35fd7c -> 09ee3faaf631fb78150c2725775c65ede66d3a77 (pr/loadlock2.3 -> pr/loadlock2.4, compare)

   Squashed 09ee3faaf631fb78150c2725775c65ede66d3a77 -> de9a1db2ed14e0c75ffd82dc031f7ad30c56d195 (pr/loadlock2.4 -> pr/loadlock2.5)

10. 
    jnewbery commented at 7:32 PM on August 25, 2017: member

    Tested ACK de9a1db2ed14e0c75ffd82dc031f7ad30c56d195

11. 
    laanwj commented at 8:56 AM on August 28, 2017: member

    utACK de9a1db

12. laanwj merged this on Aug 28, 2017
13. laanwj closed this on Aug 28, 2017

    ---

14. laanwj referenced this in commit df91e11ae1 on Aug 28, 2017
15. MarcoFalke added this to the milestone 0.15.1 on Aug 28, 2017
16. MarcoFalke added the label Needs backport on Aug 28, 2017
17. NicolasDorier cross-referenced this on Sep 14, 2017 from issue Deadlock detection bogus by NicolasDorier
18. MarcoFalke referenced this in commit 2cb720ae61 on Oct 3, 2017
19. MarcoFalke removed the label Needs backport on Oct 4, 2017
20. ryanofsky cross-referenced this on Dec 14, 2017 from issue Initial wallet loading takes locks in wrong order causing POTENTIAL DEADLOCK DETECTED by morcos
21. ryanofsky cross-referenced this on Apr 6, 2018 from issue Add AssertLockHeld assertions in CWallet::ListCoins by ryanofsky
22. UdjinM6 referenced this in commit 32d709f332 on Jul 12, 2019
23. UdjinM6 cross-referenced this on Jul 12, 2019 from issue Backports 0.15 pr20 by PastaPastaPasta
24. PastaPastaPasta referenced this in commit 2f8512b923 on Jul 12, 2019
25. barrystyle referenced this in commit 85a82c96ba on Jan 22, 2020
26. random-zebra cross-referenced this on Aug 1, 2020 from issue [Wallet] Acquire cs_main lock before cs_wallet during wallet initialization by random-zebra
27. random-zebra referenced this in commit 4715915d4c on Aug 2, 2020
28. LarryRuane referenced this in commit eff86656f7 on Mar 3, 2021
29. LarryRuane cross-referenced this on Mar 3, 2021 from issue Bitcoin 0.16 locking PRs by LarryRuane
30. LarryRuane referenced this in commit e0a1266024 on Apr 13, 2021
31. zkbot referenced this in commit 0e36226271 on Apr 17, 2021
32. bitcoin locked this on Sep 8, 2021

Contributors

ryanofskyjnewberylaanwj

Milestone
0.15.2