From b86cd155f6f661052042048aa7cfc2a397afe4f7 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 21 Feb 2020 21:52:52 +0000 Subject: [PATCH 1/7] scripted-diff: Wallet: Rename mapAddressBook to m_address_book Previous versions assumed absence of an entry in mapAddressBook indicated change. This no longer holds true (due to bugs) and will shortly be made intentional. Renaming the field helps ensure that old code using mapAddressBook directly gets checked for necessary rebasing. -BEGIN VERIFY SCRIPT- sed -i -e 's/mapAddressBook/m_address_book/g' $(git grep -l 'mapAddressBook' ./src) -END VERIFY SCRIPT- --- src/interfaces/wallet.cpp | 6 ++--- src/qt/addresstablemodel.cpp | 2 +- src/qt/test/addressbooktests.cpp | 2 +- src/wallet/rpcdump.cpp | 6 ++--- src/wallet/rpcwallet.cpp | 40 ++++++++++++++++---------------- src/wallet/wallet.cpp | 30 ++++++++++++------------ src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 4 ++-- src/wallet/wallettool.cpp | 2 +- 9 files changed, 47 insertions(+), 47 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 645a8709d2..673d5d498f 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -151,8 +151,8 @@ public: std::string* purpose) override { LOCK(m_wallet->cs_wallet); - auto it = m_wallet->mapAddressBook.find(dest); - if (it == m_wallet->mapAddressBook.end()) { + auto it = m_wallet->m_address_book.find(dest); + if (it == m_wallet->m_address_book.end()) { return false; } if (name) { @@ -170,7 +170,7 @@ public: { LOCK(m_wallet->cs_wallet); std::vector result; - for (const auto& item : m_wallet->mapAddressBook) { + for (const auto& item : m_wallet->m_address_book) { result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.name, item.second.purpose); } return result; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 3ac98a5970..7794a91beb 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -55,7 +55,7 @@ struct AddressTableEntryLessThan static AddressTableEntry::Type translateTransactionType(const QString &strPurpose, bool isMine) { AddressTableEntry::Type addressType = AddressTableEntry::Hidden; - // "refund" addresses aren't shown, and change addresses aren't in mapAddressBook at all. + // "refund" addresses aren't shown, and change addresses aren't in m_address_book at all. if (strPurpose == "send") addressType = AddressTableEntry::Sending; else if (strPurpose == "receive") diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 0f082802cc..042387286a 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -97,7 +97,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) auto check_addbook_size = [&wallet](int expected_size) { LOCK(wallet->cs_wallet); - QCOMPARE(static_cast(wallet->mapAddressBook.size()), expected_size); + QCOMPARE(static_cast(wallet->m_address_book.size()), expected_size); }; // We should start with the two addresses we added earlier and nothing else. diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index e4d0a3fa6d..1348f6091b 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -60,12 +60,12 @@ static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWall CKey key; spk_man->GetKey(keyid, key); for (const auto& dest : GetAllDestinationsForKey(key.GetPubKey())) { - if (pwallet->mapAddressBook.count(dest)) { + if (pwallet->m_address_book.count(dest)) { if (!strAddr.empty()) { strAddr += ","; } strAddr += EncodeDestination(dest); - strLabel = EncodeDumpString(pwallet->mapAddressBook.at(dest).name); + strLabel = EncodeDumpString(pwallet->m_address_book.at(dest).name); fLabelFound = true; } } @@ -168,7 +168,7 @@ UniValue importprivkey(const JSONRPCRequest& request) // label all new addresses, and label existing addresses if a // label was passed. for (const auto& dest : GetAllDestinationsForKey(pubkey)) { - if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest) == 0) { + if (!request.params[1].isNull() || pwallet->m_address_book.count(dest) == 0) { pwallet->SetAddressBook(dest, strLabel, "receive"); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5d34e592db..7a67163a85 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -505,8 +505,8 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) addressInfo.push_back(EncodeDestination(address)); addressInfo.push_back(ValueFromAmount(balances[address])); { - if (pwallet->mapAddressBook.find(address) != pwallet->mapAddressBook.end()) { - addressInfo.push_back(pwallet->mapAddressBook.find(address)->second.name); + if (pwallet->m_address_book.find(address) != pwallet->m_address_book.end()) { + addressInfo.push_back(pwallet->m_address_book.find(address)->second.name); } } jsonGrouping.push_back(addressInfo); @@ -1098,13 +1098,13 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle UniValue ret(UniValue::VARR); std::map label_tally; - // Create mapAddressBook iterator + // Create m_address_book iterator // If we aren't filtering, go from begin() to end() - auto start = pwallet->mapAddressBook.begin(); - auto end = pwallet->mapAddressBook.end(); + auto start = pwallet->m_address_book.begin(); + auto end = pwallet->m_address_book.end(); // If we are filtering, find() the applicable entry if (has_filtered_address) { - start = pwallet->mapAddressBook.find(filtered_address); + start = pwallet->m_address_book.find(filtered_address); if (start != end) { end = std::next(start); } @@ -1313,8 +1313,8 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle MaybePushAddress(entry, s.destination); entry.pushKV("category", "send"); entry.pushKV("amount", ValueFromAmount(-s.amount)); - if (pwallet->mapAddressBook.count(s.destination)) { - entry.pushKV("label", pwallet->mapAddressBook.at(s.destination).name); + if (pwallet->m_address_book.count(s.destination)) { + entry.pushKV("label", pwallet->m_address_book.at(s.destination).name); } entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-nFee)); @@ -1330,8 +1330,8 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle for (const COutputEntry& r : listReceived) { std::string label; - if (pwallet->mapAddressBook.count(r.destination)) { - label = pwallet->mapAddressBook.at(r.destination).name; + if (pwallet->m_address_book.count(r.destination)) { + label = pwallet->m_address_book.at(r.destination).name; } if (filter_label && label != *filter_label) { continue; @@ -1355,7 +1355,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle entry.pushKV("category", "receive"); } entry.pushKV("amount", ValueFromAmount(r.amount)); - if (pwallet->mapAddressBook.count(r.destination)) { + if (pwallet->m_address_book.count(r.destination)) { entry.pushKV("label", label); } entry.pushKV("vout", r.vout); @@ -2955,8 +2955,8 @@ static UniValue listunspent(const JSONRPCRequest& request) if (fValidAddress) { entry.pushKV("address", EncodeDestination(address)); - auto i = pwallet->mapAddressBook.find(address); - if (i != pwallet->mapAddressBook.end()) { + auto i = pwallet->m_address_book.find(address); + if (i != pwallet->m_address_book.end()) { entry.pushKV("label", i->second.name); } @@ -3814,8 +3814,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // DEPRECATED: Return label field if existing. Currently only one label can // be associated with an address, so the label should be equivalent to the // value of the name key/value pair in the labels array below. - if ((pwallet->chain().rpcEnableDeprecated("label")) && (pwallet->mapAddressBook.count(dest))) { - ret.pushKV("label", pwallet->mapAddressBook.at(dest).name); + if ((pwallet->chain().rpcEnableDeprecated("label")) && (pwallet->m_address_book.count(dest))) { + ret.pushKV("label", pwallet->m_address_book.at(dest).name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); @@ -3838,8 +3838,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // stable if we allow multiple labels to be associated with an address in // the future. UniValue labels(UniValue::VARR); - std::map::const_iterator mi = pwallet->mapAddressBook.find(dest); - if (mi != pwallet->mapAddressBook.end()) { + std::map::const_iterator mi = pwallet->m_address_book.find(dest); + if (mi != pwallet->m_address_book.end()) { // DEPRECATED: The previous behavior of returning an array containing a // JSON object of `name` and `purpose` key/value pairs is deprecated. if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) { @@ -3889,10 +3889,10 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request) // Find all addresses that have the given label UniValue ret(UniValue::VOBJ); std::set addresses; - for (const std::pair& item : pwallet->mapAddressBook) { + for (const std::pair& item : pwallet->m_address_book) { if (item.second.name == label) { std::string address = EncodeDestination(item.first); - // CWallet::mapAddressBook is not expected to contain duplicate + // CWallet::m_address_book is not expected to contain duplicate // address strings, but build a separate set as a precaution just in // case it does. bool unique = addresses.emplace(address).second; @@ -3953,7 +3953,7 @@ static UniValue listlabels(const JSONRPCRequest& request) // Add to a set to sort by label name, then insert into Univalue array std::set label_set; - for (const std::pair& entry : pwallet->mapAddressBook) { + for (const std::pair& entry : pwallet->m_address_book) { if (purpose.empty() || entry.second.purpose == purpose) { label_set.insert(entry.second.name); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 98f308f927..6737270e52 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1237,7 +1237,7 @@ bool CWallet::IsChange(const CScript& script) const return true; LOCK(cs_wallet); - if (!mapAddressBook.count(address)) + if (!m_address_book.count(address)) return true; } return false; @@ -3191,11 +3191,11 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add bool fUpdated = false; { LOCK(cs_wallet); - std::map::iterator mi = mapAddressBook.find(address); - fUpdated = mi != mapAddressBook.end(); - mapAddressBook[address].name = strName; + std::map::iterator mi = m_address_book.find(address); + fUpdated = mi != m_address_book.end(); + m_address_book[address].name = strName; if (!strPurpose.empty()) /* update purpose only if requested */ - mapAddressBook[address].purpose = strPurpose; + m_address_book[address].purpose = strPurpose; } NotifyAddressBookChanged(this, address, strName, IsMine(address) != ISMINE_NO, strPurpose, (fUpdated ? CT_UPDATED : CT_NEW) ); @@ -3217,11 +3217,11 @@ bool CWallet::DelAddressBook(const CTxDestination& address) // Delete destdata tuples associated with address std::string strAddress = EncodeDestination(address); - for (const std::pair &item : mapAddressBook[address].destdata) + for (const std::pair &item : m_address_book[address].destdata) { WalletBatch(*database).EraseDestData(strAddress, item.first); } - mapAddressBook.erase(address); + m_address_book.erase(address); } NotifyAddressBookChanged(this, address, "", IsMine(address) != ISMINE_NO, "", CT_DELETED); @@ -3457,7 +3457,7 @@ std::set CWallet::GetLabelAddresses(const std::string& label) co { LOCK(cs_wallet); std::set result; - for (const std::pair& item : mapAddressBook) + for (const std::pair& item : m_address_book) { const CTxDestination& address = item.first; const std::string& strName = item.second.name; @@ -3661,26 +3661,26 @@ bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const if (boost::get(&dest)) return false; - mapAddressBook[dest].destdata.insert(std::make_pair(key, value)); + m_address_book[dest].destdata.insert(std::make_pair(key, value)); return batch.WriteDestData(EncodeDestination(dest), key, value); } bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key) { - if (!mapAddressBook[dest].destdata.erase(key)) + if (!m_address_book[dest].destdata.erase(key)) return false; return batch.EraseDestData(EncodeDestination(dest), key); } void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value) { - mapAddressBook[dest].destdata.insert(std::make_pair(key, value)); + m_address_book[dest].destdata.insert(std::make_pair(key, value)); } bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const { - std::map::const_iterator i = mapAddressBook.find(dest); - if(i != mapAddressBook.end()) + std::map::const_iterator i = m_address_book.find(dest); + if(i != m_address_book.end()) { CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key); if(j != i->second.destdata.end()) @@ -3696,7 +3696,7 @@ bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, st std::vector CWallet::GetDestValues(const std::string& prefix) const { std::vector values; - for (const auto& address : mapAddressBook) { + for (const auto& address : m_address_book) { for (const auto& data : address.second.destdata) { if (!data.first.compare(0, prefix.size(), prefix)) { values.emplace_back(data.second); @@ -4098,7 +4098,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, { 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()); + walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size()); } return walletInstance; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fe59773488..c6f288a480 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -775,7 +775,7 @@ public: int64_t nOrderPosNext GUARDED_BY(cs_wallet) = 0; uint64_t nAccountingEntryNumber = 0; - std::map mapAddressBook GUARDED_BY(cs_wallet); + std::map m_address_book GUARDED_BY(cs_wallet); std::set setLockedCoins GUARDED_BY(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index a1928f45c4..2e08a044da 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -206,11 +206,11 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (strType == DBKeys::NAME) { std::string strAddress; ssKey >> strAddress; - ssValue >> pwallet->mapAddressBook[DecodeDestination(strAddress)].name; + ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].name; } else if (strType == DBKeys::PURPOSE) { std::string strAddress; ssKey >> strAddress; - ssValue >> pwallet->mapAddressBook[DecodeDestination(strAddress)].purpose; + ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].purpose; } else if (strType == DBKeys::TX) { uint256 hash; ssKey >> hash; diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index fbfdf9dd6b..8c918f4eb0 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -99,7 +99,7 @@ static void WalletShowInfo(CWallet* wallet_instance) tfm::format(std::cout, "HD (hd seed available): %s\n", wallet_instance->IsHDEnabled() ? "yes" : "no"); tfm::format(std::cout, "Keypool Size: %u\n", wallet_instance->GetKeyPoolSize()); tfm::format(std::cout, "Transactions: %zu\n", wallet_instance->mapWallet.size()); - tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->mapAddressBook.size()); + tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size()); } bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) From 144b2f85da4d51bf7d72b987888ddcaf5b429eed Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 01:52:47 +0000 Subject: [PATCH 2/7] Wallet: Require usage of new CAddressBookData::setLabel to change label --- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 10 ++++++++-- src/wallet/walletdb.cpp | 4 +++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6737270e52..6b061308ec 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3193,7 +3193,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add LOCK(cs_wallet); std::map::iterator mi = m_address_book.find(address); fUpdated = mi != m_address_book.end(); - m_address_book[address].name = strName; + m_address_book[address].SetLabel(strName); if (!strPurpose.empty()) /* update purpose only if requested */ m_address_book[address].purpose = strPurpose; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c6f288a480..47268ba88c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -181,14 +181,20 @@ public: /** Address book data */ class CAddressBookData { +private: + std::string m_label; public: - std::string name; + const std::string& name; std::string purpose; - CAddressBookData() : purpose("unknown") {} + CAddressBookData() : name(m_label), purpose("unknown") {} typedef std::map StringMap; StringMap destdata; + + void SetLabel(const std::string& label) { + m_label = label; + } }; struct CRecipient diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 2e08a044da..568b21ed00 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -206,7 +206,9 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (strType == DBKeys::NAME) { std::string strAddress; ssKey >> strAddress; - ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].name; + std::string label; + ssValue >> label; + pwallet->m_address_book[DecodeDestination(strAddress)].SetLabel(label); } else if (strType == DBKeys::PURPOSE) { std::string strAddress; ssKey >> strAddress; From 65b6bdc2b164343ec3cc3d32a0297daff9e24fec Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 02:04:27 +0000 Subject: [PATCH 3/7] Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set --- src/wallet/wallet.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 47268ba88c..788f29c039 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -182,6 +182,7 @@ public: class CAddressBookData { private: + bool m_change{true}; std::string m_label; public: const std::string& name; @@ -192,7 +193,9 @@ public: typedef std::map StringMap; StringMap destdata; + bool IsChange() const { return m_change; } void SetLabel(const std::string& label) { + m_change = false; m_label = label; } }; From 8e64b8c84bcbd63caea06f3af087af1f0609eaf5 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 02:50:46 +0000 Subject: [PATCH 4/7] Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) --- src/wallet/wallet.cpp | 10 ++++++++++ src/wallet/wallet.h | 1 + 2 files changed, 11 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6b061308ec..ab36dacf37 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4104,6 +4104,16 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, return walletInstance; } +const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest, bool allow_change) const +{ + const auto& address_book_it = m_address_book.find(dest); + if (address_book_it == m_address_book.end()) return nullptr; + if ((!allow_change) && address_book_it->second.IsChange()) { + return nullptr; + } + return &address_book_it->second; +} + void CWallet::postInitProcess() { auto locked_chain = chain().lock(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 788f29c039..c681dd28c8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -785,6 +785,7 @@ public: uint64_t nAccountingEntryNumber = 0; std::map m_address_book GUARDED_BY(cs_wallet); + const CAddressBookData* FindAddressBookEntry(const CTxDestination&, bool allow_change = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::set setLockedCoins GUARDED_BY(cs_wallet); From c751d886f499257627b308b11ffaa51c22db6cc0 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 04:16:36 +0000 Subject: [PATCH 5/7] Wallet: Avoid treating change-in-the-addressbook as non-change everywhere --- src/interfaces/wallet.cpp | 3 ++- src/qt/addresstablemodel.cpp | 2 +- src/wallet/rpcdump.cpp | 5 +++-- src/wallet/rpcwallet.cpp | 30 ++++++++++++++++++------------ src/wallet/wallet.cpp | 6 ++++-- 5 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 673d5d498f..d55b92a5ff 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -152,7 +152,7 @@ public: { LOCK(m_wallet->cs_wallet); auto it = m_wallet->m_address_book.find(dest); - if (it == m_wallet->m_address_book.end()) { + if (it == m_wallet->m_address_book.end() || it->second.IsChange()) { return false; } if (name) { @@ -171,6 +171,7 @@ public: LOCK(m_wallet->cs_wallet); std::vector result; for (const auto& item : m_wallet->m_address_book) { + if (item.second.IsChange()) continue; result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.name, item.second.purpose); } return result; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 7794a91beb..3869318fea 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -55,7 +55,7 @@ struct AddressTableEntryLessThan static AddressTableEntry::Type translateTransactionType(const QString &strPurpose, bool isMine) { AddressTableEntry::Type addressType = AddressTableEntry::Hidden; - // "refund" addresses aren't shown, and change addresses aren't in m_address_book at all. + // "refund" addresses aren't shown, and change addresses aren't returned by getAddresses at all. if (strPurpose == "send") addressType = AddressTableEntry::Sending; else if (strPurpose == "receive") diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 1348f6091b..40ea51e464 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -60,7 +60,8 @@ static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWall CKey key; spk_man->GetKey(keyid, key); for (const auto& dest : GetAllDestinationsForKey(key.GetPubKey())) { - if (pwallet->m_address_book.count(dest)) { + const auto* address_book_entry = pwallet->FindAddressBookEntry(dest); + if (address_book_entry) { if (!strAddr.empty()) { strAddr += ","; } @@ -168,7 +169,7 @@ UniValue importprivkey(const JSONRPCRequest& request) // label all new addresses, and label existing addresses if a // label was passed. for (const auto& dest : GetAllDestinationsForKey(pubkey)) { - if (!request.params[1].isNull() || pwallet->m_address_book.count(dest) == 0) { + if (!request.params[1].isNull() || !pwallet->FindAddressBookEntry(dest)) { pwallet->SetAddressBook(dest, strLabel, "receive"); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7a67163a85..464371a15f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -505,7 +505,8 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) addressInfo.push_back(EncodeDestination(address)); addressInfo.push_back(ValueFromAmount(balances[address])); { - if (pwallet->m_address_book.find(address) != pwallet->m_address_book.end()) { + const auto* address_book_entry = pwallet->FindAddressBookEntry(address); + if (address_book_entry) { addressInfo.push_back(pwallet->m_address_book.find(address)->second.name); } } @@ -1112,6 +1113,7 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle for (auto item_it = start; item_it != end; ++item_it) { + if (item_it->second.IsChange()) continue; const CTxDestination& address = item_it->first; const std::string& label = item_it->second.name; auto it = mapTally.find(address); @@ -1313,7 +1315,8 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle MaybePushAddress(entry, s.destination); entry.pushKV("category", "send"); entry.pushKV("amount", ValueFromAmount(-s.amount)); - if (pwallet->m_address_book.count(s.destination)) { + const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination); + if (address_book_entry) { entry.pushKV("label", pwallet->m_address_book.at(s.destination).name); } entry.pushKV("vout", s.vout); @@ -1330,7 +1333,8 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle for (const COutputEntry& r : listReceived) { std::string label; - if (pwallet->m_address_book.count(r.destination)) { + const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination); + if (address_book_entry) { label = pwallet->m_address_book.at(r.destination).name; } if (filter_label && label != *filter_label) { @@ -1355,7 +1359,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle entry.pushKV("category", "receive"); } entry.pushKV("amount", ValueFromAmount(r.amount)); - if (pwallet->m_address_book.count(r.destination)) { + if (address_book_entry) { entry.pushKV("label", label); } entry.pushKV("vout", r.vout); @@ -2955,9 +2959,9 @@ static UniValue listunspent(const JSONRPCRequest& request) if (fValidAddress) { entry.pushKV("address", EncodeDestination(address)); - auto i = pwallet->m_address_book.find(address); - if (i != pwallet->m_address_book.end()) { - entry.pushKV("label", i->second.name); + const auto* address_book_entry = pwallet->FindAddressBookEntry(address); + if (address_book_entry) { + entry.pushKV("label", address_book_entry->name); } std::unique_ptr provider = pwallet->GetSolvingProvider(scriptPubKey); @@ -3814,7 +3818,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // DEPRECATED: Return label field if existing. Currently only one label can // be associated with an address, so the label should be equivalent to the // value of the name key/value pair in the labels array below. - if ((pwallet->chain().rpcEnableDeprecated("label")) && (pwallet->m_address_book.count(dest))) { + const auto* address_book_entry = pwallet->FindAddressBookEntry(dest); + if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) { ret.pushKV("label", pwallet->m_address_book.at(dest).name); } @@ -3838,14 +3843,13 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // stable if we allow multiple labels to be associated with an address in // the future. UniValue labels(UniValue::VARR); - std::map::const_iterator mi = pwallet->m_address_book.find(dest); - if (mi != pwallet->m_address_book.end()) { + if (address_book_entry) { // DEPRECATED: The previous behavior of returning an array containing a // JSON object of `name` and `purpose` key/value pairs is deprecated. if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) { - labels.push_back(AddressBookDataToJSON(mi->second, true)); + labels.push_back(AddressBookDataToJSON(*address_book_entry, true)); } else { - labels.push_back(mi->second.name); + labels.push_back(address_book_entry->name); } } ret.pushKV("labels", std::move(labels)); @@ -3890,6 +3894,7 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request) UniValue ret(UniValue::VOBJ); std::set addresses; for (const std::pair& item : pwallet->m_address_book) { + if (item.second.IsChange()) continue; if (item.second.name == label) { std::string address = EncodeDestination(item.first); // CWallet::m_address_book is not expected to contain duplicate @@ -3954,6 +3959,7 @@ static UniValue listlabels(const JSONRPCRequest& request) // Add to a set to sort by label name, then insert into Univalue array std::set label_set; for (const std::pair& entry : pwallet->m_address_book) { + if (entry.second.IsChange()) continue; if (purpose.empty() || entry.second.purpose == purpose) { label_set.insert(entry.second.name); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ab36dacf37..b4cbcfd005 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1237,8 +1237,9 @@ bool CWallet::IsChange(const CScript& script) const return true; LOCK(cs_wallet); - if (!m_address_book.count(address)) + if (!FindAddressBookEntry(address)) { return true; + } } return false; } @@ -3192,7 +3193,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add { LOCK(cs_wallet); std::map::iterator mi = m_address_book.find(address); - fUpdated = mi != m_address_book.end(); + fUpdated = (mi != m_address_book.end() && !mi->second.IsChange()); m_address_book[address].SetLabel(strName); if (!strPurpose.empty()) /* update purpose only if requested */ m_address_book[address].purpose = strPurpose; @@ -3459,6 +3460,7 @@ std::set CWallet::GetLabelAddresses(const std::string& label) co std::set result; for (const std::pair& item : m_address_book) { + if (item.second.IsChange()) continue; const CTxDestination& address = item.first; const std::string& strName = item.second.name; if (strName == label) From 6d2905f57aaeb3ec3b63d31043f7673ca10003f2 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 04:27:03 +0000 Subject: [PATCH 6/7] Wallet: Avoid unnecessary/redundant m_address_book lookups --- src/wallet/rpcdump.cpp | 2 +- src/wallet/rpcwallet.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 40ea51e464..e1d8f51c4a 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -66,7 +66,7 @@ static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWall strAddr += ","; } strAddr += EncodeDestination(dest); - strLabel = EncodeDumpString(pwallet->m_address_book.at(dest).name); + strLabel = EncodeDumpString(address_book_entry->name); fLabelFound = true; } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 464371a15f..a7f7d357bc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -507,7 +507,7 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) { const auto* address_book_entry = pwallet->FindAddressBookEntry(address); if (address_book_entry) { - addressInfo.push_back(pwallet->m_address_book.find(address)->second.name); + addressInfo.push_back(address_book_entry->name); } } jsonGrouping.push_back(addressInfo); @@ -1317,7 +1317,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle entry.pushKV("amount", ValueFromAmount(-s.amount)); const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination); if (address_book_entry) { - entry.pushKV("label", pwallet->m_address_book.at(s.destination).name); + entry.pushKV("label", address_book_entry->name); } entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-nFee)); @@ -1335,7 +1335,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle std::string label; const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination); if (address_book_entry) { - label = pwallet->m_address_book.at(r.destination).name; + label = address_book_entry->name; } if (filter_label && label != *filter_label) { continue; @@ -3820,7 +3820,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // value of the name key/value pair in the labels array below. const auto* address_book_entry = pwallet->FindAddressBookEntry(dest); if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) { - ret.pushKV("label", pwallet->m_address_book.at(dest).name); + ret.pushKV("label", address_book_entry->name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); From b5795a788639305bab86a8b3f6b75d6ce81be083 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 22 Feb 2020 05:02:18 +0000 Subject: [PATCH 7/7] Wallet: Add warning comments and assert to CWallet::DelAddressBook --- src/wallet/wallet.cpp | 5 +++++ src/wallet/wallet.h | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b4cbcfd005..9538f4f2b7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3213,6 +3213,11 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { + // 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?). + assert(!IsMine(address)); + { LOCK(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c681dd28c8..7e770a40f2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -852,7 +852,10 @@ 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 + /** + * Adds a destination data tuple to the store, and saves it to disk + * When adding new fields, take care to consider how DelAddressBook should handle it! + */ 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(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);