Disallow encryption of watchonly wallets #631

pull achow101 wants to merge 1 commits into bitcoin-core:master from achow101:watchonly-disable-encryption changing 3 files +12 −0
  1. achow101 commented at 3:45 pm on July 15, 2022: member

    Watchonly wallets do not have any private keys to encrypt. It does not make sense to encrypt such wallets, so disable the option to encrypt them.

    This avoids an assertion that can be hit when encrypting watchonly descriptor wallets.

    As our current behavior allows for encrypting watchonly wallets (no crash with legacy, crash, but still encrypted with descriptors), the new NoKeys status is only returned for unencrypted watchonly wallets. This allows any watchonly wallets that were previously encrypted to show the correct encryption status (they have encryption keys, and so should be indicated as being encrypted).

  2. Disallow encryption of watchonly wallets 
    Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.

This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
    4c495413e1
  3. fanquake added the label Wallet on Jul 15, 2022
  4. w0xlt approved
  5. w0xlt commented at 5:48 pm on July 15, 2022: contributor

    tACK https://github.com/bitcoin-core/gui/pull/631/commits/4c495413e138ec1dd6874e41b44e689f0c15e0e3

    I couldn’t reproduce the crash mentioned on IRC. On my machine (Ubuntu 21.10 VM) nothing happened after encrypting a wallet with private keys disabled. But this option should be disabled for these wallets anyway.

    However, this change does not prevent the user from creating a blank wallet (without disabling the private keys) and encrypting an empty wallet.

  6. achow101 commented at 8:42 pm on July 15, 2022: member

    I couldn’t reproduce the crash mentioned on IRC. On my machine (Ubuntu 21.10 VM) nothing happened after encrypting a wallet with private keys disabled.

    The crash is for descriptor wallet specifically. Legacy wallets will “encrypt” successfully.

    However, this change does not prevent the user from creating a blank wallet (without disabling the private keys) and encrypting an empty wallet.

    Yes, blank wallets are not watchonly because they can have private keys imported.

  7. katesalazar commented at 12:35 pm on July 16, 2022: contributor

    concept ACK

    On Fri, Jul 15, 2022 at 5:45 PM Andrew Chow @.***> wrote:

    Watchonly wallets do not have any private keys to encrypt. It does not make sense to encrypt such wallets, so disable the option to encrypt them.

    This avoids an assertion that can be hit when encrypting watchonly descriptor wallets.

    As our current behavior allows for encrypting watchonly wallets (no crash with legacy, crash, but still encrypted with descriptors), the new NoKeys status is only returned for unencrypted watchonly wallets. This allows any watchonly wallets that were previously encrypted to show the correct encryption status (they have encryption keys, and so should be indicated as being encrypted).

    You can view, comment on, or merge this pull request online at:

    #631 Commit Summary

    File Changes

    (3 files https://github.com/bitcoin-core/gui/pull/631/files)

    Patch Links:

    — Reply to this email directly, view it on GitHub https://github.com/bitcoin-core/gui/pull/631, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMRS4W42QLK6KZCP3LLO2A3VUGBR3ANCNFSM53V7YK3A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

  8. kristapsk commented at 2:16 pm on July 16, 2022: contributor
    Concept ACK
  9. in src/qt/walletmodel.h:74 in 4c495413e1 
    70@@ -71,6 +71,7 @@ class WalletModel : public QObject
71 
72     enum EncryptionStatus
73     {
74+        NoKeys,       // wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)
    luke-jr commented at 7:28 pm on July 16, 2022:
    nit: Unsupported seems like it would fit better here
  10. luke-jr approved
  11. luke-jr commented at 7:41 pm on July 16, 2022: member
    tACK, 1 nit
  12. luke-jr commented at 7:47 pm on July 16, 2022: member
    Note: This could be rebased as far back as the v0.5.0 tag (or at the latest for 0.21 compatibility, 831675c8dccfa6525ffe751da3cc60709c380953) to cleanly merge to older branches.
  13. hebasto commented at 8:23 am on July 18, 2022: member

    The issue reported on IRC:

    12:01 <stevenroose> bitcoin-qt: wallet/scriptpubkeyman.cpp:1850: bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch&, const CKey&, const CPubKey&): Assertion `!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)’ failed 12:01 <stevenroose> qt crashes when trying to encrypt a watchonly wallet 12:02 <stevenroose> it prompts for the passphrase and then hangs about 5 seconds and crashes

  14. hebasto commented at 9:53 am on July 18, 2022: member

    Code changes look sane. But I cannot reproduce the initially reported issue to make sure it has been fixed.

    FWIW, here is the getwalletinfo RPC output for my test watchonly descriptor wallet which I can encrypt with no crash:

     0{
 1  "walletname": "220718d-v22-DisablePK.1",
 2  "walletversion": 169900,
 3  "format": "sqlite",
 4  "balance": 0.00100000,
 5  "unconfirmed_balance": 0.00000000,
 6  "immature_balance": 0.00000000,
 7  "txcount": 1,
 8  "keypoolsize": 1000,
 9  "keypoolsize_hd_internal": 0,
10  "paytxfee": 0.00000000,
11  "private_keys_enabled": false,
12  "avoid_reuse": false,
13  "scanning": false,
14  "descriptors": true,
15  "external_signer": false
16}

    @achow101

    The crash is for descriptor wallet specifically.

    Could you provide steps to reproduce the issue reliably?

  15. shaavan commented at 1:39 pm on July 18, 2022: contributor

    Concept ACK

    • I agree with the concept of this PR.
      • Since encrypting a wallet ⇒ encrypting its private keys. And since a watch-only wallet has none, encrypting it doesn’t make sense.
    • I was able to verify the code and confirm that encrypting a watch-only wallet in the GUI is disabled by this PR.

    Screenshot: Screenshot from 2022-07-18 19-05-59

    • However, I was also unable to reproduce the error on the master branch.
  16. achow101 commented at 4:11 pm on July 18, 2022: member

    Could you provide steps to reproduce the issue reliably?

    Create a new descriptor wallet with only private keys diasbled. It must not be marked as a blank wallet. This requires using the RPC to create the wallet as the GUI will always set the blank wallet flag. Then encrypt that wallet using the GUI and it will crash.

  17. hebasto added the label Needs backport (22.x) on Jul 19, 2022
  18. hebasto added the label Needs backport (23.x) on Jul 19, 2022
  19. hebasto approved
  20. hebasto commented at 9:00 am on July 19, 2022: member
    ACK 4c495413e138ec1dd6874e41b44e689f0c15e0e3, tested on Ubuntu 22.04.
  21. hebasto merged this on Jul 19, 2022
  22. hebasto closed this on Jul 19, 2022
  23. sidhujag referenced this in commit 8165f90317 on Jul 19, 2022
  24. hebasto referenced this in commit 31ca698f20 on Aug 12, 2022
  25. hebasto cross-referenced this on Aug 12, 2022 from issue [23.x] GUI backports by hebasto
  26. hebasto commented at 9:36 am on August 12, 2022: member
    Backported to the 23.x branch in https://github.com/bitcoin/bitcoin/pull/25828.
  27. hebasto removed the label Needs backport (23.x) on Aug 12, 2022
  28. fanquake referenced this in commit 45c9f4afa4 on Oct 2, 2022
  29. fanquake cross-referenced this on Nov 17, 2022 from issue [22.x] GUI backports by hebasto
  30. hebasto referenced this in commit 7b7bbc145a on Nov 21, 2022
  31. hebasto commented at 10:50 am on November 21, 2022: member
    Backported to the 22.x branch in https://github.com/bitcoin/bitcoin/pull/26521.
  32. hebasto removed the label Needs backport (22.x) on Nov 21, 2022
  33. fanquake cross-referenced this on Nov 21, 2022 from issue [22.x] Bump version to 22.1rc2 & add release notes by fanquake
  34. fanquake referenced this in commit c192b86c0b on Nov 22, 2022
  35. fanquake referenced this in commit 9182b2fbae on Nov 23, 2022
  36. psgreco referenced this in commit 28cf404786 on Dec 7, 2022
  37. psgreco referenced this in commit b7947ef633 on Dec 7, 2022
  38. psgreco referenced this in commit 7948a05822 on Dec 7, 2022
  39. psgreco referenced this in commit a12aceedf7 on Dec 7, 2022
  40. vertiond referenced this in commit 31057382f7 on Dec 10, 2022
  41. achow101 cross-referenced this on Oct 24, 2023 from issue qt: When using an unencrypted read-only wallet, pressing "Create Unsigned", shows "This operation needs you wallet passphrase to unlock the wallet" by stevenroose
  42. achow101 cross-referenced this on Oct 24, 2023 from issue Check for private keys disabled before attempting unlock by achow101
  43. bitcoin-core locked this on Nov 21, 2023


achow101 w0xlt katesalazar kristapsk luke-jr hebasto shaavan

Labels
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-10-23 00:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me