From 1c7e25db0c898abc9968ab487b254454b709e628 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Fri, 2 Mar 2018 09:16:20 +0100 Subject: wallet: Add missing locks --- src/wallet/test/psbt_wallet_tests.cpp | 2 ++ src/wallet/wallet.cpp | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index 526f2d983f..34d6e1b87c 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -17,6 +17,8 @@ BOOST_FIXTURE_TEST_SUITE(psbt_wallet_tests, WalletTestingSetup) BOOST_AUTO_TEST_CASE(psbt_updater_test) { + LOCK(m_wallet.cs_wallet); + // Create prevtxs and add to wallet CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION); CTransactionRef prev_tx1; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index afe47d986e..29014790e9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4093,7 +4093,7 @@ std::shared_ptr 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); CBlockIndex *pindexRescan = chainActive.Genesis(); if (!gArgs.GetBoolArg("-rescan", false)) @@ -4178,7 +4178,6 @@ std::shared_ptr 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()); -- cgit v1.2.3 From dee42927c95549633e8d722be4f64b9d55cd3966 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Sun, 26 Aug 2018 21:48:03 +0200 Subject: wallet: Add Clang thread safety analysis annotations --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/wallet.h | 40 +++++++++++++++++++++++++--------------- 2 files changed, 26 insertions(+), 16 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7d0219201e..86c371ffc0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1121,7 +1121,7 @@ struct tallyitem } }; -static UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +static UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pwallet->cs_wallet) { // Minimum confirmations int nMinDepth = 1; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index da326517c0..63b7aeb74b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,7 +460,11 @@ public: CAmount GetDebit(const isminefilter& filter) const; CAmount GetCredit(const isminefilter& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); CAmount GetImmatureCredit(bool fUseCache=true) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); - CAmount GetAvailableCredit(bool fUseCache=true, const isminefilter& filter=ISMINE_SPENDABLE) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct + // annotation "EXCLUSIVE_LOCKS_REQUIRED(cs_main, pwallet->cs_wallet)". The + // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid + // having to resolve the issue of member access into incomplete type CWallet. + CAmount GetAvailableCredit(bool fUseCache=true, const isminefilter& filter=ISMINE_SPENDABLE) const NO_THREAD_SAFETY_ANALYSIS; CAmount GetImmatureWatchOnlyCredit(const bool fUseCache=true) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); CAmount GetChange() const; @@ -492,7 +496,13 @@ public: /** Pass this transaction to the mempool. Fails if absolute fee exceeds absurd fee. */ bool AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - std::set GetConflicts() const; + // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct + // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation + // "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid having to + // resolve the issue of member access into incomplete type CWallet. Note + // that we still have the runtime check "AssertLockHeld(pwallet->cs_wallet)" + // in place. + std::set GetConflicts() const NO_THREAD_SAFETY_ANALYSIS; }; class COutput @@ -609,9 +619,9 @@ private: * mutated transactions where the mutant gets mined). */ typedef std::multimap TxSpends; - TxSpends mapTxSpends; - void AddToSpends(const COutPoint& outpoint, const uint256& wtxid); - void AddToSpends(const uint256& wtxid); + TxSpends mapTxSpends GUARDED_BY(cs_wallet); + void AddToSpends(const COutPoint& outpoint, const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void AddToSpends(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Add a transaction to the wallet, or update it. pIndex and posInBlock should @@ -632,9 +642,9 @@ private: void MarkConflicted(const uint256& hashBlock, const uint256& hashTx); /* Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */ - void MarkInputsDirty(const CTransactionRef& tx); + void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void SyncMetaData(std::pair); + void SyncMetaData(std::pair) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions. * Should be called with pindexBlock and posInBlock if this is for a transaction that is included in a block. */ @@ -653,7 +663,7 @@ private: std::map m_pool_key_to_index; std::atomic m_wallet_flags{0}; - int64_t nTimeFirstKey = 0; + int64_t nTimeFirstKey GUARDED_BY(cs_wallet) = 0; /** * Private version of AddWatchOnly method which does not accept a @@ -709,7 +719,7 @@ public: * if they are not ours */ bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, - const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const; + const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Get a name for this wallet for logging/debugging purposes. */ @@ -739,7 +749,7 @@ public: encrypted_batch = nullptr; } - std::map mapWallet; + std::map mapWallet GUARDED_BY(cs_wallet); typedef std::multimap TxItems; TxItems wtxOrdered; @@ -769,7 +779,7 @@ public: /** * Find non-change parent output. */ - const CTxOut& FindNonChangeParentOutput(const CTransaction& tx, int output) const; + const CTxOut& FindNonChangeParentOutput(const CTransaction& tx, int output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Shuffle and select coins until nTargetValue is reached while avoiding @@ -780,7 +790,7 @@ public: bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector groups, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const; - bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_wallet); std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -856,7 +866,7 @@ public: void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); - void LoadToWallet(const CWalletTx& wtxIn); + void LoadToWallet(const CWalletTx& wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock) override; @@ -1000,7 +1010,7 @@ public: int GetVersion() { LOCK(cs_wallet); return nWalletVersion; } //! Get wallet transactions that conflict with given transaction (spend same outputs) - std::set GetConflicts(const uint256& txid) const; + std::set GetConflicts(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Check if a given transaction has any of its outputs spent by another transaction in the wallet bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -1198,6 +1208,6 @@ public: // Use DummySignatureCreator, which inserts 71 byte signatures everywhere. // NOTE: this requires that all inputs must be in mapWallet (eg the tx should // be IsAllFromMe). -int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false); +int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig = false); #endif // BITCOIN_WALLET_WALLET_H -- cgit v1.2.3 From 37b2538c2df3643f7eab1f6661b9995bdcbf214a Mon Sep 17 00:00:00 2001 From: practicalswift Date: Tue, 9 Oct 2018 12:03:01 +0200 Subject: Add GUARDED_BY(cs_wallet) for encrypted_batch, nWalletMaxVersion, m_max_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(...). --- src/wallet/wallet.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 63b7aeb74b..d2c8116d94 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -601,13 +601,13 @@ private: std::mutex mutexScanning; friend class WalletRescanReserver; - WalletBatch *encrypted_batch = nullptr; + WalletBatch *encrypted_batch GUARDED_BY(cs_wallet) = nullptr; //! the current wallet version: clients below this version are not able to load the wallet int nWalletVersion = FEATURE_BASE; //! the maximum wallet format version: memory-only variable that specifies to what version this wallet may be upgraded - int nWalletMaxVersion = FEATURE_BASE; + int nWalletMaxVersion GUARDED_BY(cs_wallet) = FEATURE_BASE; int64_t nNextResend = 0; int64_t nLastResend = 0; @@ -659,7 +659,7 @@ private: std::set setInternalKeyPool; std::set setExternalKeyPool; std::set set_pre_split_keypool; - int64_t m_max_keypool_index = 0; + int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; std::atomic m_wallet_flags{0}; @@ -754,7 +754,7 @@ public: typedef std::multimap TxItems; TxItems wtxOrdered; - int64_t nOrderPosNext = 0; + int64_t nOrderPosNext GUARDED_BY(cs_wallet) = 0; uint64_t nAccountingEntryNumber = 0; std::map mapAddressBook; -- cgit v1.2.3 From 69e7ee2dd8173597e766262fd9a8caae569ddf5e Mon Sep 17 00:00:00 2001 From: practicalswift Date: Tue, 9 Oct 2018 13:57:46 +0200 Subject: Add GUARDED_BY(cs_wallet) for setExternalKeyPool, mapKeyMetadata, m_script_metadata and setLockedCoins --- src/wallet/wallet.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d2c8116d94..e6e23ab247 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -657,7 +657,7 @@ private: void DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::set setInternalKeyPool; - std::set setExternalKeyPool; + std::set setExternalKeyPool GUARDED_BY(cs_wallet); std::set set_pre_split_keypool; int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; @@ -726,13 +726,13 @@ public: const std::string& GetName() const { return m_name; } void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void MarkPreSplitKeys(); + void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); // Map from Key ID to key metadata. - std::map mapKeyMetadata; + std::map mapKeyMetadata GUARDED_BY(cs_wallet); // Map from Script ID to key metadata (for watch-only keys). - std::map m_script_metadata; + std::map m_script_metadata GUARDED_BY(cs_wallet); typedef std::map MasterKeyMap; MasterKeyMap mapMasterKeys; @@ -759,7 +759,7 @@ public: std::map mapAddressBook; - std::set setLockedCoins; + std::set setLockedCoins GUARDED_BY(cs_wallet); const CWalletTx* GetWalletTx(const uint256& hash) const; -- cgit v1.2.3