Add missing wallet locks:
GetConflicts(...) requires holding the mutex cs_walletIsSpent(...) requires holding the mutex cs_walletmapKeys and mapCryptedKeys requires holding the mutex cs_KeyStorenTimeFirstKey requires holding the mutex cs_walletmapWallet requires holding the mutex cs_walletnTimeFirstKey requires holding the mutex cs_wallet3959@@ -3953,6 +3960,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3960 for (const CWalletTx& wtxOld : vWtx)
3961 {
3962 uint256 hash = wtxOld.GetHash();
3963+ LOCK(walletInstance->cs_wallet);
Either:
mapWallet.find(hash) for all vWtx and then loop again without the lock to walletdb.WriteTx();
3956@@ -3950,6 +3957,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3957 {
3958 CWalletDB walletdb(*walletInstance->dbw);
3959
3960+ LOCK(walletInstance->cs_wallet);
Now moved before the line above. Without that lock we’ll trigger Clang thread safety analysis warnings when #11634 is merged due to:
0src/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!).
Added another commit with two more missing locks:
IsSpent requires holding mutex pwallet->cs_wallet exclusivelynWalletVersion, nWalletMaxVersion, nOrderPosNext and nTimeFirstKey require holding mutex cs_wallet
3995@@ -3991,8 +3996,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3996
3997 // No need to read and scan block if block was created before
3998 // our wallet birthday (as adjusted for block time variability)
3999- while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
4000- pindexRescan = chainActive.Next(pindexRescan);
4001+ {
4002+ LOCK(walletInstance->cs_wallet);
@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:
cs_walletcs_mainThis is a potential deadlock given that the opposite lock order is used extensively:
0$ git grep "LOCK2(cs_main,.*cs_wallet);" | wc -l
189
Please advice :-)
788@@ -789,18 +789,21 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
789
790 void SetNull()
791 {
792- nWalletVersion = FEATURE_BASE;
793- nWalletMaxVersion = FEATURE_BASE;
794+ {
795+ LOCK(cs_wallet);
GUARDED_BY/EXCLUSIVE_LOCKS_REQUIRED annotations. Please re-review :-)
3937@@ -3930,6 +3938,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
3938 int64_t nStart = GetTimeMillis();
3939 bool fFirstRun = true;
3940 CWallet *walletInstance = new CWallet(name, CWalletDBWrapper::Create(path));
3941+ LOCK(walletInstance->cs_wallet);
3942 DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
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?
Rebased!
Having this merged would have catched this locking incident: https://github.com/bitcoin/bitcoin/pull/12565/files#r171235800
Reviews welcome! Perhaps @promag or @Sjors could take a look? :-)
1836@@ -1837,7 +1837,10 @@ std::set<uint256> CWalletTx::GetConflicts() const
1837 if (pwallet != nullptr)
1838 {
1839 uint256 myHash = GetHash();
1840- result = pwallet->GetConflicts(myHash);
1841+ {
1842+ LOCK(pwallet->cs_wallet);
1934@@ -1935,6 +1935,7 @@ CAmount CWalletTx::GetAvailableCredit(bool fUseCache) const
1935
1936 CAmount nCredit = 0;
1937 uint256 hashTx = GetHash();
1938+ LOCK(pwallet->cs_wallet);
GetAvailableCredit already have the lock.
pcoin->pwallet->cs_wallet == pwallet->cs_wallet guaranteed?
EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) on CWalletTx::GetAvailableCredit when compiling with Clang’s thread-safety analysis enabled?
1978@@ -1978,6 +1979,7 @@ CAmount CWalletTx::GetAvailableWatchOnlyCredit(const bool fUseCache) const
1979 return nAvailableWatchCreditCached;
1980
1981 CAmount nCredit = 0;
1982+ LOCK(pwallet->cs_wallet);
GetAvailableWatchOnlyCredit already have the lock.
707@@ -708,7 +708,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
708 int64_t m_max_keypool_index = 0;
709 std::map<CKeyID, int64_t> m_pool_key_to_index;
710
711- int64_t nTimeFirstKey = 0;
712+ int64_t nTimeFirstKey GUARDED_BY(cs_wallet);
318@@ -319,7 +319,7 @@ bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigne
319 * Update wallet first key creation time. This should be called whenever keys
320 * are added to the wallet, with the oldest key creation time.
321 */
322-void CWallet::UpdateTimeFirstKey(int64_t nCreateTime)
323+void CWallet::UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
.h files where possible. Could you please re-review? :-)
GetConflicts has still an annotation in the cpp file?
Applying …
0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
1index a74efb919..f2926fd74 100644
2--- a/src/wallet/wallet.cpp
3+++ b/src/wallet/wallet.cpp
4@@ -1865,7 +1865,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
5 return false;
6 }
7
8-std::set<uint256> CWalletTx::GetConflicts() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
9+std::set<uint256> CWalletTx::GetConflicts() const
10 {
11 std::set<uint256> result;
12 if (pwallet != nullptr)
13diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
14index d1ffcfbdc..fa4ca5ccb 100644
15--- a/src/wallet/wallet.h
16+++ b/src/wallet/wallet.h
17@@ -493,7 +493,7 @@ public:
18 /** Pass this transaction to the mempool. Fails if absolute fee exceeds absurd fee. */
19 bool AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state);
20
21- std::set<uint256> GetConflicts() const;
22+ std::set<uint256> GetConflicts() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet);
23 };
24
25 class COutput
… results in …
0In file included from wallet/feebumper.cpp:6:
1In file included from ./wallet/coincontrol.h:11:
2./wallet/wallet.h:496:76: error: member access into incomplete type 'const CWallet'
3 std::set<uint256> GetConflicts() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet);
4 ^
OTOH, applying only …
0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
1index a74efb919..f2926fd74 100644
2--- a/src/wallet/wallet.cpp
3+++ b/src/wallet/wallet.cpp
4@@ -1865,7 +1865,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
5 return false;
6 }
7
8-std::set<uint256> CWalletTx::GetConflicts() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
9+std::set<uint256> CWalletTx::GetConflicts() const
10 {
11 std::set<uint256> result;
12 if (pwallet != nullptr)
… results in …
0wallet/wallet.cpp:1874:27: error: calling function 'GetConflicts' requires holding mutex 'pwallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
1 result = pwallet->GetConflicts(myHash);
2 ^
@MarcoFalke Another alternative could be to add NO_THREAD_SAFETY_ANALYSIS here to disable the analysis locally.
What would you suggest as the recommended way to proceed?
NO_THREAD_SAFETY_ANALYSIS). Make sure to include a comment to explain this is only temporary. Also, explain that this is safe to do, since we have a run-time AssertLockHeld in place.
NO_THREAD_SAFETY_ANALYSIS. Please review :-)
LOCKs, the second one the annotations?
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
* AddKeyPubKeyWithDB(...) reads encrypted_batch which potentially races with write in the same method.
* IncOrderPosNext(...) reads nOrderPosNext which potentially races with write in BlockDisconnected(...).
* LoadKeyPool(...) reads m_max_keypool_index which potentially races with write in BlockDisconnected(...).
* LoadMinVersion(...) reads nWalletMaxVersion which potentially races with write in BlockDisconnected(...).
Updated!
Added GUARDED_BY(cs_wallet) annotations also for encrypted_batch, nWalletMaxVersion, m_max_keypool_index and nOrderPosNext.
Rationale:
AddKeyPubKeyWithDB(...) reads encrypted_batch which potentially races with write in the same methodIncOrderPosNext(...) reads nOrderPosNext which potentially races with write in BlockDisconnected(...)LoadKeyPool(...) reads m_max_keypool_index which potentially races with write in BlockDisconnected(...)LoadMinVersion(...) reads nWalletMaxVersion which potentially races with write in BlockDisconnected(...)Please review :-)
Added GUARDED_BY(cs_wallet) for setExternalKeyPool, mapKeyMetadata, m_script_metadata and setLockedCoins.
Rationale:
0$ git grep -E 'AssertLockHeld.*(setExternalKeyPool|mapKeyMetadata|m_script_metadata|setLockedCoins)' | sort | uniq -c
1 4 src/wallet/wallet.cpp: AssertLockHeld(cs_wallet); // mapKeyMetadata
2 1 src/wallet/wallet.cpp: AssertLockHeld(cs_wallet); // m_script_metadata
3 1 src/wallet/wallet.cpp: AssertLockHeld(cs_wallet); // setExternalKeyPool
4 5 src/wallet/wallet.cpp: AssertLockHeld(cs_wallet); // setLockedCoins
4092@@ -4093,7 +4093,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4093 // Try to top up keypool. No-op if the wallet is locked.
4094 walletInstance->TopUpKeyPool();
4095
4096- LOCK(cs_main);
4097+ LOCK2(cs_main, walletInstance->cs_wallet);
Both cs_main and walletInstance->cs_wallet are required from what I can tell.
Removing that lock results in the following:
0wallet/wallet.cpp:4104:28: error: calling function 'FindForkInGlobalIndex' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
1 pindexRescan = FindForkInGlobalIndex(chainActive, locator);
2 ^
3wallet/wallet.cpp:4131:48: error: reading variable 'nTimeFirstKey' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
4 while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
5 ^
6wallet/wallet.cpp:4131:114: error: reading variable 'nTimeFirstKey' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
7 while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
8 ^
9wallet/wallet.cpp:4156:77: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
10 std::map<uint256, CWalletTx>::iterator mi = walletInstance->mapWallet.find(hash);
11 ^
12wallet/wallet.cpp:4157:43: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
13 if (mi != walletInstance->mapWallet.end())
14 ^
15wallet/wallet.cpp:4181:90: error: calling function 'GetKeyPoolSize' requires holding mutex 'walletInstance->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
16 walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n", walletInstance->GetKeyPoolSize());
17 ^
18wallet/wallet.cpp:4182:90: error: reading variable 'mapWallet' requires holding mutex 'walletInstance->cs_wallet' [-Werror,-Wthread-safety-analysis]
19 walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size());
20 ^
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?
0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
1index 29014790e..2deeb9c42 100644
2--- a/src/wallet/wallet.cpp
3+++ b/src/wallet/wallet.cpp
4@@ -4093,7 +4093,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
5 // Try to top up keypool. No-op if the wallet is locked.
6 walletInstance->TopUpKeyPool();
7
8- LOCK2(cs_main, walletInstance->cs_wallet);
9+ LOCK(cs_main);
10
11 CBlockIndex *pindexRescan = chainActive.Genesis();
12 if (!gArgs.GetBoolArg("-rescan", false))
13@@ -4128,6 +4128,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
14
15 // No need to read and scan block if block was created before
16 // our wallet birthday (as adjusted for block time variability)
17+ LOCK(walletInstance->cs_wallet);
18 while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
19 pindexRescan = chainActive.Next(pindexRescan);
20 }
21@@ -4178,6 +4179,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
22 walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
23
24 {
25+ LOCK(walletInstance->cs_wallet);
26 walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n", walletInstance->GetKeyPoolSize());
27 walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size());
28 walletInstance->WalletLogPrintf("mapAddressBook.size() = %u\n", walletInstance->mapAddressBook.size());
uiInterface.LoadWallet() with the lock held but if it’s not necessary then I wouldn’t change that. The diff looks ok.