Keystore.cpp: Is locking correctly implemented here? #7901

What happens when we interleave two threads, one calling GetPubKey() and the other RemoveWatchKey()? GetPubKey() has no locks, so if we call RemoveWatchKey() after if (it != mapWatchKeys.end()) //line23 and before vchPubKeyOut = it->second; //line24 there is going to be a problem.

This is true IMO. WatchKeyMap::const_iterator it = mapWatchKeys.find(address); at L22 should only be called while holding cs_KeyStore. I think running into a concurrency issue is very unlikely because if (!GetKey(address, key)) { at L21 takes (and releases though) cs_KeyStore.

Are we sure this is completely fixed? In particular, my reasoning is that key GetPubKey():line30 might not be thread safe, since GetPubKey() uses secp256k1_context_sign:key.cpp:line16, which is a global variable. It seems to me that any concurrent reading and writing will result in a race condition of one kind or another. I might well be wrong about this particular example, but I think we should be sure of the fact that GetPubKey() and whatever it happens to call is thread safe.

secp256k1_context_sign is safe to use without locking as it is immutable after initialization (see src/include/secp256k1/include/secp256k1.h for more information).

The relevant documentation is

• A constructed context can safely be used from multiple threads
• simultaneously, but API call that take a non-const pointer to a context
• need exclusive access to it. In particular this is the case for
• secp256k1_context_destroy and secp256k1_context_randomize.

But it is not acceptable to ignore locking on mapWatchKeys simply because code above it takes and releases a lock. Making a race condition merely unlikely just means that the bugs it would introduce won't show up in testing. :)

Sipa and gmaxwell, thanks for clarifying. Gmaxwell, with the new change from yurizhykin, access to mapWatchKeys lies within the scope of a lock.