From 082ae84501923363f9b6ca00e121627165f0bd54 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 6 Oct 2020 15:48:51 -0700 Subject: [PATCH] Merge #18192: Bugfix: Wallet: Safely deal with change in the address book Summary: b5795a788639305bab86a8b3f6b75d6ce81be083 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr) 6d2905f57aaeb3ec3b63d31043f7673ca10003f2 Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr) c751d886f499257627b308b11ffaa51c22db6cc0 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr) 8e64b8c84bcbd63caea06f3af087af1f0609eaf5 Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr) 65b6bdc2b164343ec3cc3d32a0297daff9e24fec Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr) 144b2f85da4d51bf7d72b987888ddcaf5b429eed Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr) b86cd155f6f661052042048aa7cfc2a397afe4f7 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr) Pull request description: In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change. This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue. Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label). Fixing it is accomplished by: * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime. * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them. * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile). A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`. Backport of Core [[https://github.com/bitcoin/bitcoin/pull/18192 | PR18192]] Tests for this change to follow in [[https://github.com/bitcoin/bitcoin/pull/18546 | PR18546]] Test Plan: `ninja check check-functional` for sanity Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7810 --- src/interfaces/wallet.cpp | 9 ++-- src/qt/addresstablemodel.cpp | 4 +- src/qt/test/addressbooktests.cpp | 2 +- src/wallet/rpcdump.cpp | 7 ++-- src/wallet/rpcwallet.cpp | 66 ++++++++++++++++++------------ src/wallet/test/walletdb_tests.cpp | 20 ++++----- src/wallet/wallet.cpp | 60 +++++++++++++++++++-------- src/wallet/wallet.h | 26 ++++++++++-- src/wallet/walletdb.cpp | 12 +++--- src/wallet/wallettool.cpp | 2 +- 10 files changed, 134 insertions(+), 74 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 1e10c54efc..c723131c64 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -166,8 +166,8 @@ namespace { 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 @@ namespace { 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 index d5067b7432..d3be6fc653 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -53,8 +53,8 @@ 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 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 index ae92bd5fee..79d4af5aaf 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -99,7 +99,7 @@ void TestAddAddressesToSendBook(interfaces::Node &node) { 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 index cf8c4a1c7d..649a7e7bc3 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -61,12 +61,13 @@ GetWalletAddressesForKey(const Config &config, LegacyScriptPubKeyMan *spk_man, 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 @@ UniValue importprivkey(const Config &config, const JSONRPCRequest &request) { // 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 index e94a514d8a..ffd5eaa180 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -551,10 +551,10 @@ static UniValue listaddressgroupings(const Config &config, 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 @@ ListReceived(const Config &config, interfaces::Chain::Lock &locked_chain, 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 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain, 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 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain, 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 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain, 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 @@ static UniValue listunspent(const Config &config, 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 @@ UniValue getaddressinfo(const Config &config, const JSONRPCRequest &request) { // 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 @@ UniValue getaddressinfo(const Config &config, const JSONRPCRequest &request) { // 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 getaddressesbylabel(const Config &config, 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 @@ UniValue listlabels(const Config &config, 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) { + 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 index 27fe5529b3..ac3310b790 100644 --- a/src/wallet/test/walletdb_tests.cpp +++ b/src/wallet/test/walletdb_tests.cpp @@ -41,9 +41,9 @@ BOOST_AUTO_TEST_CASE(write_erase_name) { { 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 @@ BOOST_AUTO_TEST_CASE(write_erase_name) { { 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 @@ BOOST_AUTO_TEST_CASE(write_erase_purpose) { { 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 @@ BOOST_AUTO_TEST_CASE(write_erase_purpose) { { 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.cpp b/src/wallet/wallet.cpp index b16973ed06..ea73747a3d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1316,7 +1316,7 @@ bool CWallet::IsChange(const CScript &script) const { } LOCK(cs_wallet); - if (!mapAddressBook.count(address)) { + if (!FindAddressBookEntry(address)) { return true; } } @@ -3429,12 +3429,12 @@ bool CWallet::SetAddressBookWithDB(WalletBatch &batch, { 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::SetAddressBook(const CTxDestination &address, } 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 @@ CWallet::GetLabelAddresses(const std::string &label) const { 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 @@ bool CWallet::AddDestData(WalletBatch &batch, const CTxDestination &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(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 @@ bool CWallet::EraseDestData(WalletBatch &batch, const CTxDestination &dest, 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 @@ bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, 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 @@ std::shared_ptr CWallet::CreateWalletFromFile( 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/wallet.h b/src/wallet/wallet.h index a91005b554..99840b1f84 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -190,14 +190,24 @@ class ReserveDestination { /** 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 @@ class CWallet final : public WalletStorage, 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 @@ class CWallet final : public WalletStorage, 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/walletdb.cpp b/src/wallet/walletdb.cpp index 2164a52d0f..81609270c9 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -219,15 +219,17 @@ static bool ReadKeyValue(CWallet *pwallet, CDataStream &ssKey, 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 index 8bd2c67435..5cb8da80e1 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -119,7 +119,7 @@ static void WalletShowInfo(CWallet *wallet_instance) { 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,