phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-12 04:42:36 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions

Merge #19289: wallet: GetWalletTx and IsMine require cs_wallet lock

Browse source 
b8405b833a wallet: IsChange requires cs_wallet lock (João Barbosa)
d8441f30ff wallet: IsMine overloads require cs_wallet lock (João Barbosa)
a13cafc6c6 wallet: GetWalletTx requires cs_wallet lock (João Barbosa)

Pull request description:

  This change removes some unlock/lock and lock/lock cases regarding `GetWalletTx` and `IsMine` overloads.

ACKs for top commit:
  laanwj:
    Code review ACK b8405b833a
  ryanofsky:
    Code review ACK b8405b833a. Just new commit since last review changing IsChange GetChange locks and adding annotations

Tree-SHA512: 40d37c4fe5d10a1407f57d899d5822bb285633d8dbfad8afcf15a9b41b428ed9971a9a7b1aae84318371155132df3002699a15dab56e004527d50c889829187d
This commit is contained in:
Wladimir J. van der Laan 2020-08-27 16:21:32 +02:00
commit 91af7ef831
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
3 changed files with 45 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/interfaces/wallet.cpp
View file

@ -37,6 +37,7 @@ namespace {
//! Construct wallet tx struct.
WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
{
LOCK(wallet.cs_wallet);
WalletTx result;
result.tx = wtx.tx;
result.txin_is_mine.reserve(wtx.tx->vin.size());
@ -132,7 +133,11 @@ public:
{
return m_wallet->SignMessage(message, pkhash, str_sig);
}
bool isSpendable(const CTxDestination& dest) override { return m_wallet->IsMine(dest) & ISMINE_SPENDABLE; }
bool isSpendable(const CTxDestination& dest) override
{
LOCK(m_wallet->cs_wallet);
return m_wallet->IsMine(dest) & ISMINE_SPENDABLE;
}
bool haveWatchOnly() override
{
auto spk_man = m_wallet->GetLegacyScriptPubKeyMan();

32
src/wallet/wallet.cpp
View file

@ -276,7 +276,7 @@ std::string COutput::ToString() const
const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
{
LOCK(cs_wallet);
AssertLockHeld(cs_wallet);
std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(hash);
if (it == mapWallet.end())
return nullptr;
@ -1210,8 +1210,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() const {
isminetype CWallet::IsMine(const CTxIn &txin) const
{
{
LOCK(cs_wallet);
AssertLockHeld(cs_wallet);
std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(txin.prevout.hash);
if (mi != mapWallet.end())
{
@ -1219,7 +1218,6 @@ isminetype CWallet::IsMine(const CTxIn &txin) const
if (txin.prevout.n < prev.tx->vout.size())
return IsMine(prev.tx->vout[txin.prevout.n]);
}
}
return ISMINE_NO;
}
@ -1243,16 +1241,19 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
isminetype CWallet::IsMine(const CTxOut& txout) const
{
AssertLockHeld(cs_wallet);
return IsMine(txout.scriptPubKey);
}
isminetype CWallet::IsMine(const CTxDestination& dest) const
{
AssertLockHeld(cs_wallet);
return IsMine(GetScriptForDestination(dest));
}
isminetype CWallet::IsMine(const CScript& script) const
{
AssertLockHeld(cs_wallet);
isminetype result = ISMINE_NO;
for (const auto& spk_man_pair : m_spk_managers) {
result = std::max(result, spk_man_pair.second->IsMine(script));
@ -1264,6 +1265,7 @@ CAmount CWallet::GetCredit(const CTxOut& txout, const isminefilter& filter) cons
{
if (!MoneyRange(txout.nValue))
throw std::runtime_error(std::string(__func__) + ": value out of range");
LOCK(cs_wallet);
return ((IsMine(txout) & filter) ? txout.nValue : 0);
}
@ -1281,13 +1283,12 @@ bool CWallet::IsChange(const CScript& script) const
// a better way of identifying which outputs are 'the send' and which are
// 'the change' will need to be implemented (maybe extend CWalletTx to remember
// which output, if any, was change).
AssertLockHeld(cs_wallet);
if (IsMine(script))
{
CTxDestination address;
if (!ExtractDestination(script, address))
return true;
LOCK(cs_wallet);
if (!FindAddressBookEntry(address)) {
return true;
}
@ -1297,6 +1298,7 @@ bool CWallet::IsChange(const CScript& script) const
CAmount CWallet::GetChange(const CTxOut& txout) const
{
AssertLockHeld(cs_wallet);
if (!MoneyRange(txout.nValue))
throw std::runtime_error(std::string(__func__) + ": value out of range");
return (IsChange(txout) ? txout.nValue : 0);
@ -1304,6 +1306,7 @@ CAmount CWallet::GetChange(const CTxOut& txout) const
bool CWallet::IsMine(const CTransaction& tx) const
{
AssertLockHeld(cs_wallet);
for (const CTxOut& txout : tx.vout)
if (IsMine(txout))
return true;
@ -1362,6 +1365,7 @@ CAmount CWallet::GetCredit(const CTransaction& tx, const isminefilter& filter) c
CAmount CWallet::GetChange(const CTransaction& tx) const
{
LOCK(cs_wallet);
CAmount nChange = 0;
for (const CTxOut& txout : tx.vout)
{
@ -1597,6 +1601,7 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
nFee = nDebit - nValueOut;
}
LOCK(pwallet->cs_wallet);
// Sent/received.
for (unsigned int i = 0; i < tx->vout.size(); ++i)
{
@ -1983,6 +1988,7 @@ bool CWalletTx::IsTrusted(std::set<uint256>& trusted_parents) const
if (!InMempool()) return false;
// Trusted if all inputs are from us and are in the mempool:
LOCK(pwallet->cs_wallet);
for (const CTxIn& txin : tx->vin)
{
// Transactions not sent by us: not trusted
@ -3194,6 +3200,7 @@ DBErrors CWallet::ZapWalletTx(std::list<CWalletTx>& vWtx)
bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::string& strPurpose)
{
bool fUpdated = false;
bool is_mine;
{
LOCK(cs_wallet);
std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
@ -3201,8 +3208,9 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
m_address_book[address].SetLabel(strName);
if (!strPurpose.empty()) /* update purpose only if requested */
m_address_book[address].purpose = strPurpose;
is_mine = IsMine(address) != ISMINE_NO;
}
NotifyAddressBookChanged(this, address, strName, IsMine(address) != ISMINE_NO,
NotifyAddressBookChanged(this, address, strName, is_mine,
strPurpose, (fUpdated ? CT_UPDATED : CT_NEW) );
if (!strPurpose.empty() && !batch.WritePurpose(EncodeDestination(address), strPurpose))
return false;
@ -3217,6 +3225,9 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
bool CWallet::DelAddressBook(const CTxDestination& address)
{
bool is_mine;
{
LOCK(cs_wallet);
// If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)
// NOTE: This isn't a problem for sending addresses because they never have any DestData yet!
// When adding new DestData, it should be considered here whether to retain or delete it (or move it?).
@ -3224,10 +3235,6 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __func__, PACKAGE_BUGREPORT);
return false;
}
{
LOCK(cs_wallet);
// Delete destdata tuples associated with address
std::string strAddress = EncodeDestination(address);
for (const std::pair<const std::string, std::string> &item : m_address_book[address].destdata)
@ -3235,9 +3242,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
WalletBatch(*database).EraseDestData(strAddress, item.first);
}
m_address_book.erase(address);
is_mine = IsMine(address) != ISMINE_NO;
}
NotifyAddressBookChanged(this, address, "", IsMine(address) != ISMINE_NO, "", CT_DELETED);
NotifyAddressBookChanged(this, address, "", is_mine, "", CT_DELETED);
WalletBatch(*database).ErasePurpose(EncodeDestination(address));
return WalletBatch(*database).EraseName(EncodeDestination(address));

18
src/wallet/wallet.h
View file

@ -805,7 +805,7 @@ public:
/** Interface for accessing chain state. */
interfaces::Chain& chain() const { assert(m_chain); return *m_chain; }
const CWalletTx* GetWalletTx(const uint256& hash) const;
const CWalletTx* GetWalletTx(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! check whether we are allowed to upgrade (or already support) to the named feature
bool CanSupportFeature(enum WalletFeature wf) const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return nWalletMaxVersion >= wf; }
@ -1051,20 +1051,20 @@ public:
bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
bool GetNewChangeDestination(const OutputType type, CTxDestination& dest, std::string& error);
isminetype IsMine(const CTxDestination& dest) const;
isminetype IsMine(const CScript& script) const;
isminetype IsMine(const CTxIn& txin) const;
isminetype IsMine(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
isminetype IsMine(const CScript& script) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
isminetype IsMine(const CTxIn& txin) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/**
* Returns amount of debit if the input matches the
* filter, otherwise returns 0
*/
CAmount GetDebit(const CTxIn& txin, const isminefilter& filter) const;
isminetype IsMine(const CTxOut& txout) const;
isminetype IsMine(const CTxOut& txout) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
CAmount GetCredit(const CTxOut& txout, const isminefilter& filter) const;
bool IsChange(const CTxOut& txout) const;
bool IsChange(const CScript& script) const;
CAmount GetChange(const CTxOut& txout) const;
bool IsMine(const CTransaction& tx) const;
bool IsChange(const CTxOut& txout) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool IsChange(const CScript& script) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
CAmount GetChange(const CTxOut& txout) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool IsMine(const CTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/** should probably be renamed to IsRelevantToMe */
bool IsFromMe(const CTransaction& tx) const;
CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;