Merge bitcoin/bitcoin#27217: wallet: Replace use of purpose strings with an enum

18fc71a3ad doc: Release note for purpose string restriction (Andrew Chow)
e83babe3b8 wallet: Replace use of purpose strings with an enum (Andrew Chow)
2f80005136 wallet: add AddressPurpose enum to replace string values (Ryan Ofsky)
8741522e6c wallet: Add wallet/types.h for simple public enum and struct types (Ryan Ofsky)

Pull request description:

  Instead of storing and passing around fixed strings for the purpose of an address, use an enum.

ACKs for top commit:
  josibake:
    reACK 18fc71a3ad

Tree-SHA512: 82034f020e96b99b29da34dfdd7cfe58f8b7d2afed1409ea4a290c2cac69fc43e449e8b7b2afd874a9facf8f4cd6ebb80d17462317e60a6f011ed8f9eab5d4c5

doc/release-notes-27217.md

@@ -0,0 +1,6 @@
+Wallet
+------
+
+* Address Purposes strings are now restricted to the currently known values of "send", "receive", and "refund".
+  Wallets that have unrecognized purpose strings will have loading warnings, and the `listlabels`
+  RPC will raise an error if an unrecognized purpose is requested.

src/Makefile.am

@@ -329,7 +329,6 @@ BITCOIN_CORE_H = \
   wallet/external_signer_scriptpubkeyman.h \
   wallet/feebumper.h \
   wallet/fees.h \
-  wallet/ismine.h \
   wallet/load.h \
   wallet/receive.h \
   wallet/rpc/util.h \
@@ -339,6 +338,7 @@ BITCOIN_CORE_H = \
   wallet/spend.h \
   wallet/sqlite.h \
   wallet/transaction.h \
+  wallet/types.h \
   wallet/wallet.h \
   wallet/walletdb.h \
   wallet/wallettool.h \

src/interfaces/wallet.h

@@ -35,6 +35,7 @@ struct bilingual_str;
 namespace wallet {
 class CCoinControl;
 class CWallet;
+enum class AddressPurpose;
 enum isminetype : unsigned int;
 struct CRecipient;
 struct WalletContext;
@@ -103,7 +104,7 @@ public:
     virtual bool haveWatchOnly() = 0;
 
     //! Add or update address.
-    virtual bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::string& purpose) = 0;
+    virtual bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::optional<wallet::AddressPurpose>& purpose) = 0;
 
     // Remove address.
     virtual bool delAddressBook(const CTxDestination& dest) = 0;
@@ -112,7 +113,7 @@ public:
     virtual bool getAddress(const CTxDestination& dest,
         std::string* name,
         wallet::isminetype* is_mine,
-        std::string* purpose) = 0;
+        wallet::AddressPurpose* purpose) = 0;
 
     //! Get wallet address list.
     virtual std::vector<WalletAddress> getAddresses() const = 0;
@@ -293,7 +294,7 @@ public:
     using AddressBookChangedFn = std::function<void(const CTxDestination& address,
         const std::string& label,
         bool is_mine,
-        const std::string& purpose,
+        wallet::AddressPurpose purpose,
         ChangeType status)>;
     virtual std::unique_ptr<Handler> handleAddressBookChanged(AddressBookChangedFn fn) = 0;
 
@@ -352,11 +353,11 @@ struct WalletAddress
 {
     CTxDestination dest;
     wallet::isminetype is_mine;
+    wallet::AddressPurpose purpose;
     std::string name;
-    std::string purpose;
 
-    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, std::string name, std::string purpose)
+    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name)
-        : dest(std::move(dest)), is_mine(is_mine), name(std::move(name)), purpose(std::move(purpose))
+        : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name))
     {
     }
 };

src/qt/addresstablemodel.cpp

@@ -8,6 +8,7 @@
 #include <qt/walletmodel.h>
 
 #include <key_io.h>
+#include <wallet/types.h>
 #include <wallet/wallet.h>
 
 #include <algorithm>
@@ -52,17 +53,16 @@ struct AddressTableEntryLessThan
 };
 
 /* Determine address type from address purpose */
-static AddressTableEntry::Type translateTransactionType(const QString &strPurpose, bool isMine)
+static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose purpose, bool isMine)
 {
-    AddressTableEntry::Type addressType = AddressTableEntry::Hidden;
     // "refund" addresses aren't shown, and change addresses aren't returned by getAddresses at all.
-    if (strPurpose == "send")
+    switch (purpose) {
-        addressType = AddressTableEntry::Sending;
+    case wallet::AddressPurpose::SEND: return AddressTableEntry::Sending;
-    else if (strPurpose == "receive")
+    case wallet::AddressPurpose::RECEIVE: return AddressTableEntry::Receiving;
-        addressType = AddressTableEntry::Receiving;
+    case wallet::AddressPurpose::REFUND: return AddressTableEntry::Hidden;
-    else if (strPurpose == "unknown" || strPurpose == "") // if purpose not set, guess
+    // No default case to allow for compiler to warn
-        addressType = (isMine ? AddressTableEntry::Receiving : AddressTableEntry::Sending);
+    }
-    return addressType;
+    assert(false);
 }
 
 // Private implementation
@@ -85,7 +85,7 @@ public:
                     continue;
                 }
                 AddressTableEntry::Type addressType = translateTransactionType(
-                        QString::fromStdString(address.purpose), address.is_mine);
+                        address.purpose, address.is_mine);
                 cachedAddressTable.append(AddressTableEntry(addressType,
                                   QString::fromStdString(address.name),
                                   QString::fromStdString(EncodeDestination(address.dest))));
@@ -97,7 +97,7 @@ public:
         std::sort(cachedAddressTable.begin(), cachedAddressTable.end(), AddressTableEntryLessThan());
     }
 
-    void updateEntry(const QString &address, const QString &label, bool isMine, const QString &purpose, int status)
+    void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status)
     {
         // Find address / label in model
         QList<AddressTableEntry>::iterator lower = std::lower_bound(
@@ -239,7 +239,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
     if(!index.isValid())
         return false;
     AddressTableEntry *rec = static_cast<AddressTableEntry*>(index.internalPointer());
-    std::string strPurpose = (rec->type == AddressTableEntry::Sending ? "send" : "receive");
+    wallet::AddressPurpose purpose = rec->type == AddressTableEntry::Sending ? wallet::AddressPurpose::SEND : wallet::AddressPurpose::RECEIVE;
     editStatus = OK;
 
     if(role == Qt::EditRole)
@@ -253,7 +253,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
                 editStatus = NO_CHANGES;
                 return false;
             }
-            walletModel->wallet().setAddressBook(curAddress, value.toString().toStdString(), strPurpose);
+            walletModel->wallet().setAddressBook(curAddress, value.toString().toStdString(), purpose);
         } else if(index.column() == Address) {
             CTxDestination newAddress = DecodeDestination(value.toString().toStdString());
             // Refuse to set invalid address, set error status and return false
@@ -282,7 +282,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
                 // Remove old entry
                 walletModel->wallet().delAddressBook(curAddress);
                 // Add new entry with new address
-                walletModel->wallet().setAddressBook(newAddress, value.toString().toStdString(), strPurpose);
+                walletModel->wallet().setAddressBook(newAddress, value.toString().toStdString(), purpose);
             }
         }
         return true;
@@ -334,7 +334,7 @@ QModelIndex AddressTableModel::index(int row, int column, const QModelIndex &par
 }
 
 void AddressTableModel::updateEntry(const QString &address,
-        const QString &label, bool isMine, const QString &purpose, int status)
+        const QString &label, bool isMine, wallet::AddressPurpose purpose, int status)
 {
     // Update address book model from Bitcoin core
     priv->updateEntry(address, label, isMine, purpose, status);
@@ -365,7 +365,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
         }
 
         // Add entry
-        walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, "send");
+        walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, wallet::AddressPurpose::SEND);
     }
     else if(type == Receive)
     {
@@ -416,18 +416,18 @@ QString AddressTableModel::labelForAddress(const QString &address) const
     return QString();
 }
 
-QString AddressTableModel::purposeForAddress(const QString &address) const
+std::optional<wallet::AddressPurpose> AddressTableModel::purposeForAddress(const QString &address) const
 {
-    std::string purpose;
+    wallet::AddressPurpose purpose;
     if (getAddressData(address, /* name= */ nullptr, &purpose)) {
-        return QString::fromStdString(purpose);
+        return purpose;
     }
-    return QString();
+    return std::nullopt;
 }
 
 bool AddressTableModel::getAddressData(const QString &address,
         std::string* name,
-        std::string* purpose) const {
+        wallet::AddressPurpose* purpose) const {
     CTxDestination destination = DecodeDestination(address.toStdString());
     return walletModel->wallet().getAddress(destination, name, /* is_mine= */ nullptr, purpose);
 }

src/qt/addresstablemodel.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_QT_ADDRESSTABLEMODEL_H
 #define BITCOIN_QT_ADDRESSTABLEMODEL_H
 
+#include <optional>
+
 #include <QAbstractTableModel>
 #include <QStringList>
 
@@ -16,6 +18,9 @@ class WalletModel;
 namespace interfaces {
 class Wallet;
 }
+namespace wallet {
+enum class AddressPurpose;
+} // namespace wallet
 
 /**
    Qt model of the address book in the core. This allows views to access and modify the address book.
@@ -71,7 +76,7 @@ public:
     QString labelForAddress(const QString &address) const;
 
     /** Look up purpose for address in address book, if not found return empty string. */
-    QString purposeForAddress(const QString &address) const;
+    std::optional<wallet::AddressPurpose> purposeForAddress(const QString &address) const;
 
     /* Look up row index of an address in the model.
        Return -1 if not found.
@@ -89,7 +94,7 @@ private:
     EditStatus editStatus = OK;
 
     /** Look up address book data given an address string. */
-    bool getAddressData(const QString &address, std::string* name, std::string* purpose) const;
+    bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
 
     /** Notify listeners that data changed. */
     void emitDataChanged(int index);
@@ -97,7 +102,7 @@ private:
 public Q_SLOTS:
     /* Update address list from core.
      */
-    void updateEntry(const QString &address, const QString &label, bool isMine, const QString &purpose, int status);
+    void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status);
 
     friend class AddressTablePriv;
 };

src/qt/editaddressdialog.cpp

@@ -8,6 +8,8 @@
 #include <qt/addresstablemodel.h>
 #include <qt/guiutil.h>
 
+#include <wallet/types.h>
+
 #include <QDataWidgetMapper>
 #include <QMessageBox>
 
@@ -137,9 +139,9 @@ QString EditAddressDialog::getDuplicateAddressWarning() const
 {
     QString dup_address = ui->addressEdit->text();
     QString existing_label = model->labelForAddress(dup_address);
-    QString existing_purpose = model->purposeForAddress(dup_address);
+    auto existing_purpose = model->purposeForAddress(dup_address);
 
-    if (existing_purpose == "receive" &&
+    if (existing_purpose == wallet::AddressPurpose::RECEIVE &&
             (mode == NewSendingAddress || mode == EditSendingAddress)) {
         return tr(
             "Address \"%1\" already exists as a receiving address with label "

src/qt/test/addressbooktests.cpp

@@ -113,8 +113,8 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
 
     {
         LOCK(wallet->cs_wallet);
-        wallet->SetAddressBook(r_key_dest, r_label.toStdString(), "receive");
+        wallet->SetAddressBook(r_key_dest, r_label.toStdString(), wallet::AddressPurpose::RECEIVE);
-        wallet->SetAddressBook(s_key_dest, s_label.toStdString(), "send");
+        wallet->SetAddressBook(s_key_dest, s_label.toStdString(), wallet::AddressPurpose::SEND);
     }
 
     auto check_addbook_size = [&wallet](int expected_size) {

src/qt/test/wallettests.cpp

@@ -221,7 +221,7 @@ std::shared_ptr<CWallet> SetupDescriptorsWallet(interfaces::Node& node, TestChai
     WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
     if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
     CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);
-    wallet->SetAddressBook(dest, "", "receive");
+    wallet->SetAddressBook(dest, "", wallet::AddressPurpose::RECEIVE);
     wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
     SyncUpWallet(wallet, node);
     wallet->SetBroadcastTransactions(true);

src/qt/transactiondesc.cpp

@@ -20,7 +20,7 @@
 #include <policy/policy.h>
 #include <util/system.h>
 #include <validation.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 
 #include <stdint.h>
 #include <string>

src/qt/transactionrecord.cpp

@@ -7,7 +7,7 @@
 #include <chain.h>
 #include <interfaces/wallet.h>
 #include <key_io.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 
 #include <stdint.h>
 

src/qt/walletmodel.cpp

@@ -138,7 +138,7 @@ void WalletModel::updateTransaction()
 }
 
 void WalletModel::updateAddressBook(const QString &address, const QString &label,
-        bool isMine, const QString &purpose, int status)
+        bool isMine, wallet::AddressPurpose purpose, int status)
 {
     if(addressTableModel)
         addressTableModel->updateEntry(address, label, isMine, purpose, status);
@@ -280,11 +280,11 @@ void WalletModel::sendCoins(WalletModelTransaction& transaction)
                 if (!m_wallet->getAddress(
                      dest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr))
                 {
-                    m_wallet->setAddressBook(dest, strLabel, "send");
+                    m_wallet->setAddressBook(dest, strLabel, wallet::AddressPurpose::SEND);
                 }
                 else if (name != strLabel)
                 {
-                    m_wallet->setAddressBook(dest, strLabel, ""); // "" means don't change purpose
+                    m_wallet->setAddressBook(dest, strLabel, {}); // {} means don't change purpose
                 }
             }
         }
@@ -377,18 +377,17 @@ static void NotifyKeyStoreStatusChanged(WalletModel *walletmodel)
 
 static void NotifyAddressBookChanged(WalletModel *walletmodel,
         const CTxDestination &address, const std::string &label, bool isMine,
-        const std::string &purpose, ChangeType status)
+        wallet::AddressPurpose purpose, ChangeType status)
 {
     QString strAddress = QString::fromStdString(EncodeDestination(address));
     QString strLabel = QString::fromStdString(label);
-    QString strPurpose = QString::fromStdString(purpose);
 
-    qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + strPurpose + " status=" + QString::number(status);
+    qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + QString::number(static_cast<uint8_t>(purpose)) + " status=" + QString::number(status);
     bool invoked = QMetaObject::invokeMethod(walletmodel, "updateAddressBook",
                               Q_ARG(QString, strAddress),
                               Q_ARG(QString, strLabel),
                               Q_ARG(bool, isMine),
-                              Q_ARG(QString, strPurpose),
+                              Q_ARG(wallet::AddressPurpose, purpose),
                               Q_ARG(int, status));
     assert(invoked);
 }

src/qt/walletmodel.h

@@ -236,7 +236,7 @@ public Q_SLOTS:
     /* New transaction, or transaction changed status */
     void updateTransaction();
     /* New, updated or removed address book entry */
-    void updateAddressBook(const QString &address, const QString &label, bool isMine, const QString &purpose, int status);
+    void updateAddressBook(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status);
     /* Watch-only added */
     void updateWatchOnlyFlag(bool fHaveWatchonly);
     /* Current, immature or unconfirmed balance might have changed - emit 'balanceChanged' if so */

src/wallet/interfaces.cpp

@@ -22,7 +22,7 @@
 #include <wallet/context.h>
 #include <wallet/feebumper.h>
 #include <wallet/fees.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 #include <wallet/load.h>
 #include <wallet/receive.h>
 #include <wallet/rpc/wallet.h>
@@ -179,7 +179,7 @@ public:
         }
         return false;
     };
-    bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::string& purpose) override
+    bool setAddressBook(const CTxDestination& dest, const std::string& name, const std::optional<AddressPurpose>& purpose) override
     {
         return m_wallet->SetAddressBook(dest, name, purpose);
     }
@@ -190,7 +190,7 @@ public:
     bool getAddress(const CTxDestination& dest,
         std::string* name,
         isminetype* is_mine,
-        std::string* purpose) override
+        AddressPurpose* purpose) override
     {
         LOCK(m_wallet->cs_wallet);
         const auto& entry = m_wallet->FindAddressBookEntry(dest, /*allow_change=*/false);
@@ -198,11 +198,16 @@ public:
         if (name) {
             *name = entry->GetLabel();
         }
+        std::optional<isminetype> dest_is_mine;
+        if (is_mine || purpose) {
+            dest_is_mine = m_wallet->IsMine(dest);
+        }
         if (is_mine) {
-            *is_mine = m_wallet->IsMine(dest);
+            *is_mine = *dest_is_mine;
         }
         if (purpose) {
-            *purpose = entry->purpose;
+            // In very old wallets, address purpose may not be recorded so we derive it from IsMine
+            *purpose = entry->purpose.value_or(*dest_is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND);
         }
         return true;
     }
@@ -210,9 +215,11 @@ public:
     {
         LOCK(m_wallet->cs_wallet);
         std::vector<WalletAddress> result;
-        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) {
+        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) {
             if (is_change) return;
-            result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose);
+            isminetype is_mine = m_wallet->IsMine(dest);
+            // In very old wallets, address purpose may not be recorded so we derive it from IsMine
+            result.emplace_back(dest, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), label);
         });
         return result;
     }
@@ -519,7 +526,7 @@ public:
     {
         return MakeSignalHandler(m_wallet->NotifyAddressBookChanged.connect(
             [fn](const CTxDestination& address, const std::string& label, bool is_mine,
-                 const std::string& purpose, ChangeType status) { fn(address, label, is_mine, purpose, status); }));
+                 AddressPurpose purpose, ChangeType status) { fn(address, label, is_mine, purpose, status); }));
     }
     std::unique_ptr<Handler> handleTransactionChanged(TransactionChangedFn fn) override
     {

src/wallet/receive.h

@@ -6,8 +6,8 @@
 #define BITCOIN_WALLET_RECEIVE_H
 
 #include <consensus/amount.h>
-#include <wallet/ismine.h>
 #include <wallet/transaction.h>
+#include <wallet/types.h>
 #include <wallet/wallet.h>
 
 namespace wallet {

src/wallet/rpc/addresses.cpp

@@ -141,9 +141,9 @@ RPCHelpMan setlabel()
     const std::string label{LabelFromValue(request.params[1])};
 
     if (pwallet->IsMine(dest)) {
-        pwallet->SetAddressBook(dest, label, "receive");
+        pwallet->SetAddressBook(dest, label, AddressPurpose::RECEIVE);
     } else {
-        pwallet->SetAddressBook(dest, label, "send");
+        pwallet->SetAddressBook(dest, label, AddressPurpose::SEND);
     }
 
     return UniValue::VNULL;
@@ -285,7 +285,7 @@ RPCHelpMan addmultisigaddress()
     // Construct using pay-to-script-hash:
     CScript inner;
     CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner);
-    pwallet->SetAddressBook(dest, label, "send");
+    pwallet->SetAddressBook(dest, label, AddressPurpose::SEND);
 
     // Make the descriptor
     std::unique_ptr<Descriptor> descriptor = InferDescriptor(GetScriptForDestination(dest), spk_man);
@@ -663,7 +663,7 @@ RPCHelpMan getaddressesbylabel()
     // Find all addresses that have the given label
     UniValue ret(UniValue::VOBJ);
     std::set<std::string> addresses;
-    pwallet->ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, const std::string& _purpose, bool _is_change) {
+    pwallet->ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, bool _is_change, const std::optional<AddressPurpose>& _purpose) {
         if (_is_change) return;
         if (_label == label) {
             std::string address = EncodeDestination(_dest);
@@ -677,7 +677,7 @@ RPCHelpMan getaddressesbylabel()
             // std::set in O(log(N))), UniValue::__pushKV is used instead,
             // which currently is O(1).
             UniValue value(UniValue::VOBJ);
-            value.pushKV("purpose", _purpose);
+            value.pushKV("purpose", _purpose ? PurposeToString(*_purpose) : "unknown");
             ret.__pushKV(address, value);
         }
     });
@@ -721,9 +721,15 @@ RPCHelpMan listlabels()
 
     LOCK(pwallet->cs_wallet);
 
-    std::string purpose;
+    std::optional<AddressPurpose> purpose;
     if (!request.params[0].isNull()) {
-        purpose = request.params[0].get_str();
+        std::string purpose_str = request.params[0].get_str();
+        if (!purpose_str.empty()) {
+            purpose = PurposeFromString(purpose_str);
+            if (!purpose) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid 'purpose' argument, must be a known purpose string, typically 'send', or 'receive'.");
+            }
+        }
     }
 
     // Add to a set to sort by label name, then insert into Univalue array

src/wallet/rpc/backup.cpp

@@ -188,7 +188,7 @@ RPCHelpMan importprivkey()
             // label was passed.
             for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
                 if (!request.params[1].isNull() || !pwallet->FindAddressBookEntry(dest)) {
-                    pwallet->SetAddressBook(dest, strLabel, "receive");
+                    pwallet->SetAddressBook(dest, strLabel, AddressPurpose::RECEIVE);
                 }
             }
 
@@ -608,7 +608,7 @@ RPCHelpMan importwallet()
             }
 
             if (has_label)
-                pwallet->SetAddressBook(PKHash(keyid), label, "receive");
+                pwallet->SetAddressBook(PKHash(keyid), label, AddressPurpose::RECEIVE);
             progress++;
         }
         for (const auto& script_pair : scripts) {

src/wallet/rpc/transactions.cpp

@@ -134,7 +134,7 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
     UniValue ret(UniValue::VARR);
     std::map<std::string, tallyitem> label_tally;
 
-    const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
+    const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) {
         if (is_change) return; // no change addresses
 
         auto it = mapTally.find(address);
@@ -175,7 +175,7 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
 
     if (filtered_address) {
         const auto& entry = wallet.FindAddressBookEntry(*filtered_address, /*allow_change=*/false);
-        if (entry) func(*filtered_address, entry->GetLabel(), entry->purpose, /*is_change=*/false);
+        if (entry) func(*filtered_address, entry->GetLabel(), entry->IsChange(), entry->purpose);
     } else {
         // No filtered addr, walk-through the addressbook entry
         wallet.ForEachAddrBookEntry(func);

src/wallet/scriptpubkeyman.h

@@ -14,7 +14,7 @@
 #include <util/result.h>
 #include <util/time.h>
 #include <wallet/crypter.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 #include <wallet/walletdb.h>
 #include <wallet/walletutil.h>
 

src/wallet/test/ismine_tests.cpp

@@ -8,7 +8,7 @@
 #include <script/script.h>
 #include <script/standard.h>
 #include <test/util/setup_common.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 #include <wallet/wallet.h>
 
 #include <boost/test/unit_test.hpp>

src/wallet/transaction.h

@@ -5,10 +5,12 @@
 #ifndef BITCOIN_WALLET_TRANSACTION_H
 #define BITCOIN_WALLET_TRANSACTION_H
 
+#include <bitset>
+#include <cstdint>
 #include <consensus/amount.h>
 #include <primitives/transaction.h>
 #include <serialize.h>
-#include <wallet/ismine.h>
+#include <wallet/types.h>
 #include <threadsafety.h>
 #include <tinyformat.h>
 #include <util/overloaded.h>
@@ -108,8 +110,29 @@ static inline int TxStateSerializedIndex(const TxState& state)
 }
 
 
+/**
+ * Cachable amount subdivided into watchonly and spendable parts.
+ */
+struct CachableAmount
+{
+    // NO and ALL are never (supposed to be) cached
+    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
+    CAmount m_value[ISMINE_ENUM_ELEMENTS];
+    inline void Reset()
+    {
+        m_cached.reset();
+    }
+    void Set(isminefilter filter, CAmount value)
+    {
+        m_cached.set(filter);
+        m_value[filter] = value;
+    }
+};
+
+
 typedef std::map<std::string, std::string> mapValue_t;
 
+
 /** Legacy class used for deserializing vtxPrev for backwards compatibility.
  * vtxPrev was removed in commit 93a18a3650292afbb441a47d1fa1b94aeb0164e3,
  * but old wallet.dat files may still contain vtxPrev vectors of CMerkleTxs.

src/wallet/types.h

@@ -3,20 +3,19 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#ifndef BITCOIN_WALLET_ISMINE_H
+//! @file Public type definitions that are used inside and outside of the wallet
-#define BITCOIN_WALLET_ISMINE_H
+//! (e.g. by src/wallet and src/interfaces and src/qt code).
+//!
+//! File is home for simple enum and struct definitions that don't deserve
+//! separate header files. More complicated wallet public types like
+//! CCoinControl that are used externally can have separate headers.
 
-#include <script/standard.h>
+#ifndef BITCOIN_WALLET_TYPES_H
+#define BITCOIN_WALLET_TYPES_H
 
-#include <bitset>
-#include <cstdint>
 #include <type_traits>
 
-class CScript;
-
 namespace wallet {
-class CWallet;
-
 /**
  * IsMine() return codes, which depend on ScriptPubKeyMan implementation.
  * Not every ScriptPubKeyMan covers all types, please refer to
@@ -51,23 +50,18 @@ enum isminetype : unsigned int {
 using isminefilter = std::underlying_type<isminetype>::type;
 
 /**
- * Cachable amount subdivided into watchonly and spendable parts.
+ * Address purpose field that has been been stored with wallet sending and
+ * receiving addresses since BIP70 payment protocol support was added in
+ * https://github.com/bitcoin/bitcoin/pull/2539. This field is not currently
+ * used for any logic inside the wallet, but it is still shown in RPC and GUI
+ * interfaces and saved for new addresses. It is basically redundant with an
+ * address's IsMine() result.
  */
-struct CachableAmount
+enum class AddressPurpose {
-{
+    RECEIVE,
-    // NO and ALL are never (supposed to be) cached
+    SEND,
-    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
+    REFUND, //!< Never set in current code may be present in older wallet databases
-    CAmount m_value[ISMINE_ENUM_ELEMENTS];
-    inline void Reset()
-    {
-        m_cached.reset();
-    }
-    void Set(isminefilter filter, CAmount value)
-    {
-        m_cached.set(filter);
-        m_value[filter] = value;
-    }
 };
 } // namespace wallet
 
-#endif // BITCOIN_WALLET_ISMINE_H
+#endif // BITCOIN_WALLET_TYPES_H

src/wallet/wallet.cpp

@@ -1229,7 +1229,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const SyncTxS
                         // (e.g. it wasn't generated on this node or we're restoring from backup)
                         // add it to the address book for proper transaction accounting
                         if (!*dest.internal && !FindAddressBookEntry(dest.dest, /* allow_change= */ false)) {
-                            SetAddressBook(dest.dest, "", "receive");
+                            SetAddressBook(dest.dest, "", AddressPurpose::RECEIVE);
                         }
                     }
                 }
@@ -1777,7 +1777,7 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
             CTxDestination dest;
             ExtractDestination(script, dest);
             if (IsValidDestination(dest)) {
-                SetAddressBookWithDB(batch, dest, label, "receive");
+                SetAddressBookWithDB(batch, dest, label, AddressPurpose::RECEIVE);
             }
         }
     }
@@ -2400,35 +2400,40 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
     return DBErrors::LOAD_OK;
 }
 
-bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::string& strPurpose)
+bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& new_purpose)
 {
     bool fUpdated = false;
     bool is_mine;
+    std::optional<AddressPurpose> purpose;
     {
         LOCK(cs_wallet);
         std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
         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;
         is_mine = IsMine(address) != ISMINE_NO;
+        if (new_purpose) { /* update purpose only if requested */
+            purpose = m_address_book[address].purpose = new_purpose;
+        } else {
+            purpose = m_address_book[address].purpose;
+        }
     }
+    // In very old wallets, address purpose may not be recorded so we derive it from IsMine
     NotifyAddressBookChanged(address, strName, is_mine,
-                             strPurpose, (fUpdated ? CT_UPDATED : CT_NEW));
+                             purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND),
-    if (!strPurpose.empty() && !batch.WritePurpose(EncodeDestination(address), strPurpose))
+                             (fUpdated ? CT_UPDATED : CT_NEW));
+    if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose)))
         return false;
     return batch.WriteName(EncodeDestination(address), strName);
 }
 
-bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& strPurpose)
+bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose)
 {
     WalletBatch batch(GetDatabase());
-    return SetAddressBookWithDB(batch, address, strName, strPurpose);
+    return SetAddressBookWithDB(batch, address, strName, purpose);
 }
 
 bool CWallet::DelAddressBook(const CTxDestination& address)
 {
-    bool is_mine;
     WalletBatch batch(GetDatabase());
     {
         LOCK(cs_wallet);
@@ -2446,10 +2451,9 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
             batch.EraseDestData(strAddress, item.first);
         }
         m_address_book.erase(address);
-        is_mine = IsMine(address) != ISMINE_NO;
     }
 
-    NotifyAddressBookChanged(address, "", is_mine, "", CT_DELETED);
+    NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED);
 
     batch.ErasePurpose(EncodeDestination(address));
     return batch.EraseName(EncodeDestination(address));
@@ -2503,7 +2507,7 @@ util::Result<CTxDestination> CWallet::GetNewDestination(const OutputType type, c
 
     auto op_dest = spk_man->GetNewDestination(type);
     if (op_dest) {
-        SetAddressBook(*op_dest, label, "receive");
+        SetAddressBook(*op_dest, label, AddressPurpose::RECEIVE);
     }
 
     return op_dest;
@@ -2553,7 +2557,7 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const
     AssertLockHeld(cs_wallet);
     for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book) {
         const auto& entry = item.second;
-        func(item.first, entry.GetLabel(), entry.purpose, entry.IsChange());
+        func(item.first, entry.GetLabel(), entry.IsChange(), entry.purpose);
     }
 }
 
@@ -2562,7 +2566,7 @@ std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<A
     AssertLockHeld(cs_wallet);
     std::vector<CTxDestination> result;
     AddrBookFilter filter = _filter ? *_filter : AddrBookFilter();
-    ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) {
+    ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) {
         // Filter by change
         if (filter.ignore_change && is_change) return;
         // Filter by label
@@ -2573,14 +2577,14 @@ std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<A
     return result;
 }
 
-std::set<std::string> CWallet::ListAddrBookLabels(const std::string& purpose) const
+std::set<std::string> CWallet::ListAddrBookLabels(const std::optional<AddressPurpose> purpose) const
 {
     AssertLockHeld(cs_wallet);
     std::set<std::string> label_set;
     ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label,
-                             const std::string& _purpose, bool _is_change) {
+                             bool _is_change, const std::optional<AddressPurpose>& _purpose) {
         if (_is_change) return;
-        if (purpose.empty() || _purpose == purpose) {
+        if (!purpose || purpose == _purpose) {
             label_set.insert(_label);
         }
     });
@@ -3792,7 +3796,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
             for (const auto& script : script_pub_keys) {
                 CTxDestination dest;
                 if (ExtractDestination(script, dest)) {
-                    SetAddressBook(dest, label, "receive");
+                    SetAddressBook(dest, label, AddressPurpose::RECEIVE);
                 }
             }
         }
@@ -3978,7 +3982,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
     std::vector<CTxDestination> dests_to_delete;
     for (const auto& addr_pair : m_address_book) {
         // Labels applied to receiving addresses should go based on IsMine
-        if (addr_pair.second.purpose == "receive") {
+        if (addr_pair.second.purpose == AddressPurpose::RECEIVE) {
             if (!IsMine(addr_pair.first)) {
                 // Check the address book data is the watchonly wallet's
                 if (data.watchonly_wallet) {
@@ -3986,10 +3990,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                     if (data.watchonly_wallet->IsMine(addr_pair.first)) {
                         // Add to the watchonly. Preserve the labels, purpose, and change-ness
                         std::string label = addr_pair.second.GetLabel();
-                        std::string purpose = addr_pair.second.purpose;
+                        data.watchonly_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose;
-                        if (!purpose.empty()) {
-                            data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
-                        }
                         if (!addr_pair.second.IsChange()) {
                             data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label);
                         }
@@ -4002,10 +4003,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                     if (data.solvable_wallet->IsMine(addr_pair.first)) {
                         // Add to the solvable. Preserve the labels, purpose, and change-ness
                         std::string label = addr_pair.second.GetLabel();
-                        std::string purpose = addr_pair.second.purpose;
+                        data.solvable_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose;
-                        if (!purpose.empty()) {
-                            data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
-                        }
                         if (!addr_pair.second.IsChange()) {
                             data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label);
                         }
@@ -4023,10 +4021,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                 LOCK(data.watchonly_wallet->cs_wallet);
                 // Add to the watchonly. Preserve the labels, purpose, and change-ness
                 std::string label = addr_pair.second.GetLabel();
-                std::string purpose = addr_pair.second.purpose;
+                data.watchonly_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose;
-                if (!purpose.empty()) {
-                    data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
-                }
                 if (!addr_pair.second.IsChange()) {
                     data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label);
                 }
@@ -4036,10 +4031,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                 LOCK(data.solvable_wallet->cs_wallet);
                 // Add to the solvable. Preserve the labels, purpose, and change-ness
                 std::string label = addr_pair.second.GetLabel();
-                std::string purpose = addr_pair.second.purpose;
+                data.solvable_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose;
-                if (!purpose.empty()) {
-                    data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
-                }
                 if (!addr_pair.second.IsChange()) {
                     data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label);
                 }
@@ -4054,10 +4046,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
         WalletBatch batch{wallet.GetDatabase()};
         for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
             auto address{EncodeDestination(destination)};
-            auto purpose{addr_book_data.purpose};
             auto label{addr_book_data.GetLabel()};
             // don't bother writing default values (unknown purpose, empty label)
-            if (purpose != "unknown") batch.WritePurpose(address, purpose);
+            if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));
             if (!label.empty()) batch.WriteName(address, label);
         }
     };

src/wallet/wallet.h

@@ -9,6 +9,7 @@
 #include <consensus/amount.h>
 #include <interfaces/chain.h>
 #include <interfaces/handler.h>
+#include <interfaces/wallet.h>
 #include <logging.h>
 #include <outputtype.h>
 #include <policy/feerate.h>
@@ -200,28 +201,64 @@ public:
     void KeepDestination();
 };
 
-/** Address book data */
+/**
-class CAddressBookData
+ * Address book data.
+ */
+struct CAddressBookData
 {
-private:
+    /**
-    bool m_change{true};
+     * Address label which is always nullopt for change addresses. For sending
-    std::string m_label;
+     * and receiving addresses, it will be set to an arbitrary label string
-public:
+     * provided by the user, or to "", which is the default label. The presence
-    std::string purpose;
+     * or absence of a label is used to distinguish change addresses from
+     * non-change addresses by wallet transaction listing and fee bumping code.
+     */
+    std::optional<std::string> label;
 
-    CAddressBookData() : purpose("unknown") {}
+    /**
+     * Address purpose which was originally recorded for payment protocol
+     * support but now serves as a cached IsMine value. Wallet code should
+     * not rely on this field being set.
+     */
+    std::optional<AddressPurpose> purpose;
 
+    /**
+     * Additional address metadata map that can currently hold two types of keys:
+     *
+     *   "used" keys with values always set to "1" or "p" if present. This is set on
+     *       IsMine addresses that have already been spent from if the
+     *       avoid_reuse option is enabled
+     *
+     *   "rr##" keys where ## is a decimal number, with serialized
+     *       RecentRequestEntry objects as values
+     */
     typedef std::map<std::string, std::string> StringMap;
     StringMap destdata;
 
-    bool IsChange() const { return m_change; }
+    /** Accessor methods. */
-    const std::string& GetLabel() const { return m_label; }
+    bool IsChange() const { return !label.has_value(); }
-    void SetLabel(const std::string& label) {
+    std::string GetLabel() const { return label ? *label : std::string{}; }
-        m_change = false;
+    void SetLabel(std::string name) { label = std::move(name); }
-        m_label = label;
-    }
 };
 
+inline std::string PurposeToString(AddressPurpose p)
+{
+    switch(p) {
+    case AddressPurpose::RECEIVE: return "receive";
+    case AddressPurpose::SEND: return "send";
+    case AddressPurpose::REFUND: return "refund";
+    } // no default case so the compiler will warn when a new enum as added
+    assert(false);
+}
+
+inline std::optional<AddressPurpose> PurposeFromString(std::string_view s)
+{
+    if (s == "receive") return AddressPurpose::RECEIVE;
+    else if (s == "send") return AddressPurpose::SEND;
+    else if (s == "refund") return AddressPurpose::REFUND;
+    return {};
+}
+
 struct CRecipient
 {
     CScript scriptPubKey;
@@ -300,7 +337,7 @@ private:
     /** WalletFlags set on this wallet. */
     std::atomic<uint64_t> m_wallet_flags{0};
 
-    bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::string& strPurpose);
+    bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& strPurpose);
 
     //! Unsets a wallet flag and saves it to disk
     void UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag);
@@ -664,13 +701,13 @@ public:
     /**
      * Retrieve all the known labels in the address book
      */
-    std::set<std::string> ListAddrBookLabels(const std::string& purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    std::set<std::string> ListAddrBookLabels(const std::optional<AddressPurpose> purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /**
      * Walk-through the address book entries.
      * Stops when the provided 'ListAddrBookFunc' returns false.
      */
-    using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>;
+    using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, bool is_change, const std::optional<AddressPurpose> purpose)>;
     void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /**
@@ -700,7 +737,7 @@ public:
     DBErrors LoadWallet();
     DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& purpose);
+    bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose);
 
     bool DelAddressBook(const CTxDestination& address);
 
@@ -739,7 +776,7 @@ public:
      */
     boost::signals2::signal<void(const CTxDestination& address,
                                  const std::string& label, bool isMine,
-                                 const std::string& purpose, ChangeType status)>
+                                 AddressPurpose purpose, ChangeType status)>
         NotifyAddressBookChanged;
 
     /**

src/wallet/walletdb.cpp

@@ -347,7 +347,13 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
         } else if (strType == DBKeys::PURPOSE) {
             std::string strAddress;
             ssKey >> strAddress;
-            ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].purpose;
+            std::string purpose_str;
+            ssValue >> purpose_str;
+            std::optional<AddressPurpose> purpose{PurposeFromString(purpose_str)};
+            if (!purpose) {
+                pwallet->WalletLogPrintf("Warning: nonstandard purpose string '%s' for address '%s'\n", purpose_str, strAddress);
+            }
+            pwallet->m_address_book[DecodeDestination(strAddress)].purpose = purpose;
         } else if (strType == DBKeys::TX) {
             uint256 hash;
             ssKey >> hash;

test/functional/wallet_labels.py

@@ -71,6 +71,10 @@ class WalletLabelsTest(BitcoinTestFramework):
         node = self.nodes[0]
         assert_equal(len(node.listunspent()), 0)
 
+        self.log.info("Checking listlabels' invalid parameters")
+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be a known purpose string, typically 'send', or 'receive'.", node.listlabels, "notavalidpurpose")
+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be a known purpose string, typically 'send', or 'receive'.", node.listlabels, "unknown")
+
         # Note each time we call generate, all generated coins go into
         # the same address, so we call twice to get two addresses w/50 each
         self.generatetoaddress(node, nblocks=1, address=node.getnewaddress(label='coinbase'))