diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -166,8 +166,8 @@ bool getAddress(const CTxDestination &dest, std::string *name, isminetype *is_mine, 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() || it->second.IsChange()) { return false; } if (name) { @@ -184,7 +184,10 @@ std::vector getAddresses() override { LOCK(m_wallet->cs_wallet); std::vector result; - for (const auto &item : m_wallet->mapAddressBook) { + 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); } diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -53,8 +53,8 @@ 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 returned by + // getAddresses 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 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -99,7 +99,7 @@ auto check_addbook_size = [&wallet](int expected_size) { LOCK(wallet->cs_wallet); - QCOMPARE(static_cast(wallet->mapAddressBook.size()), + QCOMPARE(static_cast(wallet->m_address_book.size()), expected_size); }; diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -61,12 +61,13 @@ CKey key; spk_man->GetKey(keyid, key); for (const auto &dest : GetAllDestinationsForKey(key.GetPubKey())) { - if (pwallet->mapAddressBook.count(dest)) { + const auto *address_book_entry = pwallet->FindAddressBookEntry(dest); + if (address_book_entry) { if (!strAddr.empty()) { strAddr += ","; } strAddr += EncodeDestination(dest, config); - strLabel = EncodeDumpString(pwallet->mapAddressBook[dest].name); + strLabel = EncodeDumpString(address_book_entry->name); fLabelFound = true; } } @@ -194,7 +195,7 @@ // label was passed. for (const auto &dest : GetAllDestinationsForKey(pubkey)) { if (!request.params[1].isNull() || - pwallet->mapAddressBook.count(dest) == 0) { + !pwallet->FindAddressBookEntry(dest)) { pwallet->SetAddressBook(dest, strLabel, "receive"); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -551,10 +551,10 @@ addressInfo.push_back(EncodeDestination(address, config)); addressInfo.push_back(ValueFromAmount(balances[address])); - if (pwallet->mapAddressBook.find(address) != - pwallet->mapAddressBook.end()) { - addressInfo.push_back( - pwallet->mapAddressBook.find(address)->second.name); + const auto *address_book_entry = + pwallet->FindAddressBookEntry(address); + if (address_book_entry) { + addressInfo.push_back(address_book_entry->name); } jsonGrouping.push_back(addressInfo); } @@ -1256,19 +1256,22 @@ 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); } } 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; std::map::iterator it = @@ -1494,9 +1497,10 @@ 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[s.destination].name); + const auto *address_book_entry = + pwallet->FindAddressBookEntry(s.destination); + if (address_book_entry) { + entry.pushKV("label", address_book_entry->name); } entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-1 * nFee)); @@ -1512,8 +1516,10 @@ if (listReceived.size() > 0 && wtx.GetDepthInMainChain() >= nMinDepth) { for (const COutputEntry &r : listReceived) { std::string label; - if (pwallet->mapAddressBook.count(r.destination)) { - label = pwallet->mapAddressBook[r.destination].name; + const auto *address_book_entry = + pwallet->FindAddressBookEntry(r.destination); + if (address_book_entry) { + label = address_book_entry->name; } if (filter_label && label != *filter_label) { continue; @@ -1536,7 +1542,7 @@ entry.pushKV("category", "receive"); } entry.pushKV("amount", ValueFromAmount(r.amount)); - if (pwallet->mapAddressBook.count(r.destination)) { + if (address_book_entry) { entry.pushKV("label", label); } entry.pushKV("vout", r.vout); @@ -3527,9 +3533,10 @@ if (fValidAddress) { entry.pushKV("address", EncodeDestination(address, config)); - auto i = pwallet->mapAddressBook.find(address); - if (i != pwallet->mapAddressBook.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); } const SigningProvider *provider = @@ -4287,8 +4294,9 @@ // 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->mapAddressBook.count(dest)) { - ret.pushKV("label", pwallet->mapAddressBook[dest].name); + const auto *address_book_entry = pwallet->FindAddressBookEntry(dest); + if (address_book_entry) { + ret.pushKV("label", address_book_entry->name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); @@ -4316,13 +4324,11 @@ // DEPRECATED: The previous behavior of returning an array containing a JSON // object of `name` and `purpose` key/value pairs has been deprecated. UniValue labels(UniValue::VARR); - std::map::iterator mi = - pwallet->mapAddressBook.find(dest); - if (mi != pwallet->mapAddressBook.end()) { + if (address_book_entry) { 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)); @@ -4365,10 +4371,13 @@ UniValue ret(UniValue::VOBJ); std::set addresses; for (const std::pair &item : - pwallet->mapAddressBook) { + pwallet->m_address_book) { + if (item.second.IsChange()) { + continue; + } if (item.second.name == label) { std::string address = EncodeDestination(item.first, config); - // 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; @@ -4429,7 +4438,10 @@ // 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) { + 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/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp --- a/src/wallet/test/walletdb_tests.cpp +++ b/src/wallet/test/walletdb_tests.cpp @@ -41,9 +41,9 @@ { auto w = LoadWallet(batch); LOCK(w->cs_wallet); - BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst1)); - BOOST_CHECK_EQUAL("name1", w->mapAddressBook[dst1].name); - BOOST_CHECK_EQUAL("name2", w->mapAddressBook[dst2].name); + BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst1)); + BOOST_CHECK_EQUAL("name1", w->m_address_book[dst1].name); + BOOST_CHECK_EQUAL("name2", w->m_address_book[dst2].name); } batch.EraseName(dst1); @@ -51,8 +51,8 @@ { auto w = LoadWallet(batch); LOCK(w->cs_wallet); - BOOST_CHECK_EQUAL(0, w->mapAddressBook.count(dst1)); - BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst2)); + BOOST_CHECK_EQUAL(0, w->m_address_book.count(dst1)); + BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst2)); } } @@ -67,9 +67,9 @@ { auto w = LoadWallet(batch); LOCK(w->cs_wallet); - BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst1)); - BOOST_CHECK_EQUAL("purpose1", w->mapAddressBook[dst1].purpose); - BOOST_CHECK_EQUAL("purpose2", w->mapAddressBook[dst2].purpose); + BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst1)); + BOOST_CHECK_EQUAL("purpose1", w->m_address_book[dst1].purpose); + BOOST_CHECK_EQUAL("purpose2", w->m_address_book[dst2].purpose); } batch.ErasePurpose(dst1); @@ -77,8 +77,8 @@ { auto w = LoadWallet(batch); LOCK(w->cs_wallet); - BOOST_CHECK_EQUAL(0, w->mapAddressBook.count(dst1)); - BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst2)); + BOOST_CHECK_EQUAL(0, w->m_address_book.count(dst1)); + BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst2)); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -190,14 +190,24 @@ /** Address book data */ class CAddressBookData { +private: + bool m_change{true}; + 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; + + bool IsChange() const { return m_change; } + void SetLabel(const std::string &label) { + m_change = false; + m_label = label; + } }; struct CRecipient { @@ -858,7 +868,11 @@ uint64_t nAccountingEntryNumber = 0; std::map - mapAddressBook GUARDED_BY(cs_wallet); + 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); @@ -969,7 +983,11 @@ 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); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1316,7 +1316,7 @@ } LOCK(cs_wallet); - if (!mapAddressBook.count(address)) { + if (!FindAddressBookEntry(address)) { return true; } } @@ -3429,12 +3429,12 @@ { LOCK(cs_wallet); std::map::iterator mi = - mapAddressBook.find(address); - fUpdated = mi != mapAddressBook.end(); - mapAddressBook[address].name = strName; + m_address_book.find(address); + fUpdated = (mi != m_address_book.end() && !mi->second.IsChange()); + m_address_book[address].SetLabel(strName); // Update purpose only if requested. if (!strPurpose.empty()) { - mapAddressBook[address].purpose = strPurpose; + m_address_book[address].purpose = strPurpose; } } @@ -3455,16 +3455,24 @@ } 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); - // Delete destdata tuples associated with address. + // Delete destdata tuples associated with address for (const std::pair &item : - mapAddressBook[address].destdata) { + m_address_book[address].destdata) { WalletBatch(*database).EraseDestData(address, item.first); } - - mapAddressBook.erase(address); + m_address_book.erase(address); } NotifyAddressBookChanged(this, address, "", IsMine(address) != ISMINE_NO, @@ -3718,7 +3726,10 @@ LOCK(cs_wallet); std::set result; for (const std::pair &item : - mapAddressBook) { + m_address_book) { + if (item.second.IsChange()) { + continue; + } const CTxDestination &address = item.first; const std::string &strName = item.second.name; if (strName == label) { @@ -3930,13 +3941,13 @@ 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(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; } @@ -3945,14 +3956,14 @@ 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()) { + 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()) { @@ -3969,7 +3980,7 @@ 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); @@ -4442,12 +4453,25 @@ 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; } +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(); LOCK(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -219,15 +219,17 @@ if (strType == DBKeys::NAME) { std::string strAddress; ssKey >> strAddress; - ssValue >> pwallet - ->mapAddressBook[DecodeDestination( - strAddress, pwallet->chainParams)] - .name; + std::string label; + ssValue >> label; + pwallet + ->m_address_book[DecodeDestination(strAddress, + pwallet->chainParams)] + .SetLabel(label); } else if (strType == DBKeys::PURPOSE) { std::string strAddress; ssKey >> strAddress; ssValue >> pwallet - ->mapAddressBook[DecodeDestination( + ->m_address_book[DecodeDestination( strAddress, pwallet->chainParams)] .purpose; } else if (strType == DBKeys::TX) { diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -119,7 +119,7 @@ tfm::format(std::cout, "Transactions: %zu\n", wallet_instance->mapWallet.size()); tfm::format(std::cout, "Address Book: %zu\n", - wallet_instance->mapAddressBook.size()); + wallet_instance->m_address_book.size()); } bool ExecuteWalletToolFunc(const std::string &command,