Fix RPC deadlocks spotted by ArtForz. Unify order in which locks are acquired.
RPC Deadlock fixes in sendfrom, setaccount #136
pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:deadlock-fixes changing 4 files +84 −67-
jgarzik commented at 5:27 PM on March 26, 2011: contributor
-
gavinandresen commented at 9:47 PM on March 28, 2011: contributor
FYI: stress-testing bitcoind with this patch using a testnet-in-a-box I managed to hang one RPC server thread. What I done did:
In one shell window: for i in 1 2 3 4 5 6 7 8 9 10 11; do ~/src/integration_btc/bitcoind -datadir=/Users/gavin/testnet-box/2 sendfrom "" $(~/src/integration_btc/bitcoind -datadir=/Users/gavin/testnet-box/1 getaccountaddress "") $i; done
And in the other: for i in 1 2 3 4 5 6 7 8 9 10 11; do ~/src/integration_btc/bitcoind -datadir=/Users/gavin/testnet-box/1 sendfrom "" $(~/src/integration_btc/bitcoind -datadir=/Users/gavin/testnet-box/2 getaccountaddress "") $i; done
I'm working on modifying the CRITICAL_BLOCK macros to get more information about what is locked, when...
-
jgarzik commented at 10:05 PM on March 28, 2011: contributor
Your hang is verified to be a CRITICAL_BLOCK deadlock?
-
gavinandresen commented at 10:58 PM on March 28, 2011: contributor
Update: yeah, definitely CRITICAL_BLOCK deadlock. Reproduced with a hacked CCritcalSection that spits out its name as it waits/locks/unlocks, and hung process is waiting on sequence cs_main / cs_mapWallet, and I see lots of the other order.
-
gavinandresen commented at 11:34 PM on March 28, 2011: contributor
Definitely missed a few-- see https://gist.github.com/891544
-
jgarzik commented at 7:45 PM on March 31, 2011: contributor
IMO, cs_main should always be acquired before cs_mapWallet, as it is the "bigger" lock. I'll look at how difficult it is to change the code to follow that ordering.
-
f5f1878ba1
Fix deadlocks in setaccount, sendfrom RPC calls
SendMoney*() now requires caller to acquire cs_main. GetAccountAddress() now requires caller to acquire cs_main, cs_mapWallet. Ordering is intended to match these two callchains[1]: 1. CRITICAL_BLOCK(cs_main) ProcessMessage(pfrom, strCommand, vMsg) AddToWalletIfMine() AddToWallet(wtx) CRITICAL_BLOCK(cs_mapWallet) 2. CRITICAL_BLOCK(cs_main) ProcessMessage(pfrom, strCommand, vMsg) AddToWalletIfMine() AddToWallet(wtx) CRITICAL_BLOCK(cs_mapWallet) walletdb.WriteName(PubKeyToAddress(vchDefaultKey), "") CRITICAL_BLOCK(cs_mapAddressBook) Spotted by ArtForz. Additional deadlock fixes by Gavin. [1] http://www.bitcoin.org/smf/index.php?topic=4904.msg71897#msg71897 -
jgarzik commented at 2:28 AM on April 5, 2011: contributor
closing, superceded.
- jgarzik closed this on Apr 5, 2011
- zathras-crypto referenced this in commit 3e81dd44ae on Sep 26, 2014
- glv2 referenced this in commit 4b7b9d59be on Dec 1, 2014
- sipa referenced this in commit ecae2acb06 on Dec 11, 2014
- dexX7 referenced this in commit 2ab7bbe46b on Jul 16, 2015
- TheBlueMatt referenced this in commit 582b2934e6 on Oct 20, 2015
- ptschip referenced this in commit 563daa035e on Oct 27, 2016
- deadalnix referenced this in commit aaba2e0f4b on Jan 19, 2017
- jtimon referenced this in commit 627dfebede on Apr 20, 2017
- destenson referenced this in commit 779894f7fe on Nov 18, 2017
- lateminer referenced this in commit dab13ef531 on Feb 23, 2019
- attilaaf referenced this in commit 68af305140 on Jan 13, 2020
- KolbyML referenced this in commit ca35a68cc7 on Sep 4, 2020
- jonasschnelli referenced this in commit 02b01651c5 on Jan 28, 2021
- MarcoFalke referenced this in commit 3bcd278aa6 on Mar 29, 2021
- rajarshimaitra referenced this in commit a422c7f2f3 on Aug 5, 2021
- DrahtBot locked this on Sep 8, 2021
- DrahtBot added the label CI failed on Apr 11, 2023
- MarcoFalke removed the label CI failed on Apr 11, 2023
Contributors