qt: Fix sign message address book filtering #945

Bushstar commented at 2:16 PM on July 9, 2026: contributor

Two bugs here found when signing some pretty ancient addresses! Fixed in one PR as they share the same root cause and code path.

This PR fixes sign message address selection so it only shows signable legacy addresses and no longer mutates the wallet’s shared address book model.

The problem was that the sign message picker used WalletModel::refresh(bool pk_hash_only) as a filter, it was was too loose a filter and it replaced the wallet's shared address book model leaving other views stale.

The fix is one change, stop mutating the shared wallet model and give the signing picker its own local filter.

This is related it seems and could be closed: #62

Reproduce address picker showing invalid addresses

In console: createwallet "watchonly" true true
Select watchonly wallet in console
In console: importdescriptors '[{"desc":"addr(mipcBbFg9gMiCh81Kj8tqqdgoZub1ZJRfn)#eccnnjdy","timestamp":"now","label":"keyless legacy"}]'
In main window select the watchonly wallet
Open File > Sign message....
Click the address book button next to the signing address field.
Actual current behavior: keyless legacy appears as a selectable signing address.
Choose it, enter a message, click Sign Message.

Reproduce Receiving Addresses window stops updating

In console: createwallet "non-updating"
In the main window select "non-updating" wallet
Open File > Sign message..., click the signing address-book button, then close the picker.
In console: getnewaddress "after picker" bech32
Open Window > Receiving addresses, the new address is not shown

DrahtBot commented at 2:17 PM on July 9, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Stale ACK	pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in src/qt/walletmodel.cpp:586 in 1238afb8e5 outdated

 582 | @@ -583,11 +583,6 @@ bool WalletModel::isMultiwallet() const
 583 |      return m_node.walletLoader().getWallets().size() > 1;
 584 |  }
 585 |  
 586 | -void WalletModel::refresh(bool pk_hash_only)

pablomartin4btc commented at 2:19 PM on July 11, 2026:

nit (only if you have to retouch and you agree ofc): I'd create another commit for the removal of WalletModel::refresh() as a separate cleanup commit, so a reviewer clearly sees: here's the new mechanism, here's the removal of the old broken one.

in src/qt/addressbookpage.h:41 in 1238afb8e5

  37 | @@ -38,7 +38,12 @@ class AddressBookPage : public QDialog
  38 |          ForEditing  /**< Open address book for editing */
  39 |      };
  40 |  
  41 | -    explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = nullptr);
  42 | +    enum class Filter {

pablomartin4btc commented at 2:58 PM on July 11, 2026:

nit: I think Filter is too generic — even it sits alongside Mode and Tabs in the same class; could be AddressFilter or just be more descriptive (other options to consider RowFilter or EntryFilter). Since it's enum class it's scoped (externally AddressBookPage::Filter) which helps, but within the class itself Filter somehow clashes conceptually with QSortFilterProxyModel filtering already happening.

in src/qt/addressbookpage.h:43 in 1238afb8e5

  37 | @@ -38,7 +38,12 @@ class AddressBookPage : public QDialog
  38 |          ForEditing  /**< Open address book for editing */
  39 |      };
  40 |  
  41 | -    explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = nullptr);
  42 | +    enum class Filter {
  43 | +        None,
  44 | +        Signable,

pablomartin4btc commented at 3:00 PM on July 11, 2026:

nit: Mode already uses that doxygen /**< style inline on each value...

        Signable, /**< Show only addresses that can sign messages */

in src/qt/addressbookpage.h:42 in 1238afb8e5

  37 | @@ -38,7 +38,12 @@ class AddressBookPage : public QDialog
  38 |          ForEditing  /**< Open address book for editing */
  39 |      };
  40 |  
  41 | -    explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = nullptr);
  42 | +    enum class Filter {
  43 | +        None,

pablomartin4btc commented at 3:12 PM on July 11, 2026:

nit: minor, but None as the default (meaning "no filter applied, show all") perhaps is slightly counterintuitive. ShowAll would read more naturally as the default I think (or NoFilter).

        ShowAll,

pablomartin4btc commented at 3:29 PM on July 11, 2026: contributor

tACK 1238afb8e5190d6a9929e10b1a64aa1f28d6bcc9

Reviewed both bugs carefully, reproduced both on master, confirmed fixed on this branch.

<details> <summary>No Qt tests existed for this flow. Worked on two regression tests in <code>addressbooktests.cpp</code> on top if this PR if you want to include them...</summary> <br>

TestAddressTableModelStability: spins up SignVerifyMessageDialog, invokes on_addressBookButton_SM_clicked() via QMetaObject::invokeMethod, closes the modal via QTimer::singleShot, verifies the shared addressTableModel pointer is unchanged after the interaction; (d318af73a9ce35a4a59d1a4023343ebd998b16f1)
TestSignableAddressFilter: sets up three addresses (watch-only PKHash, bech32, spendable PKHash via pkh() descriptor); verifies only the spendable PKHash reports CanSignMessageRole=true and that Filter::Signable shows exactly one row. (c671e8782e7e64ca798bab4710d87ae13769c1fe)

Both are skipped on macOS, but run successfully on Linux.

</details>

Left a couple of suggestions (none blocker).

qt: Filter sign message address book locally 2370288219

qt: Remove unused wallet model refresh 3ef6593eab

qt: Add regression test for stale AddressTableModel pointer

Test that WalletModel::getAddressTableModel() returns the same pointer
after SignVerifyMessageDialog's address book button is clicked. Previously
WalletModel::refresh(pk_hash_only=true) was called at that point,
replacing the shared pointer and leaving other views (e.g. Receiving
Addresses) with stale data.

2187f2b1da

Bushstar force-pushed on Jul 12, 2026

DrahtBot added the label CI failed on Jul 12, 2026

qt: Add regression test for signable address filter

Test that CanSignMessageRole correctly identifies only spendable PKHash addresses as signable — excluding watch-only PKHash addresses (no private key) and bech32 addresses (not PKHash). Also verifies that AddressBookPage::AddressFilter::Signable shows exactly those addresses.

Previously the filter used pk_hash_only which only checked address type, allowing watch-only addresses through.

891068acf3

Bushstar force-pushed on Jul 12, 2026

Bushstar commented at 11:46 AM on July 12, 2026: contributor

@pablomartin4btc thanks for your review and tests. I've updated the PR as per your suggestions and updated the filter var as well to be consistent with the renamed AddressFilter enum for better clarity. Few minor updates to the tests for the rename changes and to satisfy the LLM linter.

DrahtBot removed the label CI failed on Jul 12, 2026