Wallet locking fixes for -DDEBUG_LOCKORDER #3704
Conversation
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.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ca4cf5cff6fb60c9769b62acce2e3a8fcd0e7aae for binaries and test log. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
Wallet locking fixes for -DDEBUG_LOCKORDER
3 participants
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.