Wallet locking fixes for -DDEBUG_LOCKORDER #3704

1. 
   gavinandresen commented at 5:17 pm on February 18, 2014: contributor

   Compiling with -DDEBUG_LOCKORDER and running the qa/rpc-test/ regression tests uncovered a couple of wallet methods that should (but didn’t) acquire the cs_wallet mutext.

   I also changed the AssertLockHeld() routine print to stderr and abort, instead of printing to debug.log and then assert()‘ing. It is annoying to look in debug.log to find out which AssertLockHeld is failing.

2. Wallet locking fixes for -DDEBUG_LOCKORDER

   Compiling with -DDEBUG_LOCKORDER and running the qa/rpc-test/ regression
   tests uncovered a couple of wallet methods that should (but didn't)
   acquire the cs_wallet mutext.
   
   I also changed the AssertLockHeld() routine print to stderr and
   abort, instead of printing to debug.log and then assert()'ing.
   It is annoying to look in debug.log to find out which
   AssertLockHeld is failing.

   ca4cf5cff6
3. 
   BitcoinPullTester commented at 5:45 pm on February 18, 2014: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ca4cf5cff6fb60c9769b62acce2e3a8fcd0e7aae 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.
4. in src/wallet.cpp: in ca4cf5cff6

   1628-    mapAddressBook[address].name = strName;
   1629-    if (!strPurpose.empty()) /* update purpose only if requested */
   1630-        mapAddressBook[address].purpose = strPurpose;
   1631+    bool fUpdated = false;
   1632+    {
   1633+        LOCK(cs_wallet); // mapAddressBook
   

   ---

   ---

   

   laanwj commented at 8:15 am on February 19, 2014:

   I’ve never known the specific reason why some methods in CWallet acquire the lock and others such as address book manipulation do not (and assume the caller already holds it). But it’s always been that way.

   ---

   

   gavinandresen commented at 6:05 pm on February 19, 2014:

   Ideally cs_wallet and mapWallet would be private members, and all methods would acquire the lock themselves. But ideally we’d completely rewrite the wallet with deterministic key support, multisig/off-device-signing support, etc.

   ---

   

   laanwj commented at 6:56 am on February 20, 2014:

   I’m not speaking about ideally, just about the way things are implemented now :) Will it have effect in any of the caller sites that the locking behavior of SetAddressBook changed? After all the caller sites (if they’re correct) already acquire the locks. I recently fixed some UI functions that forgot to acquire the wallet lock (28352af). I suppose this is not an issue due to the use of recursive mutexes so it doesn’t matter that it acquires/releases another instance of the lock.
5. 
   gavinandresen commented at 7:39 pm on February 24, 2014: contributor

   Merging, because I keep tripping myself up testing other patches without this.

   RE: locks held by caller: indeed, recursive mutexes mean if callers hold the wallet lock, all will be OK.

6. gavinandresen referenced this in commit a16ad1c0f4 on Feb 24, 2014
7. gavinandresen merged this on Feb 24, 2014
8. gavinandresen closed this on Feb 24, 2014

   ---

9. gavinandresen deleted the branch on Mar 13, 2014
10. DrahtBot locked this on Sep 8, 2021

Contributors
gavinandresen BitcoinPullTester laanwj