pinheadmz commented at 6:47 pm on March 24, 2023: contributor

Closes https://github.com/bitcoin/bitcoin/issues/3314 Built off of https://github.com/bitcoin/bitcoin/pull/27216 (== first two commits of this PR, required core updates)

This PR adds a “warnings” column in the receive coins dialog (recent requests table) and the address book receive page. Hovering over the ⚠️ if present shows a tooltip with a list of warnings. We also add a “warnings” field in the receive request dialog (shown after double-clicking an address in the first table) with the same message.

The warnings shown can be anything implemented in new methods GetWarnings() but for now the only message we show there is:

This address should not be used. It was derived from an inactive seed, was imported, or may have been stored unencrypted.

Example screenshots:

1. Create new unencrypted wallet, get new receive address

2. Encrypt wallet

Notice how the first address is immediately flagged.

3. Get new receive address after encrypting wallet

New address is not flagged, it is active and safe to use.

DrahtBot commented at 6:47 pm on March 24, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	RandyMcMillan, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#684 (Improve ‘Requested Payments History’ Multiselect by john-moffett)
#bitcoin/bitcoin/26836 (wallet: finish addressbook encapsulation and simplify addressbook migration by furszy)
#bitcoin/bitcoin/26715 (Introduce MockableDatabase for wallet unit tests by achow101)
#bitcoin/bitcoin/20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#bitcoin/bitcoin/19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#bitcoin/bitcoin/19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#bitcoin/bitcoin/10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto added the label Wallet on Mar 27, 2023

pinheadmz force-pushed on Apr 3, 2023

pinheadmz force-pushed on Apr 4, 2023

pinheadmz renamed this:
~~Flag addresses as "active" (or not)~~
Add warnings for non-active addresses in receive tab and address book
on Apr 4, 2023

in src/qt/addresstablemodel.cpp:251 in e4e7466c86 outdated

243@@ -230,6 +244,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
244             return {};
245         } // no default case, so the compiler can warn about missing cases
246         assert(false);
247+    } else if (role == Qt::DecorationRole) {
248+        if (index.column() == Warnings) {
249+            if (rec->GetWarnings().isEmpty())
250+                return {};
251+            else

jonatack commented at 8:09 pm on April 4, 2023:

Missing brackets here (we require brackets with conditionals over one line due to CVE-2014-1266, the Apple “goto fail” vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).

in src/qt/addresstablemodel.cpp:256 in e4e7466c86 outdated

251+            else
252+                return platformStyle->TextColorIcon(QIcon(":/icons/warning"));
253+        }
254+    } else if (role == Qt::ToolTipRole) {
255+        if (index.column() == Warnings)
256+            return rec->GetWarnings();

jonatack commented at 8:10 pm on April 4, 2023:

Brackets here as well. Perhaps can combine the 2 conditionals into one.

in src/qt/addresstablemodel.cpp:365 in e4e7466c86 outdated

362+void AddressTableModel::updateEntry(const QString& address,
363+                                    const QString& label,
364+                                    bool isMine,
365+                                    const QString& purpose,
366+                                    int status,
367+                                    bool isActive)

jonatack commented at 8:11 pm on April 4, 2023:

nit, I prefer to read left-to-right rather than up-to-down, when these can all be on one line

in src/qt/addresstablemodel.h:36 in e4e7466c86 outdated

34     enum ColumnIndex {
35-        Label = 0,   /**< User specified label */
36-        Address = 1  /**< Bitcoin address */
37+        Warnings = 0, /**< Warn user about using this address with tooltip explanation */
38+        Label = 1,    /**< User specified label */
39+        Address = 2   /**< Bitcoin address */

jonatack commented at 8:12 pm on April 4, 2023:

Could set only the first enum value for smaller diffs/less churn in future changes.

in src/qt/receivecoinsdialog.cpp:169 in e4e7466c86 outdated

164+        /* Store request for later reference */
165+        std::optional<RecentRequestEntry> entry = model->getRecentRequestsTableModel()->addNewRequest(info);
166+
167+        // Wallet failed to set address receive request
168+        if (!entry.has_value())
169+            break;

jonatack commented at 8:13 pm on April 4, 2023:

(As above) same line or brackets here.

in src/qt/recentrequeststablemodel.cpp:29 in e4e7466c86 outdated

26-    QAbstractTableModel(parent), walletModel(parent)
27+QString RecentRequestEntry::GetWarnings() const
28+{
29+    QString warnings;
30+    if (!m_is_active)
31+        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");

jonatack commented at 8:14 pm on April 4, 2023:

(Idem brackets.)

nit, looks like this warning is the same one as above at https://github.com/bitcoin-core/gui/pull/723/files#diff-74dd156c97c401ff337c0ed5399192d86ea2120f7baed1368d2a407a74e347cfR74. Maybe define it in one place if it makes sense to do so.

pinheadmz commented at 5:28 pm on April 5, 2023:

Yeah the warning is the same, but in the GUI we have these related-but-different classes RecentRequestEntry for one view and AddressTableEntry for another. I don’t see a great common file in qt/ to declare warning strings, so I’m open to suggestions or maybe I should just start a new file?

in src/qt/recentrequeststablemodel.cpp:116 in e4e7466c86 outdated

106@@ -95,6 +107,16 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons
107     {
108         if (index.column() == Amount)
109             return (int)(Qt::AlignRight|Qt::AlignVCenter);
110+    } else if (role == Qt::DecorationRole) {
111+        if (index.column() == Warnings) {
112+            if (rec->GetWarnings().isEmpty())
113+                return {};
114+            else
115+                return platformStyle->TextColorIcon(QIcon(":/icons/warning"));

jonatack commented at 8:16 pm on April 4, 2023:

(Idem brackets here and lines 118-119)

in src/qt/recentrequeststablemodel.h:71 in e4e7466c86 outdated

71-        Amount = 3,
72+        Warnings = 0,
73+        Date = 1,
74+        Label = 2,
75+        Message = 3,
76+        Amount = 4,

jonatack commented at 8:17 pm on April 4, 2023:

Could set only the first enum value for smaller diffs/less churn in future changes.

in src/qt/walletmodel.cpp:145 in e4e7466c86 outdated

142+void WalletModel::updateAddressBook(const QString& address,
143+                                    const QString& label,
144+                                    bool isMine,
145+                                    const QString& purpose,
146+                                    int status,
147+                                    bool isActive)

jonatack commented at 8:19 pm on April 4, 2023:

Why not just on one line?

in src/qt/walletmodel.cpp:388 in e4e7466c86 outdated

386+                                     const CTxDestination& address,
387+                                     const std::string& label,
388+                                     bool isMine,
389+                                     const std::string& purpose,
390+                                     ChangeType status,
391+                                     bool isActive)

jonatack commented at 8:19 pm on April 4, 2023:

Why not just on one line?

in src/wallet/scriptpubkeyman.cpp:401 in e4e7466c86 outdated

396+{
397+    LOCK(cs_KeyStore);
398+
399+    // Not in the keystore at all
400+    if (!IsMine(script))
401+        return false;

jonatack commented at 8:20 pm on April 4, 2023:

(Same line or brackets)

in src/wallet/wallet.cpp:2566 in e4e7466c86 outdated

2557@@ -2556,6 +2558,17 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const
2558     }
2559 }
2560 
2561+bool CWallet::IsDestinationActive(const CTxDestination& dest) const
2562+{
2563+    CScript script = GetScriptForDestination(dest);
2564+    for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
2565+        if (spk_man->IsKeyActive(script))
2566+            return true;

jonatack commented at 8:22 pm on April 4, 2023:

(Same line or brackets)

in src/interfaces/wallet.h:121 in e4e7466c86 outdated

116@@ -117,8 +117,8 @@ class Wallet
117     //! Get wallet address list.
118     virtual std::vector<WalletAddress> getAddresses() const = 0;
119 
120-    //! Get receive requests.
121-    virtual std::vector<std::string> getAddressReceiveRequests() = 0;
122+    //! Get receive requests. Bool indicating key is active, string containing serialized destination data
123+    virtual std::vector<std::pair<bool, std::string>> getAddressReceiveRequests() = 0;

jonatack commented at 8:26 pm on April 4, 2023:

I have a personal preference for using a struct rather than a pair in these cases to make the code clearer to read and understand, as it provides named fields like object.member_name instead of just pair.second.

in src/qt/recentrequeststablemodel.cpp:42 in e4e7466c86 outdated

41         addNewRequest(request);
42     }
43 
44     /* These columns must match the indices in the ColumnIndex enumeration */
45-    columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();
46+    columns << tr("Warnings") << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();

jonatack commented at 8:28 pm on April 4, 2023:

In general, I think I’d start with the date column first, and then the warnings one.

jonatack commented at 8:42 pm on April 4, 2023:

In general, I think I’d start with the date column first, and then the warnings one.

I see that Jonas Schnelli used a date-first order as well in https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-81567665.

jonatack commented at 8:38 pm on April 4, 2023: member

Quick GitHub review, will swing back and build/test.

A couple thoughts:

GetWarnings is already the name of a method in src/warnings; perhaps use a slightly different name.
Watch out for missing brackets in conditionals as mentioned in the developer notes; we require brackets with conditionals over one line due to CVE-2014-1266 (the Apple “goto fail” vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).
Consider notating pure/getter-like or need-to-check-returned-value methods with [[nodiscard]]. As written recently here in the repo by martinus, a [[nodiscard]] attribute is most useful in two cases:
- if a function has no effects beyond returning a certain result, i.e. is pure. If the result is not used, the call is certainly useless.
- if the return value must be checked, e.g. for a C-like interface that returns error codes instead of throwing.

pinheadmz force-pushed on Apr 5, 2023

pinheadmz commented at 5:36 pm on April 5, 2023: contributor

Thanks for all the style notes @jonatack I addressed all your comments including adding a new struct for ReceiveRequest and renaming/qualifying the new method to [[nodiscard]] GetAddressWarnings(). Also addresed your nit in https://github.com/bitcoin/bitcoin/pull/27216 and rebased both PRs on master.

pinheadmz commented at 5:44 pm on April 5, 2023: contributor

I also moved the warnings column to the right, after date. I think we could just remove the title “warnings” and that column would narrow down to 1 character …?

Screen Shot 2023-04-05 at 1 43 25 PM

pinheadmz marked this as ready for review on Apr 5, 2023

pinheadmz force-pushed on Apr 8, 2023

DrahtBot added the label Needs rebase on Apr 12, 2023

pinheadmz force-pushed on Apr 12, 2023

pinheadmz commented at 5:42 pm on April 12, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Thanks D-bot! rebased.

DrahtBot removed the label Needs rebase on Apr 12, 2023

pinheadmz force-pushed on Apr 14, 2023

DrahtBot added the label Needs rebase on May 1, 2023

wallet: implement IsKeyActive() in scriptpubkeyman

This new method returns true if the given CScript key is derived
from the SPKM. For Legacy that means checking the hd_seed_id in the
key's metadata.

Also patches PKDescriptor to return associated public keys in
MakeScripts()

ea1f304a9a

wallet: implement IsDestinationActive() and add to rpc getaddressinfo

This connects SPKM.IsKeyActive() up to the wallet level.

abad1e0613

test: cover ScriptPubKeyMan::IsKeyActive() c6d256562e

test: cover "isactive" field in rpc getaddressinfo 1b2e020295

doc: release notes for #27216 712e366b61

RandyMcMillan commented at 3:43 pm on May 1, 2023: contributor

Concept ACK

qt: add warnings to receive request table and dialog

Tool tip and dialog field can be expanded in the future.
For now the first warning we show is if the address was
derived from a non-active seed or descriptor.
See https://github.com/bitcoin/bitcoin/issues/3314

979f8c2294

qt: add warning labels in address book receive tab

Tool tip can be expanded in the future.
For now the first warning we show is if the address was
derived from a non-active seed or descriptor.
See https://github.com/bitcoin/bitcoin/issues/3314

fc6cc2aa07

qt: update address warnings when wallet is encrypted d6519a80ea

pinheadmz force-pushed on May 1, 2023

pinheadmz commented at 5:28 pm on May 1, 2023: contributor

Concept ACK

Thanks Randy! I just rebased on master to fix merge conflict. If you’d like to see this move forward, you can help my reviewing the parent Bitcoin Core PR: https://github.com/bitcoin/bitcoin/pull/27216 🙏 ❤️

DrahtBot removed the label Needs rebase on May 1, 2023

DrahtBot added the label Needs rebase on May 15, 2023

DrahtBot commented at 11:45 am on May 15, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

luke-jr commented at 9:47 pm on July 27, 2023: member

Concept NACK, there’s no reason such an address shouldn’t be used.

Closes https://github.com/bitcoin/bitcoin/issues/3314

Except those categories are entirely unrelated to whether it’s “active” or not.

hebasto commented at 6:19 pm on September 22, 2023: member

Built off of bitcoin/bitcoin#27216 (== first two commits of this PR, required core updates)

While the base PR is still under reviewing, maybe convert this one to a draft?

pinheadmz marked this as a draft on Sep 22, 2023

pablomartin4btc commented at 7:57 pm on October 7, 2023: contributor

Concept ACK

I’d prefer your suggestion of removing the title of the column or even removing the column completely and put the warning icon next to the date-time in the Date column as proposed by @jonasschnelli.

pinheadmz commented at 2:51 pm on October 12, 2023: contributor

closing for now, #27216 needs better approach

pinheadmz closed this on Oct 12, 2023

bitcoin-core locked this on Oct 11, 2024

Add warnings for non-active addresses in receive tab and address book #723

1. Create new unencrypted wallet, get new receive address

2. Encrypt wallet

3. Get new receive address after encrypting wallet

Reviews

Conflicts