Skip to content

Commit

Permalink
Merge #949: [Wallet] Lock cs_wallet in additional places.
Browse files Browse the repository at this point in the history
c9cdb22 Lock cs_wallet in additional places. (Zannick)

Pull request description:

  ### Problem
  tsan identifies a data race within WalletBatch, commonly between KeepKey and AddToWallet.

  ### Solution
  Declare CWallet::KeepKey as requiring `cs_wallet`, and take it before all calls as prescribed by clang. (This moved the tsan data race to the nearby fields of CReserveKey, which was mitigated by including their access within the lock.)
  Add another cs_wallet use per clang warning: AddKeyPubKey requires holding mutex 'pwalletParent->cs_wallet' exclusively.

  ### Tested
  Configured with --with-sanitizers=thread, built with clang, and run on regtest. tsan warnings for this area no longer appear.

Tree-SHA512: 07ab9ac13404c270178c5c60bc68289c5d2cb933811ed18bdc2f7556d686ff5f511fe39af4cd923417e76950d010217dd7bee0381ab350c58dffc76c6a09a682
  • Loading branch information
codeofalltrades committed May 26, 2021
2 parents 2eb430d + c9cdb22 commit d87eb85
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 4 deletions.
1 change: 1 addition & 0 deletions src/veil/ringct/anonwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4527,6 +4527,7 @@ bool AnonWallet::AddStealthDestinationMeta(const CKeyID& idStealth, const CKeyID

bool AnonWallet::AddKeyToParent(const CKey& keySharedSecret)
{
LOCK(pwalletParent->cs_wallet);
//Since the new key is not derived through Ext, for now keep it in CWallet keystore
bool fSuccess = true;
if (!pwalletParent->AddKeyPubKey(keySharedSecret, keySharedSecret.GetPubKey())) {
Expand Down
6 changes: 5 additions & 1 deletion src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4647,6 +4647,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co

bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
{
LOCK(pwallet->cs_wallet);
if (nIndex == -1)
{
CKeyPool keypool;
Expand All @@ -4663,14 +4664,17 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)

void CReserveKey::KeepKey()
{
if (nIndex != -1)
LOCK(pwallet->cs_wallet);
if (nIndex != -1) {
pwallet->KeepKey(nIndex);
}
nIndex = -1;
vchPubKey = CPubKey();
}

void CReserveKey::ReturnKey()
{
LOCK(pwallet->cs_wallet);
if (nIndex != -1) {
pwallet->ReturnKey(nIndex, fInternal, vchPubKey);
}
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -1157,7 +1157,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
* or external keypool
*/
bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
void KeepKey(int64_t nIndex);
void KeepKey(int64_t nIndex) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
bool GetKeyFromPool(CPubKey &key, bool internal = false);
int64_t GetOldestKeyPoolTime();
Expand Down Expand Up @@ -1380,8 +1380,8 @@ class CReserveKey final : public CReserveScript
{
protected:
CWallet* pwallet;
int64_t nIndex;
CPubKey vchPubKey;
int64_t nIndex GUARDED_BY(pwallet->cs_wallet);
CPubKey vchPubKey GUARDED_BY(pwallet->cs_wallet);
bool fInternal;
public:
explicit CReserveKey(CWallet* pwalletIn)
Expand Down

0 comments on commit d87eb85

Please sign in to comment.