Document cs_wallet lock and add AssertLockHeld #3401

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2013_12_wallet_mutex changing 5 files +35 −9

laanwj commented at 8:10 AM on December 12, 2013: member
- Document what the wallet lock is protecting (did I get this right?)
  
  /// Main wallet lock. /// This lock protects all the fields added by CWallet /// except for: /// fFileBacked (immutable after instantiation) /// strWalletFile (immutable after instantiation)
- Add locking assertions to wallet to all methods that access internal fields and do not aquire the cs_wallet mutex (and add comment why the lock is needed)
sipa commented at 9:28 PM on December 12, 2013: member

ACK code changes, looks correct. I didn't test.
jgarzik commented at 11:42 PM on December 12, 2013: contributor

untested ACK
laanwj commented at 4:34 AM on December 13, 2013: member
I think I found this is not 100% reliable: ENTER_CRITICAL_SECTION uses the stringified value of the argument passed in as the lock name. This works perfectly for global locks, however the wallet lock object is referred to differently based on the context
```
LOCK(cs_wallet)
LOCK(pwallet->cs_wallet)
LOCK(pwalletMain->cs_wallet)
```
Not sure how this could be avoided...
laanwj commented at 7:33 AM on December 18, 2013: member

I think I know a way to make AssertLockHeld work for non-global locks (also @gavinandresen):

Make AssertLockHeld check the lock stack for the mutex pointer instead of the name.

Does this sound sane?
gavinandresen commented at 8:13 AM on December 19, 2013: contributor

Sure, passing address-of-mutex-object instead of name-of-mutex-object sounds sane.

Use mutex pointer instead of name for AssertLockHeld

This makes it useable for non-global locks such as the wallet and
keystore locks.

19a5676280

Document cs_wallet lock and add AssertLockHeld

Add locking assertions to wallet to all methods that
access internal fields and do not aquire the cs_wallet mutex.

956916806a

LoadWallet: acquire cs_wallet mutex before clearing setKeyPool

Make the function mutex-aware, to prevent having to lock cs_wallet
at the call site in Init.

012ca1c9e4

laanwj commented at 9:01 AM on December 19, 2013: member

Ok, this works now! It finds the problems fixed in #3413.
BitcoinPullTester commented at 9:25 AM on December 19, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/012ca1c9e4f21e0414f965cb812ecb1f2ddb4f67 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
laanwj referenced this in commit 7aedb91476 on Jan 6, 2014
laanwj merged this on Jan 6, 2014
laanwj closed this on Jan 6, 2014
laanwj referenced this in commit a51c46ce24 on Jan 6, 2014
laanwj referenced this in commit c88302fd5e on Jan 6, 2014
laanwj deleted the branch on Jan 6, 2014
laanwj referenced this in commit d31ad26550 on Jan 6, 2014
MathyV referenced this in commit ca0783979b on Aug 5, 2014
DrahtBot locked this on Sep 8, 2021

Contributors