Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions src/qt/addressbookpage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
class AddressBookSortFilterProxyModel final : public QSortFilterProxyModel
{
const QString m_type;
const AddressBookPage::AddressFilter m_address_filter;

public:
AddressBookSortFilterProxyModel(const QString& type, QObject* parent)
AddressBookSortFilterProxyModel(const QString& type, AddressBookPage::AddressFilter address_filter, QObject* parent)
: QSortFilterProxyModel(parent)
, m_type(type)
, m_address_filter(address_filter)
{
setDynamicSortFilter(true);
setFilterCaseSensitivity(Qt::CaseInsensitive);
Expand All @@ -42,18 +44,23 @@ class AddressBookSortFilterProxyModel final : public QSortFilterProxyModel
}

auto address = model->index(row, AddressTableModel::Address, parent);
if (m_address_filter == AddressBookPage::AddressFilter::Signable &&
!model->data(address, AddressTableModel::CanSignMessageRole).toBool()) {
return false;
}

const auto pattern = filterRegularExpression();
return (model->data(address).toString().contains(pattern) ||
model->data(label).toString().contains(pattern));
}
};

AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode, Tabs _tab, QWidget *parent) :
AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode, Tabs _tab, QWidget *parent, AddressFilter _address_filter) :
QDialog(parent, GUIUtil::dialog_flags),
ui(new Ui::AddressBookPage),
mode(_mode),
tab(_tab)
tab(_tab),
address_filter(_address_filter)
{
ui->setupUi(this);

Expand All @@ -73,7 +80,11 @@ AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode,
switch(tab)
{
case SendingTab: setWindowTitle(tr("Choose the address to send coins to")); break;
case ReceivingTab: setWindowTitle(tr("Choose the address to receive coins with")); break;
case ReceivingTab:
setWindowTitle(address_filter == AddressFilter::Signable ?
tr("Choose the address to sign with") :
tr("Choose the address to receive coins with"));
break;
}
connect(ui->tableView, &QTableView::doubleClicked, this, &QDialog::accept);
ui->tableView->setEditTriggers(QAbstractItemView::NoEditTriggers);
Expand All @@ -89,7 +100,9 @@ AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode,
ui->newAddress->setVisible(true);
break;
case ReceivingTab:
ui->labelExplanation->setText(tr("These are your Bitcoin addresses for receiving payments. Use the 'Create new receiving address' button in the receive tab to create new addresses.\nSigning is only possible with addresses of the type 'legacy'."));
ui->labelExplanation->setText(address_filter == AddressFilter::Signable ?
tr("These are your Bitcoin addresses that can be used for message signing.") :
tr("These are your Bitcoin addresses for receiving payments. Use the 'Create new receiving address' button in the receive tab to create new addresses."));
ui->deleteAddress->setVisible(false);
ui->newAddress->setVisible(false);
break;
Expand Down Expand Up @@ -123,7 +136,7 @@ void AddressBookPage::setModel(AddressTableModel *_model)
return;

auto type = tab == ReceivingTab ? AddressTableModel::Receive : AddressTableModel::Send;
proxyModel = new AddressBookSortFilterProxyModel(type, this);
proxyModel = new AddressBookSortFilterProxyModel(type, address_filter, this);
proxyModel->setSourceModel(_model);

connect(ui->searchLineEdit, &QLineEdit::textChanged, proxyModel, &QSortFilterProxyModel::setFilterWildcard);
Expand Down
8 changes: 7 additions & 1 deletion src/qt/addressbookpage.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ class AddressBookPage : public QDialog
ForEditing /**< Open address book for editing */
};

explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = nullptr);
enum class AddressFilter {
ShowAll,
Signable, /**< Show only addresses that can sign messages */
};

explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = nullptr, AddressFilter address_filter = AddressFilter::ShowAll);
~AddressBookPage();

void setModel(AddressTableModel *model);
Expand All @@ -52,6 +57,7 @@ public Q_SLOTS:
AddressTableModel* model{nullptr};
Mode mode;
Tabs tab;
AddressFilter address_filter;
QString returnValue;
AddressBookSortFilterProxyModel *proxyModel;
QMenu *contextMenu;
Expand Down
5 changes: 5 additions & 0 deletions src/qt/addresstablemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ 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 == CanSignMessageRole) {
const auto destination{DecodeDestination(rec->address.toStdString())};
return std::holds_alternative<PKHash>(destination) &&
!walletModel->wallet().privateKeysDisabled() &&
walletModel->wallet().isSpendable(destination);
}
return QVariant();
}
Expand Down
3 changes: 2 additions & 1 deletion src/qt/addresstablemodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class AddressTableModel : public QAbstractTableModel
};

enum RoleIndex {
TypeRole = Qt::UserRole /**< Type of address (#Send or #Receive) */
TypeRole = Qt::UserRole, /**< Type of address (#Send or #Receive) */
CanSignMessageRole /**< Whether the address can be used for message signing */
};

/** Return status of edit/insert operation */
Expand Down
3 changes: 1 addition & 2 deletions src/qt/signverifymessagedialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ void SignVerifyMessageDialog::on_addressBookButton_SM_clicked()
{
if (model && model->getAddressTableModel())
{
model->refresh(/*pk_hash_only=*/true);
AddressBookPage dlg(platformStyle, AddressBookPage::ForSelection, AddressBookPage::ReceivingTab, this);
AddressBookPage dlg(platformStyle, AddressBookPage::ForSelection, AddressBookPage::ReceivingTab, this, AddressBookPage::AddressFilter::Signable);
dlg.setModel(model->getAddressTableModel());
if (dlg.exec())
{
Expand Down
147 changes: 147 additions & 0 deletions src/qt/test/addressbooktests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,18 @@
#include <interfaces/chain.h>
#include <interfaces/node.h>
#include <qt/addressbookpage.h>
#include <qt/addresstablemodel.h>
#include <qt/clientmodel.h>
#include <qt/editaddressdialog.h>
#include <qt/signverifymessagedialog.h>
#include <qt/optionsmodel.h>
#include <qt/platformstyle.h>
#include <qt/qvalidatedlineedit.h>
#include <qt/walletmodel.h>

#include <key.h>
#include <key_io.h>
#include <script/descriptor.h>
#include <wallet/wallet.h>
#include <wallet/test/util.h>
#include <walletinitinterface.h>
Expand All @@ -36,6 +39,7 @@ using wallet::CreateMockableWalletDatabase;
using wallet::RemoveWallet;
using wallet::WALLET_FLAG_DESCRIPTORS;
using wallet::WalletContext;
using wallet::WalletDescriptor;

namespace
{
Expand Down Expand Up @@ -208,6 +212,147 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
QCOMPARE(table_view->model()->rowCount(), 3);
}

/**
* Test that CanSignMessageRole correctly filters the sign-message address picker:
* - watch-only PKHash addresses (no private key) → not signable
* - bech32 addresses (not PKHash) → not signable
* - spendable PKHash addresses (private key imported) → signable
* Also verifies AddressBookPage::AddressFilter::Signable shows only signable addresses.
*/
void TestSignableAddressFilter(interfaces::Node& node)
{
TestChain100Setup test;
auto wallet_loader = interfaces::MakeWalletLoader(*test.m_node.chain, *Assert(test.m_node.args));
test.m_node.wallet_loader = wallet_loader.get();
node.setContext(&test.m_node);

const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(
node.context()->chain.get(), "", CreateMockableWalletDatabase());

{
LOCK(wallet->cs_wallet);
wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
wallet->SetupDescriptorScriptPubKeyMans();

// (a) Watch-only PKHash: address in book but no private key → not signable
const PKHash watchonly_dest{GenerateRandomKey().GetPubKey()};
wallet->SetAddressBook(watchonly_dest, "watchonly", wallet::AddressPurpose::RECEIVE);

// (b) Bech32 (P2WPKH): not a PKHash destination → not signable
const WitnessV0KeyHash bech32_dest{GenerateRandomKey().GetPubKey()};
wallet->SetAddressBook(bech32_dest, "bech32", wallet::AddressPurpose::RECEIVE);

// (c) Spendable PKHash: private key imported via pkh() descriptor → signable
CKey legacy_key;
legacy_key.MakeNewKey(true);
FlatSigningProvider legacy_provider;
std::string legacy_err;
auto legacy_descs = Parse("pkh(" + EncodeSecret(legacy_key) + ")", legacy_provider, legacy_err, /*require_checksum=*/false);
assert(!legacy_descs.empty());
WalletDescriptor legacy_w_desc(std::move(legacy_descs[0]), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/1);
QVERIFY(wallet->AddWalletDescriptor(legacy_w_desc, legacy_provider, "", /*internal=*/false));
const PKHash legacy_dest{legacy_key.GetPubKey()};
wallet->SetAddressBook(legacy_dest, "legacy", wallet::AddressPurpose::RECEIVE);
}

std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
OptionsModel optionsModel(node);
bilingual_str error;
QVERIFY(optionsModel.Init(error));
ClientModel clientModel(node, &optionsModel);
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
AddressTableModel* addrModel = walletModel.getAddressTableModel();
QVERIFY(addrModel);

// Only the spendable PKHash should report CanSignMessageRole=true
int signable_count = 0;
for (int i = 0; i < addrModel->rowCount({}); ++i) {
if (addrModel->data(addrModel->index(i, AddressTableModel::Address, {}),
AddressTableModel::CanSignMessageRole).toBool()) {
++signable_count;
}
}
QCOMPARE(signable_count, 1);

// AddressFilter::Signable must show only the spendable PKHash; no filter shows all
AddressBookPage page_all{platformStyle.get(), AddressBookPage::ForSelection, AddressBookPage::ReceivingTab};
page_all.setModel(addrModel);
AddressBookPage page_signable{platformStyle.get(), AddressBookPage::ForSelection, AddressBookPage::ReceivingTab,
nullptr, AddressBookPage::AddressFilter::Signable};
page_signable.setModel(addrModel);

auto* table_all = page_all.findChild<QTableView*>("tableView");
auto* table_signable = page_signable.findChild<QTableView*>("tableView");
QVERIFY(table_all != nullptr);
QVERIFY(table_signable != nullptr);
QCOMPARE(table_signable->model()->rowCount(), 1);
QVERIFY(table_all->model()->rowCount() >= 3);
}

/**
* Regression test for the stale shared model bug: clicking the address book
* button in SignVerifyMessageDialog previously called
* WalletModel::refresh(pk_hash_only=true), which replaced the shared
* addressTableModel pointer with a new filtered instance. Any other view
* (e.g. the Receiving Addresses window) holding that pointer was left stale
* and would not reflect addresses added after the picker was opened.
*
* Directly invokes on_addressBookButton_SM_clicked() — the exact trigger of
* the bug — and verifies the shared addressTableModel pointer is unchanged.
*/
void TestAddressTableModelStability(interfaces::Node& node)
{
TestChain100Setup test;
auto wallet_loader = interfaces::MakeWalletLoader(*test.m_node.chain, *Assert(test.m_node.args));
test.m_node.wallet_loader = wallet_loader.get();
node.setContext(&test.m_node);

const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(
node.context()->chain.get(), "", CreateMockableWalletDatabase());
{
LOCK(wallet->cs_wallet);
wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
wallet->SetupDescriptorScriptPubKeyMans();
wallet->SetAddressBook(PKHash{GenerateRandomKey().GetPubKey()}, "addr1", wallet::AddressPurpose::RECEIVE);
}

std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
OptionsModel optionsModel(node);
bilingual_str error;
QVERIFY(optionsModel.Init(error));
ClientModel clientModel(node, &optionsModel);
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);

// Store the shared model pointer — this is what Receiving Addresses holds
AddressTableModel* initial_model = walletModel.getAddressTableModel();
QVERIFY(initial_model != nullptr);
QCOMPARE(initial_model->rowCount({}), 1);

SignVerifyMessageDialog svd(platformStyle.get(), nullptr);
svd.setModel(&walletModel);

// Close the address book picker as soon as it opens — it uses exec() so
// the timer fires inside that modal event loop
QTimer::singleShot(0, []() {
for (QWidget* widget : QApplication::topLevelWidgets()) {
if (auto* page = qobject_cast<AddressBookPage*>(widget)) page->reject();
}
});

// Invoke the exact slot that previously called model->refresh(pk_hash_only=true)
QMetaObject::invokeMethod(&svd, "on_addressBookButton_SM_clicked");

// The shared model pointer must be unchanged — Receiving Addresses is not stale
QCOMPARE(walletModel.getAddressTableModel(), initial_model);
QCOMPARE(initial_model->rowCount({}), 1);
}

} // namespace

void AddressBookTests::addressBookTests()
Expand All @@ -224,4 +369,6 @@ void AddressBookTests::addressBookTests()
}
#endif
TestAddAddressesToSendBook(m_node);
TestAddressTableModelStability(m_node);
TestSignableAddressFilter(m_node);
}
5 changes: 0 additions & 5 deletions src/qt/walletmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -583,11 +583,6 @@ bool WalletModel::isMultiwallet() const
return m_node.walletLoader().getWallets().size() > 1;
}

void WalletModel::refresh(bool pk_hash_only)
Comment thread
Bushstar marked this conversation as resolved.
{
addressTableModel = new AddressTableModel(this, pk_hash_only);
}

uint256 WalletModel::getLastBlockProcessed() const
{
return m_client_model ? m_client_model->getBestBlockHash() : uint256{};
Expand Down
2 changes: 0 additions & 2 deletions src/qt/walletmodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,6 @@ class WalletModel : public QObject

bool isMultiwallet() const;

void refresh(bool pk_hash_only = false);

uint256 getLastBlockProcessed() const;

// Retrieve the cached wallet balance
Expand Down
Loading