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

pull pinheadmz wants to merge 8 commits into bitcoin-core:master from pinheadmz:used-addr-ui-gui changing 27 files +548 −115
  1. 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

    1

    2. Encrypt wallet

    Notice how the first address is immediately flagged.

    2

    3. Get new receive address after encrypting wallet

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

    3

  2. 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:

    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.

  3. hebasto added the label Wallet on Mar 27, 2023
  4. pinheadmz force-pushed on Apr 3, 2023
  5. pinheadmz force-pushed on Apr 3, 2023
  6. pinheadmz force-pushed on Apr 4, 2023
  7. pinheadmz force-pushed on Apr 4, 2023
  8. pinheadmz force-pushed on Apr 4, 2023
  9. 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
  10. 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).
  11. 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.
  12. 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
  13. 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.
  14. 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.
  15. 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?
  16. 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)
  17. 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.
  18. 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?
  19. 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?
  20. 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)
  21. 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)
  22. 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.
  23. 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.

  24. 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.
  25. pinheadmz force-pushed on Apr 5, 2023
  26. pinheadmz force-pushed on Apr 5, 2023
  27. 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.
  28. 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

  29. pinheadmz marked this as ready for review on Apr 5, 2023
  30. pinheadmz force-pushed on Apr 8, 2023
  31. DrahtBot added the label Needs rebase on Apr 12, 2023
  32. pinheadmz force-pushed on Apr 12, 2023
  33. 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.

  34. DrahtBot removed the label Needs rebase on Apr 12, 2023
  35. pinheadmz force-pushed on Apr 14, 2023
  36. DrahtBot added the label Needs rebase on May 1, 2023
  37. 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
  38. wallet: implement IsDestinationActive() and add to rpc getaddressinfo 
    This connects SPKM.IsKeyActive() up to the wallet level.
    abad1e0613
  39. test: cover ScriptPubKeyMan::IsKeyActive() c6d256562e
  40. test: cover "isactive" field in rpc getaddressinfo 1b2e020295
  41. doc: release notes for #27216 712e366b61
  42. RandyMcMillan commented at 3:43 pm on May 1, 2023: contributor
    Concept ACK
  43. 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
  44. 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
  45. qt: update address warnings when wallet is encrypted d6519a80ea
  46. pinheadmz force-pushed on May 1, 2023
  47. 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 🙏 ❤️

  48. DrahtBot removed the label Needs rebase on May 1, 2023
  49. DrahtBot added the label Needs rebase on May 15, 2023
  50. DrahtBot commented at 11:45 am on May 15, 2023: contributor

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

  51. 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.

  52. 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?

  53. pinheadmz marked this as a draft on Sep 22, 2023
  54. 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.

  55. pinheadmz commented at 2:51 pm on October 12, 2023: contributor
    closing for now, #27216 needs better approach
  56. pinheadmz closed this on Oct 12, 2023
  57. bitcoin-core locked this on Oct 11, 2024


pinheadmz DrahtBot jonatack RandyMcMillan luke-jr hebasto pablomartin4btc

Labels
Needs rebase Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-04 08:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me