From 01f45dd00eb032a19d142026e4d019944192da19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Sat, 2 Nov 2019 16:14:36 +0000 Subject: [PATCH 1/2] wallet: Avoid recursive lock in CWallet::SetUsedDestinationState --- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 069ae578789..d37b3eaef0c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -693,13 +693,13 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash) void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool used) { + AssertLockHeld(cs_wallet); const CWalletTx* srctx = GetWalletTx(hash); if (!srctx) return; CTxDestination dst; if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) { if (IsMine(dst)) { - LOCK(cs_wallet); if (used && !GetDestData(dst, "used", nullptr)) { AddDestData(dst, "used", "p"); // p for "present", opposite of absent (null) } else if (!used && GetDestData(dst, "used", nullptr)) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f3b791441c9..64c22939225 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -795,7 +795,7 @@ public: // Whether this or any UTXO with the same CTxDestination has been spent. bool IsUsedDestination(const CTxDestination& dst) const; bool IsUsedDestination(const uint256& hash, unsigned int n) const; - void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used); + void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; From 0b75a7f0680d16a41043864a897470324917b1e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Sat, 2 Nov 2019 16:20:45 +0000 Subject: [PATCH 2/2] wallet: Reuse existing batch in CWallet::SetUsedDestinationState --- src/interfaces/wallet.cpp | 6 ++++-- src/wallet/test/wallet_tests.cpp | 7 ++++--- src/wallet/wallet.cpp | 16 ++++++++-------- src/wallet/wallet.h | 6 +++--- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index b6ede08b14f..3e6bb16c4f9 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -170,12 +170,14 @@ public: bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override { LOCK(m_wallet->cs_wallet); - return m_wallet->AddDestData(dest, key, value); + WalletBatch batch{m_wallet->GetDatabase()}; + return m_wallet->AddDestData(batch, dest, key, value); } bool eraseDestData(const CTxDestination& dest, const std::string& key) override { LOCK(m_wallet->cs_wallet); - return m_wallet->EraseDestData(dest, key); + WalletBatch batch{m_wallet->GetDatabase()}; + return m_wallet->EraseDestData(batch, dest, key); } std::vector getDestValues(const std::string& prefix) override { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 72e1b4e83ba..4d11d3bee95 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -338,9 +338,10 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests) { CTxDestination dest = PKHash(); LOCK(m_wallet.cs_wallet); - m_wallet.AddDestData(dest, "misc", "val_misc"); - m_wallet.AddDestData(dest, "rr0", "val_rr0"); - m_wallet.AddDestData(dest, "rr1", "val_rr1"); + WalletBatch batch{m_wallet.GetDatabase()}; + m_wallet.AddDestData(batch, dest, "misc", "val_misc"); + m_wallet.AddDestData(batch, dest, "rr0", "val_rr0"); + m_wallet.AddDestData(batch, dest, "rr1", "val_rr1"); auto values = m_wallet.GetDestValues("rr"); BOOST_CHECK_EQUAL(values.size(), 2U); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d37b3eaef0c..2909019d9b5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -691,7 +691,7 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash) return success; } -void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool used) +void CWallet::SetUsedDestinationState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used) { AssertLockHeld(cs_wallet); const CWalletTx* srctx = GetWalletTx(hash); @@ -701,9 +701,9 @@ void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) { if (IsMine(dst)) { if (used && !GetDestData(dst, "used", nullptr)) { - AddDestData(dst, "used", "p"); // p for "present", opposite of absent (null) + AddDestData(batch, dst, "used", "p"); // p for "present", opposite of absent (null) } else if (!used && GetDestData(dst, "used", nullptr)) { - EraseDestData(dst, "used"); + EraseDestData(batch, dst, "used"); } } } @@ -734,7 +734,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) // Mark used destinations for (const CTxIn& txin : wtxIn.tx->vin) { const COutPoint& op = txin.prevout; - SetUsedDestinationState(op.hash, op.n, true); + SetUsedDestinationState(batch, op.hash, op.n, true); } } @@ -3410,20 +3410,20 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const return nTimeSmart; } -bool CWallet::AddDestData(const CTxDestination &dest, const std::string &key, const std::string &value) +bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value) { if (boost::get(&dest)) return false; mapAddressBook[dest].destdata.insert(std::make_pair(key, value)); - return WalletBatch(*database).WriteDestData(EncodeDestination(dest), key, value); + return batch.WriteDestData(EncodeDestination(dest), key, value); } -bool CWallet::EraseDestData(const CTxDestination &dest, const std::string &key) +bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key) { if (!mapAddressBook[dest].destdata.erase(key)) return false; - return WalletBatch(*database).EraseDestData(EncodeDestination(dest), key); + return batch.EraseDestData(EncodeDestination(dest), key); } void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 64c22939225..525cf432908 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -795,7 +795,7 @@ public: // Whether this or any UTXO with the same CTxDestination has been spent. bool IsUsedDestination(const CTxDestination& dst) const; bool IsUsedDestination(const uint256& hash, unsigned int n) const; - void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void SetUsedDestinationState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; @@ -820,9 +820,9 @@ public: bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } //! Adds a destination data tuple to the store, and saves it to disk - bool AddDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Erases a destination data tuple in the store and on disk - bool EraseDestData(const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool EraseDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a destination data tuple to the store, without saving it to disk void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Look up a destination data tuple in the store, return true if found false otherwise