setAddress.clear() without a lock in CCryptoKeyStore::GetKeys and CBasicKeyStore::GetKeys

[lyle-nel]

Describe the issue

CCryptoKeyStore::GetKeys at line 173 of file src/wallet/crypter.h and
CBasicKeyStore::GetKeys at line 75 of file src/keystore.h call setAddress.clear(); without a lock.

What version of bitcoin-core are you using?

Master branch:0c173a15ca1bf20999f74987988985508c9de463

[benma]

If it needs to be locked, then at the callsite, not in the caller.

However, it doesn't need to be locked. The only callsite is wallet.cpp:3664, where the variable is a local stack variable, so there is no thread-safety issue.

The setAddress.clear(); line doesn't do anything meaningful either (the passed set is already empty). We could think about returning the result directly instead of passing it by reference to avoid the clear() and also to make it more clear that locking isn't needed there.

Edit: a lock seems to be missing though from crypter.h, GetKeys(), but not for setAddress.clear(), but for looping through mapCryptedKeys.