diff --git a/doc/release-notes/release-notes-27216.md b/doc/release-notes/release-notes-27216.md new file mode 100644 index 00000000000..9250cf1711d --- /dev/null +++ b/doc/release-notes/release-notes-27216.md @@ -0,0 +1,8 @@ +Updated RPCs +------------ + +- RPC `getaddressinfo` now returns a new boolean field `"isactive"`, which is +only `true` if the address is derived from an active descriptor or HD seed. +This information can inform a user to avoid reusing certain addresses that may +have been imported from external sources or generated by their wallet before it +was encrypted. (#27216) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 8c31112fc94..2fc2a1448a1 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -46,6 +46,7 @@ namespace interfaces { class Handler; struct WalletAddress; +struct ReceiveRequest; struct WalletBalances; struct WalletTx; struct WalletTxOut; @@ -118,8 +119,8 @@ class Wallet //! Get wallet address list. virtual std::vector getAddresses() const = 0; - //! Get receive requests. - virtual std::vector getAddressReceiveRequests() = 0; + //! Get receive requests. Bool indicating key is active, string containing serialized destination data + virtual std::vector getAddressReceiveRequests() = 0; //! Save or remove receive request. virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; @@ -292,10 +293,12 @@ class Wallet //! Register handler for address book changed messages. using AddressBookChangedFn = std::function; + const std::string& label, + bool is_mine, + wallet::AddressPurpose purpose, + ChangeType status, + bool is_active)>; + virtual std::unique_ptr handleAddressBookChanged(AddressBookChangedFn fn) = 0; //! Register handler for transaction changed messages. @@ -355,13 +358,25 @@ struct WalletAddress wallet::isminetype is_mine; wallet::AddressPurpose purpose; std::string name; + bool is_active{false}; - WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name) - : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name)) + WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name, bool is_active) + : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name)), is_active(is_active) { } }; +//! Information about one receive request. +struct ReceiveRequest { + std::string m_data; + bool m_is_active; + + ReceiveRequest(std::string data, bool is_active) + : m_data(std::move(data)), m_is_active(is_active) {} + + bool operator==(const ReceiveRequest& a) const { return (m_data == a.m_data) && (m_is_active == a.m_is_active); }; +}; + //! Collection of wallet balances. struct WalletBalances { diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 0d0f1a4d154..a5d838685ca 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -30,10 +30,12 @@ struct AddressTableEntry Type type; QString label; QString address; + bool is_active; AddressTableEntry() = default; - AddressTableEntry(Type _type, const QString &_label, const QString &_address): - type(_type), label(_label), address(_address) {} + AddressTableEntry(Type _type, const QString& _label, const QString& _address, bool is_active) : type(_type), label(_label), address(_address), is_active(is_active) {} + + [[nodiscard]] QString GetAddressWarnings() const; }; struct AddressTableEntryLessThan @@ -65,6 +67,15 @@ static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose p assert(false); } +[[nodiscard]] QString AddressTableEntry::GetAddressWarnings() const +{ + QString warnings; + if (!is_active) + warnings += QObject::tr("This address should not be used. It was derived from an inactive seed, was imported, or may have been stored unencrypted.\n"); + + return warnings; +} + // Private implementation class AddressTablePriv { @@ -87,8 +98,9 @@ class AddressTablePriv AddressTableEntry::Type addressType = translateTransactionType( address.purpose, address.is_mine); cachedAddressTable.append(AddressTableEntry(addressType, - QString::fromStdString(address.name), - QString::fromStdString(EncodeDestination(address.dest)))); + QString::fromStdString(address.name), + QString::fromStdString(EncodeDestination(address.dest)), + address.is_active)); } } // std::lower_bound() and std::upper_bound() require our cachedAddressTable list to be sorted in asc order @@ -97,7 +109,7 @@ class AddressTablePriv std::sort(cachedAddressTable.begin(), cachedAddressTable.end(), AddressTableEntryLessThan()); } - void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status) + void updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { // Find address / label in model QList::iterator lower = std::lower_bound( @@ -118,7 +130,7 @@ class AddressTablePriv break; } parent->beginInsertRows(QModelIndex(), lowerIndex, lowerIndex); - cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address)); + cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address, isActive)); parent->endInsertRows(); break; case CT_UPDATED: @@ -162,10 +174,9 @@ class AddressTablePriv } }; -AddressTableModel::AddressTableModel(WalletModel *parent, bool pk_hash_only) : - QAbstractTableModel(parent), walletModel(parent) +AddressTableModel::AddressTableModel(const PlatformStyle* platformStyle, WalletModel* parent, bool pk_hash_only) : QAbstractTableModel(parent), walletModel(parent), platformStyle(platformStyle) { - columns << tr("Label") << tr("Address"); + columns << tr("Warnings") << tr("Label") << tr("Address"); priv = new AddressTablePriv(this); priv->refreshAddressTable(parent->wallet(), pk_hash_only); } @@ -201,6 +212,8 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const const auto column = static_cast(index.column()); if (role == Qt::DisplayRole || role == Qt::EditRole) { switch (column) { + case Warnings: + return {}; case Label: if (rec->label.isEmpty() && role == Qt::DisplayRole) { return tr("(no label)"); @@ -213,6 +226,7 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const assert(false); } else if (role == Qt::FontRole) { switch (column) { + case Warnings: case Label: return QFont(); case Address: @@ -230,6 +244,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const return {}; } // no default case, so the compiler can warn about missing cases assert(false); + } else if (role == Qt::DecorationRole) { + if (index.column() == Warnings) { + if (rec->GetAddressWarnings().isEmpty()) { + return {}; + } else { + return platformStyle->TextColorIcon(QIcon(":/icons/warning")); + } + } + } else if (role == Qt::ToolTipRole) { + if (index.column() == Warnings) return rec->GetAddressWarnings(); } return QVariant(); } @@ -333,11 +357,10 @@ QModelIndex AddressTableModel::index(int row, int column, const QModelIndex &par } } -void AddressTableModel::updateEntry(const QString &address, - const QString &label, bool isMine, wallet::AddressPurpose purpose, int status) +void AddressTableModel::updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { // Update address book model from Bitcoin core - priv->updateEntry(address, label, isMine, purpose, status); + priv->updateEntry(address, label, isMine, purpose, status, isActive); } QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address, const OutputType address_type) diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h index 599aa89cadd..88ada425385 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -6,6 +6,7 @@ #define BITCOIN_QT_ADDRESSTABLEMODEL_H #include +#include #include #include @@ -30,12 +31,13 @@ class AddressTableModel : public QAbstractTableModel Q_OBJECT public: - explicit AddressTableModel(WalletModel *parent = nullptr, bool pk_hash_only = false); + explicit AddressTableModel(const PlatformStyle* platformStyle, WalletModel* parent = nullptr, bool pk_hash_only = false); ~AddressTableModel(); enum ColumnIndex { - Label = 0, /**< User specified label */ - Address = 1 /**< Bitcoin address */ + Warnings = 0, /**< Warn user about using this address with tooltip explanation */ + Label, /**< User specified label */ + Address /**< Bitcoin address */ }; enum RoleIndex { @@ -92,6 +94,7 @@ class AddressTableModel : public QAbstractTableModel AddressTablePriv *priv = nullptr; QStringList columns; EditStatus editStatus = OK; + const PlatformStyle* platformStyle; /** Look up address book data given an address string. */ bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const; @@ -102,7 +105,7 @@ class AddressTableModel : public QAbstractTableModel public Q_SLOTS: /* Update address list from core. */ - void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status); + void updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive); friend class AddressTablePriv; }; diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index d26ef52eb43..15c77612b30 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -1418,6 +1418,9 @@ void BitcoinGUI::updateWalletStatus() WalletModel * const walletModel = walletView->getWalletModel(); setEncryptionStatus(walletModel->getEncryptionStatus()); setHDStatus(walletModel->wallet().privateKeysDisabled(), walletModel->wallet().hdEnabled()); + + // Refresh requests table and address book to show warnings on old unencrypted keys + walletView->refreshAddressTables(); } #endif // ENABLE_WALLET diff --git a/src/qt/forms/receiverequestdialog.ui b/src/qt/forms/receiverequestdialog.ui index 70a7cf71de9..f1dbb4401b6 100644 --- a/src/qt/forms/receiverequestdialog.ui +++ b/src/qt/forms/receiverequestdialog.ui @@ -232,7 +232,39 @@ - + + + + + 75 + true + + + + Warnings: + + + Qt::NoTextInteraction + + + + + + + warnings + + + Qt::PlainText + + + true + + + Qt::TextSelectableByMouse + + + + diff --git a/src/qt/receivecoinsdialog.cpp b/src/qt/receivecoinsdialog.cpp index be7741e8a8c..e135748119c 100644 --- a/src/qt/receivecoinsdialog.cpp +++ b/src/qt/receivecoinsdialog.cpp @@ -160,14 +160,19 @@ void ReceiveCoinsDialog::on_receiveButton_clicked() // Success SendCoinsRecipient info(address, label, ui->reqAmount->value(), ui->reqMessage->text()); + + /* Store request for later reference */ + std::optional entry = model->getRecentRequestsTableModel()->addNewRequest(info); + + // Wallet failed to set address receive request + if (!entry.has_value()) break; + ReceiveRequestDialog *dialog = new ReceiveRequestDialog(this); dialog->setAttribute(Qt::WA_DeleteOnClose); dialog->setModel(model); - dialog->setInfo(info); + dialog->setInfo(entry.value()); dialog->show(); - /* Store request for later reference */ - model->getRecentRequestsTableModel()->addNewRequest(info); break; } case AddressTableModel::EditStatus::WALLET_UNLOCK_FAILURE: @@ -194,7 +199,7 @@ void ReceiveCoinsDialog::on_recentRequestsView_doubleClicked(const QModelIndex & const RecentRequestsTableModel *submodel = model->getRecentRequestsTableModel(); ReceiveRequestDialog *dialog = new ReceiveRequestDialog(this); dialog->setModel(model); - dialog->setInfo(submodel->entry(index.row()).recipient); + dialog->setInfo(submodel->entry(index.row())); dialog->setAttribute(Qt::WA_DeleteOnClose); dialog->show(); } diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index 3453857f985..dfe06237bd5 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/src/qt/receiverequestdialog.cpp @@ -42,9 +42,9 @@ void ReceiveRequestDialog::setModel(WalletModel *_model) update(); } -void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) +void ReceiveRequestDialog::setInfo(const RecentRequestEntry& entry) { - this->info = _info; + this->info = entry.recipient; setWindowTitle(tr("Request payment to %1").arg(info.label.isEmpty() ? info.address : info.label)); QString uri = GUIUtil::formatBitcoinURI(info); @@ -89,6 +89,14 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) ui->wallet_content->hide(); } + QString warnings{entry.GetAddressWarnings()}; + if (warnings.isEmpty()) { + ui->warnings_tag->hide(); + ui->warnings_content->hide(); + } else { + ui->warnings_content->setText(warnings); + } + ui->btnVerify->setVisible(model->wallet().hasExternalSigner()); connect(ui->btnVerify, &QPushButton::clicked, [this] { diff --git a/src/qt/receiverequestdialog.h b/src/qt/receiverequestdialog.h index d64fee36636..6a64209e70f 100644 --- a/src/qt/receiverequestdialog.h +++ b/src/qt/receiverequestdialog.h @@ -5,7 +5,7 @@ #ifndef BITCOIN_QT_RECEIVEREQUESTDIALOG_H #define BITCOIN_QT_RECEIVEREQUESTDIALOG_H -#include +#include #include @@ -24,7 +24,7 @@ class ReceiveRequestDialog : public QDialog ~ReceiveRequestDialog(); void setModel(WalletModel *model); - void setInfo(const SendCoinsRecipient &info); + void setInfo(const RecentRequestEntry& entry); private Q_SLOTS: void on_btnCopyURI_clicked(); diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp index 52d4e45d49f..6ba53cb8668 100644 --- a/src/qt/recentrequeststablemodel.cpp +++ b/src/qt/recentrequeststablemodel.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -17,19 +18,29 @@ #include +#include #include #include -RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) : - QAbstractTableModel(parent), walletModel(parent) +[[nodiscard]] QString RecentRequestEntry::GetAddressWarnings() const +{ + QString warnings; + if (!m_is_active) { + warnings += QObject::tr("This address should not be used. It was derived from an inactive seed, was imported, or may have been stored unencrypted.\n"); + } + + return warnings; +} + +RecentRequestsTableModel::RecentRequestsTableModel(const PlatformStyle* platformStyle, WalletModel* parent) : QAbstractTableModel(parent), walletModel(parent) { // Load entries from wallet - for (const std::string& request : parent->wallet().getAddressReceiveRequests()) { + for (const interfaces::ReceiveRequest& request : parent->wallet().getAddressReceiveRequests()) { addNewRequest(request); } /* These columns must match the indices in the ColumnIndex enumeration */ - columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle(); + columns << tr("Date") << tr("Warnings") << tr("Label") << tr("Message") << getAmountTitle(); connect(walletModel->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &RecentRequestsTableModel::updateDisplayUnit); } @@ -57,11 +68,13 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons if(!index.isValid() || index.row() >= list.length()) return QVariant(); + const RecentRequestEntry* rec = &list[index.row()]; if(role == Qt::DisplayRole || role == Qt::EditRole) { - const RecentRequestEntry *rec = &list[index.row()]; switch(index.column()) { + case Warnings: + return {}; case Date: return GUIUtil::dateTimeStr(rec->date); case Label: @@ -95,6 +108,16 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons { if (index.column() == Amount) return (int)(Qt::AlignRight|Qt::AlignVCenter); + } else if (role == Qt::DecorationRole) { + if (index.column() == Warnings) { + if (rec->GetAddressWarnings().isEmpty()) { + return {}; + } else { + return platformStyle->TextColorIcon(QIcon(":/icons/warning")); + } + } + } else if (role == Qt::ToolTipRole) { + if (index.column() == Warnings) return rec->GetAddressWarnings(); } return QVariant(); } @@ -168,7 +191,7 @@ Qt::ItemFlags RecentRequestsTableModel::flags(const QModelIndex &index) const } // called when adding a request from the GUI -void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient) +std::optional RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient& recipient) { RecentRequestEntry newEntry; newEntry.id = ++nReceiveRequestsMaxId; @@ -179,35 +202,44 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient ss << newEntry; if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str())) - return; + return std::nullopt; + + // If we are using the GUI to get a new receive address, + // the key must be active (derived from active seed or descriptor) + newEntry.m_is_active = true; addNewRequest(newEntry); + return newEntry; } // called from ctor when loading from wallet -void RecentRequestsTableModel::addNewRequest(const std::string &recipient) +std::optional RecentRequestsTableModel::addNewRequest(const interfaces::ReceiveRequest& recipient) { - std::vector data(recipient.begin(), recipient.end()); + std::vector data(recipient.m_data.begin(), recipient.m_data.end()); DataStream ss{data}; RecentRequestEntry entry; ss >> entry; if (entry.id == 0) // should not happen - return; + return std::nullopt; if (entry.id > nReceiveRequestsMaxId) nReceiveRequestsMaxId = entry.id; + entry.m_is_active = recipient.m_is_active; + addNewRequest(entry); + return entry; } // actually add to table in GUI -void RecentRequestsTableModel::addNewRequest(RecentRequestEntry &recipient) +std::optional RecentRequestsTableModel::addNewRequest(RecentRequestEntry& recipient) { beginInsertRows(QModelIndex(), 0, 0); list.prepend(recipient); endInsertRows(); + return recipient; } void RecentRequestsTableModel::sort(int column, Qt::SortOrder order) diff --git a/src/qt/recentrequeststablemodel.h b/src/qt/recentrequeststablemodel.h index 151f8322a8a..f0f57bf6aaa 100644 --- a/src/qt/recentrequeststablemodel.h +++ b/src/qt/recentrequeststablemodel.h @@ -5,8 +5,12 @@ #ifndef BITCOIN_QT_RECENTREQUESTSTABLEMODEL_H #define BITCOIN_QT_RECENTREQUESTSTABLEMODEL_H +#include + +#include #include +#include #include #include @@ -26,12 +30,16 @@ class RecentRequestEntry QDateTime date; SendCoinsRecipient recipient; + bool m_is_active{false}; // memory only + SERIALIZE_METHODS(RecentRequestEntry, obj) { unsigned int date_timet; SER_WRITE(obj, date_timet = obj.date.toSecsSinceEpoch()); READWRITE(obj.nVersion, obj.id, date_timet, obj.recipient); SER_READ(obj, obj.date = QDateTime::fromSecsSinceEpoch(date_timet)); } + + [[nodiscard]] QString GetAddressWarnings() const; }; class RecentRequestEntryLessThan @@ -54,14 +62,15 @@ class RecentRequestsTableModel: public QAbstractTableModel Q_OBJECT public: - explicit RecentRequestsTableModel(WalletModel *parent); + explicit RecentRequestsTableModel(const PlatformStyle* platformStyle, WalletModel* parent); ~RecentRequestsTableModel(); enum ColumnIndex { Date = 0, - Label = 1, - Message = 2, - Amount = 3, + Warnings, + Label, + Message, + Amount, NUMBER_OF_COLUMNS }; @@ -79,9 +88,9 @@ class RecentRequestsTableModel: public QAbstractTableModel /*@}*/ const RecentRequestEntry &entry(int row) const { return list[row]; } - void addNewRequest(const SendCoinsRecipient &recipient); - void addNewRequest(const std::string &recipient); - void addNewRequest(RecentRequestEntry &recipient); + std::optional addNewRequest(const SendCoinsRecipient& recipient); + std::optional addNewRequest(const interfaces::ReceiveRequest& recipient); + std::optional addNewRequest(RecentRequestEntry& recipient); public Q_SLOTS: void updateDisplayUnit(); @@ -91,6 +100,7 @@ public Q_SLOTS: QStringList columns; QList list; int64_t nReceiveRequestsMaxId{0}; + const PlatformStyle* platformStyle; /** Updates the column title to "Amount (DisplayUnit)" and emits headerDataChanged() signal for table headers to react. */ void updateAmountColumnTitle(); diff --git a/src/qt/signverifymessagedialog.cpp b/src/qt/signverifymessagedialog.cpp index 0e725acb33e..6a4bc0cb5fa 100644 --- a/src/qt/signverifymessagedialog.cpp +++ b/src/qt/signverifymessagedialog.cpp @@ -90,7 +90,7 @@ void SignVerifyMessageDialog::on_addressBookButton_SM_clicked() { if (model && model->getAddressTableModel()) { - model->refresh(/*pk_hash_only=*/true); + model->RefreshAddressTableModel(platformStyle, /*pk_hash_only=*/true); AddressBookPage dlg(platformStyle, AddressBookPage::ForSelection, AddressBookPage::ReceivingTab, this); dlg.setModel(model->getAddressTableModel()); if (dlg.exec()) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 072718fe151..5a718391d30 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -5,9 +5,9 @@ #include #include -#include #include #include +#include #include #include #include @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -366,10 +367,11 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr& wallet) QCOMPARE(currentRowCount, initialRowCount+1); // Check addition to wallet - std::vector requests = walletModel.wallet().getAddressReceiveRequests(); + std::vector requests = walletModel.wallet().getAddressReceiveRequests(); QCOMPARE(requests.size(), size_t{1}); + QVERIFY(requests[0].m_is_active); // Key was derived from active seed or descriptor RecentRequestEntry entry; - DataStream{MakeUCharSpan(requests[0])} >> entry; + DataStream{MakeUCharSpan(requests[0].m_data)} >> entry; QCOMPARE(entry.nVersion, int{1}); QCOMPARE(entry.id, int64_t{1}); QVERIFY(entry.date.isValid()); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ee3327530c4..76008af53ff 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -49,9 +49,9 @@ WalletModel::WalletModel(std::unique_ptr wallet, ClientModel timer(new QTimer(this)) { fHaveWatchOnly = m_wallet->haveWatchOnly(); - addressTableModel = new AddressTableModel(this); + addressTableModel = new AddressTableModel(platformStyle, this); transactionTableModel = new TransactionTableModel(platformStyle, this); - recentRequestsTableModel = new RecentRequestsTableModel(this); + recentRequestsTableModel = new RecentRequestsTableModel(platformStyle, this); subscribeToCoreSignals(); } @@ -137,11 +137,10 @@ void WalletModel::updateTransaction() fForceCheckBalanceChanged = true; } -void WalletModel::updateAddressBook(const QString &address, const QString &label, - bool isMine, wallet::AddressPurpose purpose, int status) +void WalletModel::updateAddressBook(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { if(addressTableModel) - addressTableModel->updateEntry(address, label, isMine, purpose, status); + addressTableModel->updateEntry(address, label, isMine, purpose, status, isActive); } void WalletModel::updateWatchOnlyFlag(bool fHaveWatchonly) @@ -375,20 +374,19 @@ static void NotifyKeyStoreStatusChanged(WalletModel *walletmodel) assert(invoked); } -static void NotifyAddressBookChanged(WalletModel *walletmodel, - const CTxDestination &address, const std::string &label, bool isMine, - wallet::AddressPurpose purpose, ChangeType status) +static void NotifyAddressBookChanged(WalletModel* walletmodel, const CTxDestination& address, const std::string& label, bool isMine, wallet::AddressPurpose purpose, ChangeType status, bool isActive) { QString strAddress = QString::fromStdString(EncodeDestination(address)); QString strLabel = QString::fromStdString(label); - qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + QString::number(static_cast(purpose)) + " status=" + QString::number(status); + qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + QString::number(static_cast(purpose)) + " status=" + QString::number(status) + " isActive=" + QString::number(isActive); bool invoked = QMetaObject::invokeMethod(walletmodel, "updateAddressBook", - Q_ARG(QString, strAddress), - Q_ARG(QString, strLabel), - Q_ARG(bool, isMine), - Q_ARG(wallet::AddressPurpose, purpose), - Q_ARG(int, status)); + Q_ARG(QString, strAddress), + Q_ARG(QString, strLabel), + Q_ARG(bool, isMine), + Q_ARG(wallet::AddressPurpose, purpose), + Q_ARG(int, status), + Q_ARG(bool, isActive)); assert(invoked); } @@ -427,7 +425,7 @@ void WalletModel::subscribeToCoreSignals() // Connect signals to wallet m_handler_unload = m_wallet->handleUnload(std::bind(&NotifyUnload, this)); m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this)); - m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5)); + m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5, std::placeholders::_6)); m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2)); m_handler_show_progress = m_wallet->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2)); m_handler_watch_only_changed = m_wallet->handleWatchOnlyChanged(std::bind(NotifyWatchonlyChanged, this, std::placeholders::_1)); @@ -600,9 +598,14 @@ bool WalletModel::isMultiwallet() const return m_node.walletLoader().getWallets().size() > 1; } -void WalletModel::refresh(bool pk_hash_only) +void WalletModel::RefreshAddressTableModel(const PlatformStyle* platformStyle, bool pk_hash_only) { - addressTableModel = new AddressTableModel(this, pk_hash_only); + addressTableModel = new AddressTableModel(platformStyle, this, pk_hash_only); +} + +void WalletModel::RefreshRecentRequestsTableModel(const PlatformStyle* platformStyle) +{ + recentRequestsTableModel = new RecentRequestsTableModel(platformStyle, this); } uint256 WalletModel::getLastBlockProcessed() const diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 4f75d41404a..379c9f7cc25 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -149,7 +149,8 @@ class WalletModel : public QObject bool isMultiwallet() const; - void refresh(bool pk_hash_only = false); + void RefreshAddressTableModel(const PlatformStyle* platformStyle, bool pk_hash_only = false); + void RefreshRecentRequestsTableModel(const PlatformStyle* platformStyle); uint256 getLastBlockProcessed() const; @@ -236,7 +237,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, wallet::AddressPurpose purpose, int status); + void updateAddressBook(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive); /* Watch-only added */ void updateWatchOnlyFlag(bool fHaveWatchonly); /* Current, immature or unconfirmed balance might have changed - emit 'balanceChanged' if so */ diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 09e07715340..f4115acbf79 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -200,6 +200,14 @@ void WalletView::showOutOfSyncWarning(bool fShow) overviewPage->showOutOfSyncWarning(fShow); } +void WalletView::refreshAddressTables() +{ + walletModel->RefreshRecentRequestsTableModel(platformStyle); + receiveCoinsPage->setModel(walletModel); + walletModel->RefreshAddressTableModel(platformStyle); + usedReceivingAddressesPage->setModel(walletModel->getAddressTableModel()); +} + void WalletView::encryptWallet() { auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Encrypt, this); diff --git a/src/qt/walletview.h b/src/qt/walletview.h index 475085044db..c571a290a6a 100644 --- a/src/qt/walletview.h +++ b/src/qt/walletview.h @@ -48,6 +48,7 @@ class WalletView : public QStackedWidget bool handlePaymentRequest(const SendCoinsRecipient& recipient); void showOutOfSyncWarning(bool fShow); + void refreshAddressTables(); private: ClientModel* clientModel{nullptr}; diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 0951504ee02..04315f73270 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -730,8 +730,11 @@ class PKDescriptor final : public DescriptorImpl private: const bool m_xonly; protected: - std::vector MakeScripts(const std::vector& keys, Span, FlatSigningProvider&) const override + std::vector MakeScripts(const std::vector& keys, Span, FlatSigningProvider& out) const override { + CKeyID id = keys[0].GetID(); + out.pubkeys.emplace(id, keys[0]); + if (m_xonly) { CScript script = CScript() << ToByteVector(XOnlyPubKey(keys[0])) << OP_CHECKSIG; return Vector(std::move(script)); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index cd438cfe2f3..6a762640f39 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -219,11 +219,12 @@ class WalletImpl : public Wallet if (is_change) return; 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); + result.emplace_back(dest, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), label, m_wallet->IsDestinationActive(dest)); }); return result; } - std::vector getAddressReceiveRequests() override { + std::vector getAddressReceiveRequests() override + { LOCK(m_wallet->cs_wallet); return m_wallet->GetAddressReceiveRequests(); } @@ -539,7 +540,7 @@ class WalletImpl : public Wallet { return MakeSignalHandler(m_wallet->NotifyAddressBookChanged.connect( [fn](const CTxDestination& address, const std::string& label, bool is_mine, - AddressPurpose purpose, ChangeType status) { fn(address, label, is_mine, purpose, status); })); + AddressPurpose purpose, ChangeType status, bool is_active) { fn(address, label, is_mine, purpose, status, is_active); })); } std::unique_ptr handleTransactionChanged(TransactionChangedFn fn) override { diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index a5be0739a9b..15035395609 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -502,6 +502,7 @@ RPCHelpMan getaddressinfo() {RPCResult::Type::STR, "address", "The bitcoin address validated."}, {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded scriptPubKey generated by the address."}, {RPCResult::Type::BOOL, "ismine", "If the address is yours."}, + {RPCResult::Type::BOOL, "isactive", "If the key is in the active keypool (always equal to \"ismine\" in descriptor wallets)."}, {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."}, {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."}, {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."}, @@ -572,6 +573,7 @@ RPCHelpMan getaddressinfo() isminetype mine = pwallet->IsMine(dest); ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE)); + ret.pushKV("isactive", pwallet->IsDestinationActive(dest)); if (provider) { auto inferred = InferDescriptor(scriptPubKey, *provider); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 62bd0c06cd0..b8b304c9e63 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -392,6 +392,23 @@ std::vector LegacyScriptPubKeyMan::MarkUnusedAddresses(const return result; } +bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script) const +{ + LOCK(cs_KeyStore); + + if (!IsMine(script)) return false; // Not in the keystore at all + + for (const auto& key_id : GetAffectedKeys(script, *this)) { + const auto it = mapKeyMetadata.find(key_id); + if (it == mapKeyMetadata.end()) return false; // This key must be really old + + if (!it->second.hd_seed_id.IsNull() && it->second.hd_seed_id == m_hd_chain.seed_id) return true; + } + + // Imported or dumped for a new keypool + return false; +} + void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { LOCK(cs_KeyStore); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 22b67c88e95..dd0465a68b3 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -198,6 +198,9 @@ class ScriptPubKeyMan */ virtual std::vector MarkUnusedAddresses(const CScript& script) { return {}; } + /* Determines if address is derived from active key manager */ + [[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; }; + /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active. * Returns false if already setup or setup fails, true if setup is successful * Set force=true to make it re-setup if already setup, used for upgrades @@ -382,6 +385,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::vector MarkUnusedAddresses(const CScript& script) override; + [[nodiscard]] bool IsKeyActive(const CScript& script) const override; + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo void UpgradeKeyMetadata(); @@ -607,6 +612,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan std::vector MarkUnusedAddresses(const CScript& script) override; + [[nodiscard]] bool IsKeyActive(const CScript& script) const override { return IsMine(script); } + bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 90042f52526..f8c8dc2be57 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -39,5 +39,159 @@ BOOST_AUTO_TEST_CASE(CanProvide) BOOST_CHECK(keyman.CanProvide(p2sh_script, data)); } +BOOST_AUTO_TEST_CASE(Legacy_IsKeyActive) +{ + CWallet wallet(m_node.chain.get(), "", CreateMockWalletDatabase()); + { + LOCK(wallet.cs_wallet); + wallet.SetMinVersion(FEATURE_LATEST); + wallet.m_keypool_size = 10; + } + LegacyScriptPubKeyMan& spkm = *wallet.GetOrCreateLegacyScriptPubKeyMan(); + + // Start off empty + BOOST_CHECK(spkm.GetScriptPubKeys().empty()); + + // Generate 20 keypool keys (10 internal, 10 external) + { + LOCK(wallet.cs_wallet); + spkm.SetupGeneration(); + } + + // 4 scripts per keypool key (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH) + // Plus 4 scripts for the seed key + auto scripts1 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts1.size(), 84); + + // All keys are active + for (const CScript& script : scripts1) { + BOOST_CHECK(spkm.IsKeyActive(script)); + } + + // Requesting single from spkm should not deactivate key + CTxDestination dest1; + { + LOCK(wallet.cs_wallet); + auto result = spkm.GetNewDestination(OutputType::BECH32); + dest1 = result.value(); + } + CScript script = GetScriptForDestination(dest1); + BOOST_CHECK(spkm.IsKeyActive(script)); + + // Key pool size did not change + auto scripts2 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts2.size(), 84); + + // Use key that is not the next key + // (i.e. address gap in wallet recovery) + { + LOCK(wallet.cs_wallet); + LOCK(spkm.cs_KeyStore); + auto keys = spkm.MarkReserveKeysAsUsed(5); + BOOST_CHECK_EQUAL(keys.size(), 4); // Because we already used one with GetNewDestination + } + + // Key pool size did not change + auto scripts3 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts3.size(), 84); + + // All keys are still active + for (const CScript& script : scripts3) { + BOOST_CHECK(spkm.IsKeyActive(script)); + } + + // When user encrypts wallet for the first time, + // all existing keys are removed from active keypool + { + LOCK(wallet.cs_wallet); + // called by EncryptWallet() + spkm.SetupGeneration(true); + } + + // 20 new keys were added + auto scripts4 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts4.size(), 84 * 2); + + // All 10 original keys are now inactive + for (const CScript& script : scripts3) { + BOOST_CHECK(!spkm.IsKeyActive(script)); + } +} + +BOOST_AUTO_TEST_CASE(Descriptor_IsKeyActive) +{ + CWallet wallet(m_node.chain.get(), "", CreateMockWalletDatabase()); + { + LOCK(wallet.cs_wallet); + wallet.LoadMinVersion(FEATURE_LATEST); + wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet.m_keypool_size = 10; + wallet.SetupDescriptorScriptPubKeyMans(); + } + DescriptorScriptPubKeyMan* spkm = dynamic_cast(wallet.GetScriptPubKeyMan(OutputType::BECH32, /*internal=*/false)); + + // Start off with 10 pre-generated keys, 1 script each + auto scripts1 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts1.size(), 10); + + // All keys are active + for (const CScript& script : scripts1) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // Requesting single key from spkm should not deactivate key + auto dest1 = spkm->GetNewDestination(OutputType::BECH32); + CScript script = GetScriptForDestination(dest1.value()); + BOOST_CHECK(spkm->IsKeyActive(script)); + + // Key pool size did not change + auto scripts2 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts2.size(), 10); + + // Use key that is not the next key + // (i.e. address gap in wallet recovery) + { + LOCK(spkm->cs_desc_man); + WalletDescriptor descriptor = spkm->GetWalletDescriptor(); + FlatSigningProvider provider; + std::vector scripts3; + descriptor.descriptor->ExpandFromCache(/*pos=*/5, descriptor.cache, scripts3, provider); + + BOOST_CHECK_EQUAL(scripts3.size(), 1); + spkm->MarkUnusedAddresses(scripts3.front()); + } + + // Key pool size increased to replace used keys + auto scripts4 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts4.size(), 16); + + // All keys are still active + for (const CScript& script : scripts4) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // When user encrypts wallet for the first time, + // all existing keys are removed from active keypool + { + LOCK(wallet.cs_wallet); + // called by EncryptWallet() + wallet.SetupDescriptorScriptPubKeyMans(); + } + + // This SPKM is not affected + for (const CScript& script : scripts4) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // ...but at the wallet level all the keys from that SPKM are deactivated + int num_script_keys_not_found = 0; + for (const CScript& script : scripts4) { + if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) { + ++num_script_keys_not_found; + } + } + BOOST_CHECK_EQUAL(num_script_keys_not_found, 16); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index da4b553394c..dc0c146da25 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -451,6 +451,13 @@ void TestLoadWallet(const std::string& name, DatabaseFormat format, std::functio WITH_LOCK(wallet->cs_wallet, f(wallet)); } +void CompareRecReqVectors(std::vector a, std::vector b) +{ + BOOST_REQUIRE(a.size() == b.size()); + for (size_t i = 0; i < a.size(); ++i) + BOOST_CHECK(a[i] == b[i]); +} + BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) { for (DatabaseFormat format : DATABASE_FORMATS) { @@ -469,9 +476,10 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(wallet->IsAddressPreviouslySpent(PKHash())); BOOST_CHECK(wallet->IsAddressPreviouslySpent(ScriptHash())); - auto requests = wallet->GetAddressReceiveRequests(); - auto erequests = {"val_rr11", "val_rr20"}; - BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); + std::vector requests = wallet->GetAddressReceiveRequests(); + // Key should be marked as not active (m_is_active = false) since it was effectively imported, not derived from active seed + std::vector erequests = {{"val_rr11", false}, {"val_rr20", false}}; + CompareRecReqVectors(requests, erequests); WalletBatch batch{wallet->GetDatabase()}; BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); BOOST_CHECK(batch.EraseAddressData(ScriptHash())); @@ -479,9 +487,9 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash())); BOOST_CHECK(!wallet->IsAddressPreviouslySpent(ScriptHash())); - auto requests = wallet->GetAddressReceiveRequests(); - auto erequests = {"val_rr11"}; - BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); + std::vector requests = wallet->GetAddressReceiveRequests(); + std::vector erequests = {{"val_rr11", false}}; + CompareRecReqVectors(requests, erequests); }); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index caf95a3f03f..09b21fa2418 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2371,6 +2371,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add bool fUpdated = false; bool is_mine; std::optional purpose; + bool is_active = IsDestinationActive(address); { LOCK(cs_wallet); std::map::iterator mi = m_address_book.find(address); @@ -2386,7 +2387,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add // In very old wallets, address purpose may not be recorded so we derive it from IsMine NotifyAddressBookChanged(address, strName, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), - (fUpdated ? CT_UPDATED : CT_NEW)); + (fUpdated ? CT_UPDATED : CT_NEW), is_active); if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose))) return false; return batch.WriteName(EncodeDestination(address), strName); @@ -2415,7 +2416,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address) m_address_book.erase(address); } - NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED); + NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED, false); batch.ErasePurpose(EncodeDestination(address)); return batch.EraseName(EncodeDestination(address)); @@ -2523,6 +2524,13 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const } } +bool CWallet::IsDestinationActive(const CTxDestination& dest) const +{ + const CScript& script{GetScriptForDestination(dest)}; + const std::set& spkms{GetActiveScriptPubKeyMans()}; + return std::any_of(spkms.cbegin(), spkms.cend(), [&script](const auto& spkm) { return spkm->IsKeyActive(script); }); +} + std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const { AssertLockHeld(cs_wallet); @@ -2816,12 +2824,13 @@ bool CWallet::IsAddressPreviouslySpent(const CTxDestination& dest) const return false; } -std::vector CWallet::GetAddressReceiveRequests() const +std::vector CWallet::GetAddressReceiveRequests() const { - std::vector values; + std::vector values; for (const auto& [dest, entry] : m_address_book) { for (const auto& [id, request] : entry.receive_requests) { - values.emplace_back(request); + bool active{IsDestinationActive(dest)}; + values.emplace_back(interfaces::ReceiveRequest(request, active)); } } return values; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5252b46cdc8..934fc39dd26 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -715,6 +715,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati using ListAddrBookFunc = std::function purpose)>; void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** + * Determines if a destination is in the active spkm (not imported and not dumped for a new keypool) + */ + [[nodiscard]] bool IsDestinationActive(const CTxDestination& dest) const; + /** * Marks all outputs in each one of the destinations dirty, so their cache is * reset and does not return outdated information. @@ -749,7 +754,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsAddressPreviouslySpent(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::vector GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -782,7 +787,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati */ boost::signals2::signal + AddressPurpose purpose, ChangeType status, bool isActive)> NotifyAddressBookChanged; /** diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index bd978511536..33812affbcd 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -4,11 +4,17 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the wallet keypool and interaction with wallet encryption/locking.""" +import os +import re import time from decimal import Decimal from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error +from test_framework.descriptors import descsum_create + +TEST_KEYPOOL_SIZE = 10 +TEST_NEW_KEYPOOL_SIZE = TEST_KEYPOOL_SIZE + 2 class KeyPoolTest(BitcoinTestFramework): def add_options(self, parser): @@ -16,20 +22,89 @@ def add_options(self, parser): def set_test_params(self): self.num_nodes = 1 + self.extra_args = [[f"-keypool={TEST_KEYPOOL_SIZE}"]] def skip_test_if_missing_module(self): self.skip_if_no_wallet() def run_test(self): nodes = self.nodes + + # Derive addresses from the wallet without removing them from keypool + addrs = [] + if not self.options.descriptors: + path = os.path.join(self.nodes[0].datadir, 'wallet.dump') + nodes[0].dumpwallet(path) + file = open(path, "r", encoding="utf8") + m = re.search(r"masterkey: (\w+)", file.read()) + file.close() + xpriv = m.group(1) + desc = descsum_create(f"wpkh({xpriv}/0h/0h/*h)") + addrs = nodes[0].deriveaddresses(descriptor=desc, range=[0, 9]) + else: + list_descriptors = nodes[0].listdescriptors() + for desc in list_descriptors["descriptors"]: + if desc['active'] and not desc["internal"] and desc["desc"][:4] == "wpkh": + addrs = nodes[0].deriveaddresses(descriptor=desc["desc"], range=[0, 9]) + + addr0 = addrs[0] + addr9 = addrs[9] # arbitrary future address index + + # Address is active before it is removed from keypool by getnewaddress + addr0_before_getting_data = nodes[0].getaddressinfo(addr0) + assert addr0_before_getting_data['isactive'] + addr_before_encrypting = nodes[0].getnewaddress() addr_before_encrypting_data = nodes[0].getaddressinfo(addr_before_encrypting) + assert addr0 == addr_before_encrypting + # Address is still active even after being removed from keypool + assert addr_before_encrypting_data['isactive'] + wallet_info_old = nodes[0].getwalletinfo() if not self.options.descriptors: assert addr_before_encrypting_data['hdseedid'] == wallet_info_old['hdseedid'] + # Address is active before wallet is encrypted (resetting keypool) + addr9_before_encrypting_data = nodes[0].getaddressinfo(addr9) + assert addr9_before_encrypting_data['isactive'] + + # Imported things are never considered active, no need to rescan + if self.options.descriptors: + nodes[0].importdescriptors([{ + "desc": "addr(bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v)", + "timestamp": "now" + }]) + else: + nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", rescan=False) + import_addr_data = nodes[0].getaddressinfo("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v") + assert not import_addr_data["ismine"] + assert import_addr_data["iswatchonly"] is not self.options.descriptors + assert not import_addr_data["isactive"] + + if self.options.descriptors: + nodes[0].importdescriptors([{ + "desc": "pk(02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e)", + "timestamp": "now" + }]) + else: + nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", rescan=False) + import_pub_data = nodes[0].getaddressinfo("bcrt1q4v7a8wn5vqd6fk4026s5gzzxyu7cfzz23n576h") + assert not import_pub_data["ismine"] + assert import_pub_data["iswatchonly"] is not self.options.descriptors + assert not import_pub_data["isactive"] + + nodes[0].importprivkey("cPMX7v5CNV1zCphFSq2hnR5rCjzAhA1GsBfD1qrJGdj4QEfu38Qx", "label", rescan=False) + import_priv_data = nodes[0].getaddressinfo("bcrt1qa985v5d53qqtrfujmzq2zrw3r40j6zz4ns02kj") + assert import_priv_data["ismine"] + assert not import_priv_data["iswatchonly"] + assert not import_priv_data["isactive"] + # Encrypt wallet and wait to terminate nodes[0].encryptwallet('test') + addr9_after_encrypting_data = nodes[0].getaddressinfo(addr9) + # Key is from unencrypted seed, no longer considered active + assert not addr9_after_encrypting_data['isactive'] + if self.options.descriptors: # Import hardened derivation only descriptors nodes[0].walletpassphrase('test', 10) @@ -75,7 +150,9 @@ def run_test(self): } ]) nodes[0].walletlock() - # Keep creating keys + # Keep creating keys until we run out + for _ in range(TEST_KEYPOOL_SIZE - 1): + nodes[0].getnewaddress() addr = nodes[0].getnewaddress() addr_data = nodes[0].getaddressinfo(addr) wallet_info = nodes[0].getwalletinfo() @@ -84,50 +161,46 @@ def run_test(self): assert addr_data['hdseedid'] == wallet_info['hdseedid'] assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # put six (plus 2) new keys in the keypool (100% external-, +100% internal-keys, 1 in min) + # put two new keys in the keypool nodes[0].walletpassphrase('test', 12000) - nodes[0].keypoolrefill(6) + nodes[0].keypoolrefill(TEST_NEW_KEYPOOL_SIZE) nodes[0].walletlock() wi = nodes[0].getwalletinfo() if self.options.descriptors: - assert_equal(wi['keypoolsize_hd_internal'], 24) - assert_equal(wi['keypoolsize'], 24) + # Descriptors wallet: keypool size applies to both internal and external + # chains and there are four of each (legacy, nested, segwit, and taproot) + assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE * 4) + assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE * 4) else: - assert_equal(wi['keypoolsize_hd_internal'], 6) - assert_equal(wi['keypoolsize'], 6) + # Legacy wallet: keypool size applies to both internal and external HD chains + assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE) + assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE) # drain the internal keys - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - addr = set() + for _ in range(TEST_NEW_KEYPOOL_SIZE): + nodes[0].getrawchangeaddress() # the next one should fail assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getrawchangeaddress) # drain the external keys - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - assert len(addr) == 6 + addr = set() + for _ in range(TEST_NEW_KEYPOOL_SIZE): + addr.add(nodes[0].getnewaddress(address_type="bech32")) # the next one should fail assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # refill keypool with three new addresses + # refill keypool nodes[0].walletpassphrase('test', 1) - nodes[0].keypoolrefill(3) + # At this point the keypool has >45 keys in it + # calling keypoolrefill with anything smaller than that is a noop + nodes[0].keypoolrefill(50) # test walletpassphrase timeout time.sleep(1.1) assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) # drain the keypool - for _ in range(3): + for _ in range(50): nodes[0].getnewaddress() assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getnewaddress)