wallet: Add missing cs_wallet/cs_KeyStore locks to wallet #11634

Either:

• lock once before the loop;
• loop first with the lock to mapWallet.find(hash) for all vWtx and then loop again without the lock to walletdb.WriteTx();
• lock only the mapWallet.find(hash).

This should be before the line above. Anyway, there is no need to lock this wallet since it's a fresh instance, unknown to the remaining system. Unless you are adding the lock because of other asserts and if so please commit them to justify this change?

Now moved before the line above. Without that lock we'll trigger Clang thread safety analysis warnings when #11634 is merged due to:

src/wallet/wallet.h:    std::map<uint256, CWalletTx> mapWallet GUARDED_BY(cs_wallet);

As agreed upon in #11226 (comment) all guard/lock annotations are added in #11634 and all extra locking is submitted as separate PR:s (such as this one).

The annotations are compile-time only whereas the locks change run-time behaviour (and thus needs extra scrunity!).

As agreed upon in #11226 (comment) all guard/lock annotations are added in #11634 and all extra locking is submitted as separate PR:s (such as this one)

I think you swapped the PR numbers?

Yes, you're right - the annotations are added in #11226 :-)

Given this is effectively just the wallet's constructor, might as well just take the lock further up and hold it the whole time.

@TheBlueMatt Moving it further up will make the scope of the walletInstance->cs_wallet lock cover the ScanForWalletTransactions call. ScanForWalletTransactions locks cs_main giving us the lock order:

• 1. cs_wallet
• 2. cs_main

This is a potential deadlock given that the opposite lock order is used extensively:

$ git grep "LOCK2(cs_main,.*cs_wallet);" | wc -l
89

Please advice :-)

A solution to that is to also lock cs_main.. but meh

Same here. Just add a cs_wallet lock for the whole function.

Note that LoadWallet takes LOCK2(cs_main, cs_wallet), so taking cs_wallet before cs_main leads to a deadlock in case another thread takes cs_main and then cs_wallet, no?

Thanks. Now fixed. See #11634 (review).

I believe this lock is not necessary, all callers have the lock already.

Unnecessary, callers of GetAvailableCredit already have the lock.

Is pcoin->pwallet->cs_wallet == pwallet->cs_wallet guaranteed?

Yes, otherwise we have a big problem I think 😛

Unfortunately Clang's thread-safety analysis does not seem to understand that :-(

@promag I think I'm unable to resolve this. Are you able to make this work with EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) on CWalletTx::GetAvailableCredit when compiling with Clang's thread-safety analysis enabled?

Agree with @promag . We wouldn't want to add LOCKs just to please a static analyser that isn't smart enough to figure out the lock is already taken.

Unnecessary, callers of GetAvailableWatchOnlyCredit already have the lock.

@promag See above. In theory you're right, but I've been unable to convince Clang's thread-safety analysis that so is the case in practice. See comment above. I might need some help here :-)

Why remove the initialization?

Ouch, that was not the intention. Now fixed. Thanks for noticing. Please re-review.

All those are redundant to the header

No annotation requires this lock right?

Both cs_main and walletInstance->cs_wallet are required from what I can tell.

Removing that lock results in the following:

wallet/wallet.cpp:4104:28: error: calling function 'FindForkInGlobalIndex' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
            pindexRescan = FindForkInGlobalIndex(chainActive, locator);
                           ^
wallet/wallet.cpp:4131:48: error: reading variable 'nTimeFirstKey' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
        while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
                                               ^
wallet/wallet.cpp:4131:114: error: reading variable 'nTimeFirstKey' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
        while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
                                                                                                                 ^
wallet/wallet.cpp:4156:77: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
                std::map<uint256, CWalletTx>::iterator mi = walletInstance->mapWallet.find(hash);
                                                                            ^
wallet/wallet.cpp:4157:43: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
                if (mi != walletInstance->mapWallet.end())
                                          ^
wallet/wallet.cpp:4181:90: error: calling function 'GetKeyPoolSize' requires holding mutex 'walletInstance->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
        walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n",      walletInstance->GetKeyPoolSize());
                                                                                         ^
wallet/wallet.cpp:4182:90: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
        walletInstance->WalletLogPrintf("mapWallet.size() = %u\n",       walletInstance->mapWallet.size());
                                                                                         ^

Right, the new annotations..

It could make sense to avoid calling uiInterface.LoadWallet() with the lock held.

@promag Ah, now I follow. Suggested fix below. What do you think?

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 29014790e..2deeb9c42 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4093,7 +4093,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
     // Try to top up keypool. No-op if the wallet is locked.
     walletInstance->TopUpKeyPool();

-    LOCK2(cs_main, walletInstance->cs_wallet);
+    LOCK(cs_main);

     CBlockIndex *pindexRescan = chainActive.Genesis();
     if (!gArgs.GetBoolArg("-rescan", false))
@@ -4128,6 +4128,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,

         // No need to read and scan block if block was created before
         // our wallet birthday (as adjusted for block time variability)
+        LOCK(walletInstance->cs_wallet);
         while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
             pindexRescan = chainActive.Next(pindexRescan);
         }
@@ -4178,6 +4179,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
     walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));

     {
+        LOCK(walletInstance->cs_wallet);
         walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n",      walletInstance->GetKeyPoolSize());
         walletInstance->WalletLogPrintf("mapWallet.size() = %u\n",       walletInstance->mapWallet.size());
         walletInstance->WalletLogPrintf("mapAddressBook.size() = %u\n",  walletInstance->mapAddressBook.size());

I think that currently there is no harm in calling uiInterface.LoadWallet() with the lock held but if it's not necessary then I wouldn't change that. The diff looks ok.