Merge #18192: Bugfix: Wallet: Safely deal with change in the address book

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`.

ACKs for top commit:
  ryanofsky:
    Code review ACK b5795a788639305bab86a8b3f6b75d6ce81be083. Pretty clever and nicely implemented fix!
  jonatack:
    ACK b5795a788639305bab86a8b3f6b75d6ce81be083 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4ed06893d7c4 and tested manually with rpc/cli
  jnewbery:
    Good fix. utACK b5795a788.

Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0

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 returned by getAddresses at all.
     if (strPurpose == "send")
         addressType = AddressTableEntry::Sending;
     else if (strPurpose == "receive")

src/qt/test/addressbooktests.cpp

@@ -98,7 +98,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
 
     auto check_addbook_size = [wallet](int expected_size) {
         LOCK(wallet->cs_wallet);
-        QCOMPARE(static_cast<int>(wallet->mapAddressBook.size()), expected_size);
+        QCOMPARE(static_cast<int>(wallet->m_address_book.size()), expected_size);
     };
 
     // We should start with the two addresses we added earlier and nothing else.

src/wallet/interfaces.cpp

@@ -240,8 +240,8 @@ public:
         std::string* purpose) override
     {
         LOCK(m_wallet->cs_wallet);
-        auto it = m_wallet->mapAddressBook.find(dest);
+        auto it = m_wallet->m_address_book.find(dest);
-        if (it == m_wallet->mapAddressBook.end()) {
+        if (it == m_wallet->m_address_book.end() || it->second.IsChange()) {
             return false;
         }
         if (name) {
@@ -259,7 +259,8 @@ public:
     {
         LOCK(m_wallet->cs_wallet);
         std::vector<WalletAddress> 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);
         }
         return result;

src/wallet/rpcdump.cpp

@@ -142,7 +142,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
         {
             pwallet->MarkDirty();
 
-            if (!request.params[1].isNull() || pwallet->mapAddressBook.count(vchAddress) == 0) {
+            if (!request.params[1].isNull() || !pwallet->FindAddressBookEntry(vchAddress)) {
                 pwallet->SetAddressBook(vchAddress, strLabel, "receive");
             }
 
@@ -1011,8 +1011,9 @@ UniValue dumpwallet(const JSONRPCRequest& request)
         CKey key;
         if (spk_man.GetKey(keyid, key)) {
             file << strprintf("%s %s ", EncodeSecret(key), strTime);
-            if (pwallet->mapAddressBook.count(pkhash)) {
+            const auto* address_book_entry = pwallet->FindAddressBookEntry(pkhash);
-                file << strprintf("label=%s", EncodeDumpString(pwallet->mapAddressBook.at(pkhash).name));
+            if (address_book_entry) {
+                file << strprintf("label=%s", EncodeDumpString(address_book_entry->name));
             } else if (mapKeyPool.count(keyid)) {
                 file << "reserve=1";
             } else {

src/wallet/rpcwallet.cpp

@@ -500,8 +500,9 @@ 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()) {
+                const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
-                    addressInfo.push_back(pwallet->mapAddressBook.find(address)->second.name);
+                if (address_book_entry) {
+                    addressInfo.push_back(address_book_entry->name);
                 }
             }
             jsonGrouping.push_back(addressInfo);
@@ -1114,13 +1115,13 @@ static UniValue ListReceived(const CWallet * const pwallet, const UniValue& para
     UniValue ret(UniValue::VARR);
     std::map<std::string, tallyitem> 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 start = pwallet->m_address_book.begin();
-    auto end = pwallet->mapAddressBook.end();
+    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);
         }
@@ -1128,6 +1129,7 @@ static UniValue ListReceived(const CWallet * const pwallet, const UniValue& para
 
     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);
@@ -1330,8 +1332,9 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx,
             std::map<std::string, std::string>::const_iterator it = wtx.mapValue.find("DS");
             entry.pushKV("category", (it != wtx.mapValue.end() && it->second == "1") ? "coinjoin" : "send");
             entry.pushKV("amount", ValueFromAmount(-s.amount));
-            if (pwallet->mapAddressBook.count(s.destination)) {
+            const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination);
-                entry.pushKV("label", pwallet->mapAddressBook.at(s.destination).name);
+            if (address_book_entry) {
+                entry.pushKV("label", address_book_entry->name);
             }
             entry.pushKV("vout", s.vout);
             entry.pushKV("fee", ValueFromAmount(-nFee));
@@ -1348,8 +1351,9 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx,
         for (const COutputEntry& r : listReceived)
         {
             std::string label;
-            if (pwallet->mapAddressBook.count(r.destination)) {
+            const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination);
-                label = pwallet->mapAddressBook.at(r.destination).name;
+            if (address_book_entry) {
+                label = address_book_entry->name;
             }
             if (filter_label && label != *filter_label) {
                 continue;
@@ -1373,7 +1377,7 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx,
                 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);
@@ -3152,9 +3156,9 @@ static UniValue listunspent(const JSONRPCRequest& request)
         if (fValidAddress) {
             entry.pushKV("address", EncodeDestination(address));
 
-            auto i = pwallet->mapAddressBook.find(address);
+            const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
-            if (i != pwallet->mapAddressBook.end()) {
+            if (address_book_entry) {
-                entry.pushKV("label", i->second.name);
+                entry.pushKV("label", address_book_entry->name);
             }
 
             std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
@@ -3713,8 +3717,9 @@ 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))) {
+    const auto* address_book_entry = pwallet->FindAddressBookEntry(dest);
-        ret.pushKV("label", pwallet->mapAddressBook.at(dest).name);
+    if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) {
+        ret.pushKV("label", address_book_entry->name);
     }
 
     ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
@@ -3744,14 +3749,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<CTxDestination, CAddressBookData>::const_iterator mi = pwallet->mapAddressBook.find(dest);
+    if (address_book_entry) {
-    if (mi != pwallet->mapAddressBook.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")) {
-            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));
@@ -3792,10 +3796,11 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
     // Find all addresses that have the given label
     UniValue ret(UniValue::VOBJ);
     std::set<std::string> addresses;
-    for (const std::pair<CTxDestination, CAddressBookData> item : pwallet->mapAddressBook) {
+    for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->m_address_book) {
+        if (item.second.IsChange()) continue;
         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;
@@ -3853,7 +3858,8 @@ static UniValue listlabels(const JSONRPCRequest& request)
 
     // Add to a set to sort by label name, then insert into Univalue array
     std::set<std::string> label_set;
-    for (const std::pair<CTxDestination, CAddressBookData> entry : pwallet->mapAddressBook) {
+    for (const std::pair<const CTxDestination, CAddressBookData>& entry : pwallet->m_address_book) {
+        if (entry.second.IsChange()) continue;
         if (purpose.empty() || entry.second.purpose == purpose) {
             label_set.insert(entry.second.name);
         }

src/wallet/wallet.cpp

@@ -1501,9 +1501,10 @@ bool CWallet::IsChange(const CScript& script) const
             return true;
 
         LOCK(cs_wallet);
-        if (!mapAddressBook.count(address))
+        if (!FindAddressBookEntry(address)) {
             return true;
         }
+    }
     return false;
 }
 
@@ -3905,11 +3906,11 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
     bool fUpdated = false;
     {
         LOCK(cs_wallet);
-        std::map<CTxDestination, CAddressBookData>::iterator mi = mapAddressBook.find(address);
+        std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
-        fUpdated = mi != mapAddressBook.end();
+        fUpdated = (mi != m_address_book.end() && !mi->second.IsChange());
-        mapAddressBook[address].name = strName;
+        m_address_book[address].SetLabel(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) );
@@ -3926,16 +3927,21 @@ 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);
 
         // Delete destdata tuples associated with address
         std::string strAddress = EncodeDestination(address);
-        for (const std::pair<const std::string, std::string> &item : mapAddressBook[address].destdata)
+        for (const std::pair<const std::string, std::string> &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);
@@ -4172,8 +4178,9 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
 {
     LOCK(cs_wallet);
     std::set<CTxDestination> result;
-    for (const std::pair<const CTxDestination, CAddressBookData>& item : mapAddressBook)
+    for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book)
     {
+        if (item.second.IsChange()) continue;
         const CTxDestination& address = item.first;
         const std::string& strName = item.second.name;
         if (strName == label)
@@ -4406,26 +4413,26 @@ bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const
     if (std::get_if<CNoDestination>(&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<CTxDestination, CAddressBookData>::const_iterator i = mapAddressBook.find(dest);
+    std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
-    if(i != mapAddressBook.end())
+    if(i != m_address_book.end())
     {
         CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
         if(j != i->second.destdata.end())
@@ -4441,7 +4448,7 @@ bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, st
 std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
 {
     std::vector<std::string> 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);
@@ -4801,7 +4808,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
         walletInstance->WalletLogPrintf("setExternalKeyPool.size() = %u\n",   walletInstance->KeypoolCountExternalKeys());
         walletInstance->WalletLogPrintf("setInternalKeyPool.size() = %u\n",   walletInstance->KeypoolCountInternalKeys());
         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());
         for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) {
             walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", spk_man->GetTimeFirstKey());
         }
@@ -4831,6 +4838,16 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error, std::vector<bilin
     return true;
 }
 
+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()
 {
     LOCK(cs_wallet);

src/wallet/wallet.h

@@ -187,14 +187,23 @@ public:
 /** 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<std::string, std::string> StringMap;
     StringMap destdata;
+
+    bool IsChange() const { return m_change; }
+    void SetLabel(const std::string& label) {
+        m_change = false;
+        m_label = label;
+    }
 };
 
 struct CRecipient
@@ -847,7 +856,8 @@ public:
     int64_t nOrderPosNext GUARDED_BY(cs_wallet) = 0;
     uint64_t nAccountingEntryNumber = 0;
 
-    std::map<CTxDestination, CAddressBookData> mapAddressBook GUARDED_BY(cs_wallet);
+    std::map<CTxDestination, CAddressBookData> m_address_book GUARDED_BY(cs_wallet);
+    const CAddressBookData* FindAddressBookEntry(const CTxDestination&, bool allow_change = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);
 
@@ -933,7 +943,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);

src/wallet/walletdb.cpp

@@ -243,11 +243,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
         if (strType == DBKeys::NAME) {
             std::string strAddress;
             ssKey >> strAddress;
-            ssValue >> pwallet->mapAddressBook[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;
-            ssValue >> pwallet->mapAddressBook[DecodeDestination(strAddress)].purpose;
+            ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].purpose;
         } else if (strType == DBKeys::TX) {
             uint256 hash;
             ssKey >> hash;

src/wallet/wallettool.cpp

@@ -101,7 +101,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)