Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Oct 24, 2018

Conversation

@practicalswift
Copy link
Member

practicalswift commented Nov 8, 2017

Add missing wallet locks:

  • Calling the function GetConflicts(...) requires holding the mutex cs_wallet
  • Calling the function IsSpent(...) requires holding the mutex cs_wallet
  • Accessing the variables mapKeys and mapCryptedKeys requires holding the mutex cs_KeyStore
  • Accessing the variable nTimeFirstKey requires holding the mutex cs_wallet
  • Accessing the variable mapWallet requires holding the mutex cs_wallet
  • Accessing the variable nTimeFirstKey requires holding the mutex cs_wallet
@fanquake fanquake added the Wallet label Nov 8, 2017
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Nov 8, 2017
src/wallet/wallet.cpp Outdated
@@ -3953,6 +3960,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
for (const CWalletTx& wtxOld : vWtx)
{
uint256 hash = wtxOld.GetHash();
LOCK(walletInstance->cs_wallet);

This comment has been minimized.

@promag

promag Nov 8, 2017 Member

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).
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Nov 8, 2017
@practicalswift
Copy link
Member Author

practicalswift commented Nov 8, 2017

@promag Thanks for reviewing! Feedback addressed. Looks good? :-)

src/wallet/wallet.cpp Outdated
@@ -3950,6 +3957,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
{
CWalletDB walletdb(*walletInstance->dbw);

LOCK(walletInstance->cs_wallet);

This comment has been minimized.

@promag

promag Nov 8, 2017 Member

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?

This comment has been minimized.

@practicalswift

practicalswift Nov 8, 2017 Author Member

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!).

This comment has been minimized.

@promag

promag Nov 8, 2017 Member

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?

This comment has been minimized.

@practicalswift

practicalswift Nov 8, 2017 Author Member

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

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Nov 8, 2017
@promag
Copy link
Member

promag commented Nov 8, 2017

utACK 007fcbf.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017
* Reading the variables mapTxSpends and mapWallet (via IsSpent(...) call) require holding the mutex cs_wallet.
* Reading the variables mapKeys and mapCryptedKeys require holding the mutex cs_KeyStore.
* Reading the variable nTimeFirstKey requires holding the mutex cs_wallet.
* Reading the variable mapWallet requires holding the mutex cs_wallet.

Github-Pull: bitcoin#11634
Rebased-From: 007fcbff2fe5cd4798721963ca7ce5b5d3674626
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Nov 21, 2017
@practicalswift
Copy link
Member Author

practicalswift commented Nov 21, 2017

Added another commit with two more missing locks:

  • calling function IsSpent requires holding mutex pwallet->cs_wallet exclusively
  • writing variable nWalletVersion, nWalletMaxVersion, nOrderPosNext and nTimeFirstKey require holding mutex cs_wallet
@practicalswift
Copy link
Member Author

practicalswift commented Nov 21, 2017

@promag Would you mind re-reviewing? :-)

src/wallet/wallet.cpp Outdated
while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
pindexRescan = chainActive.Next(pindexRescan);
{
LOCK(walletInstance->cs_wallet);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Dec 13, 2017 Contributor

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

This comment has been minimized.

@practicalswift

practicalswift Mar 12, 2018 Author Member

@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
    1. 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 :-)

This comment has been minimized.

@promag

promag Mar 14, 2018 Member

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

src/wallet/wallet.h Outdated
nWalletVersion = FEATURE_BASE;
nWalletMaxVersion = FEATURE_BASE;
{
LOCK(cs_wallet);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Dec 13, 2017 Contributor

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

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch 2 times, most recently Feb 22, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Feb 22, 2018

@TheBlueMatt Thanks for reviewing! Feedback addressed. Please re-review :-)

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Mar 2, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Mar 2, 2018

Fixed build issue. Please re-review :-)

@MarcoFalke
Copy link
Member

MarcoFalke commented Mar 2, 2018

Given that https://github.com/bitcoin/bitcoin/pull/11226/files#diff-12635a58447c65585f51d32b7e04075bR857 is now closed, wouldn't it make sense to add the clang annotations within this commit?

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch 4 times, most recently Mar 10, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Mar 10, 2018

@MarcoFalke @TheBlueMatt @promag Thanks for reviewing. I've now addressed the feedback and added corresponding GUARDED_BY/EXCLUSIVE_LOCKS_REQUIRED annotations. Please re-review :-)

src/wallet/wallet.cpp Outdated
@@ -3930,6 +3938,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
int64_t nStart = GetTimeMillis();
bool fFirstRun = true;
CWallet *walletInstance = new CWallet(name, CWalletDBWrapper::Create(path));
LOCK(walletInstance->cs_wallet);
DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 11, 2018 Member

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?

This comment has been minimized.

@practicalswift

practicalswift Mar 12, 2018 Author Member

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

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch 2 times, most recently Mar 12, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Mar 12, 2018

Please re-review :-)

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Mar 14, 2018
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Aug 26, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Aug 26, 2018

@MarcoFalke Done! Please re-review :-)

@MarcoFalke
Copy link
Member

MarcoFalke commented Aug 27, 2018

utACK 75cb9c068570ec433f6ebc966f0771caa92efe81

@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch Aug 30, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Aug 30, 2018

Rebased!

@MarcoFalke @promag - please re-review your utACK:s :-)

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 21, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14498 (rpcwallet: listsentbyaddress RPC by mrwhythat)
  • #14437 (Refactor: Start to separate wallet from node by ryanofsky)
  • #14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
  • #13756 (wallet: -avoidreuse feature for improved privacy by kallewoof)
  • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

…ax_keypool_index and nOrderPosNext

* 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(...).
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch to 37b2538 Oct 9, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Oct 9, 2018

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 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(...)

Please review :-)

@practicalswift
Copy link
Member Author

practicalswift commented Oct 9, 2018

Added GUARDED_BY(cs_wallet) for setExternalKeyPool, mapKeyMetadata, m_script_metadata and setLockedCoins.

Rationale:

$ git grep -E 'AssertLockHeld.*(setExternalKeyPool|mapKeyMetadata|m_script_metadata|setLockedCoins)' | sort | uniq -c
      4 src/wallet/wallet.cpp:    AssertLockHeld(cs_wallet); // mapKeyMetadata
      1 src/wallet/wallet.cpp:    AssertLockHeld(cs_wallet); // m_script_metadata
      1 src/wallet/wallet.cpp:    AssertLockHeld(cs_wallet); // setExternalKeyPool
      5 src/wallet/wallet.cpp:    AssertLockHeld(cs_wallet); // setLockedCoins
…cript_metadata and setLockedCoins
@practicalswift practicalswift force-pushed the practicalswift:missing-wallet-locks branch to 69e7ee2 Oct 9, 2018
@practicalswift
Copy link
Member Author

practicalswift commented Oct 24, 2018

@MarcoFalke @promag @TheBlueMatt Would you mind reviewing the updated version? :-)

@MarcoFalke
Copy link
Member

MarcoFalke commented Oct 24, 2018

utACK 69e7ee2

MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this pull request Oct 24, 2018
…to wallet

69e7ee2 Add GUARDED_BY(cs_wallet) for setExternalKeyPool, mapKeyMetadata, m_script_metadata and setLockedCoins (practicalswift)
37b2538 Add GUARDED_BY(cs_wallet) for encrypted_batch, nWalletMaxVersion, m_max_keypool_index and nOrderPosNext (practicalswift)
dee4292 wallet: Add Clang thread safety analysis annotations (practicalswift)
1c7e25d wallet: Add missing locks (practicalswift)

Pull request description:

  Add missing wallet locks:

  * Calling the function `GetConflicts(...)` requires holding the mutex `cs_wallet`
  * Calling the function `IsSpent(...)` requires holding the mutex `cs_wallet`
  * Accessing the variables `mapKeys` and `mapCryptedKeys` requires holding the mutex `cs_KeyStore`
  * Accessing the variable `nTimeFirstKey` requires holding the mutex `cs_wallet`
  * Accessing the variable `mapWallet` requires holding the mutex `cs_wallet`
  * Accessing the variable `nTimeFirstKey` requires holding the mutex `cs_wallet`

Tree-SHA512: 8a7b9a4e1f2147e77c04b817617a06304a2e2159148d3eb3514a3c09c41d77ef7e773df6e63880ad9acc026e00690f72d0c51f3f86279177f672d477423accca
@MarcoFalke MarcoFalke merged commit 69e7ee2 into bitcoin:master Oct 24, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Copy link
Member

promag left a comment

utACK 69e7ee2.

@@ -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();

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

This comment has been minimized.

@promag

promag Oct 24, 2018 Member

No annotation requires this lock right?

This comment has been minimized.

@practicalswift

practicalswift Oct 24, 2018 Author Member

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());
                                                                                         ^

This comment has been minimized.

@promag

promag Oct 24, 2018 Member

Right, the new annotations..

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

This comment has been minimized.

@practicalswift

practicalswift Oct 24, 2018 Author Member

@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());

This comment has been minimized.

@promag

promag Oct 24, 2018 Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.