Disallow encryption of watchonly wallets

[achow101]

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

[w0xlt]

tACK 4c49541

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.

[achow101]

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.

[katesalazar]

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 - 4c49541 <4c49541> Disallow encryption of watchonly wallets File Changes (3 files <https://github.com/bitcoin-core/gui/pull/631/files>) - *M* src/qt/bitcoingui.cpp <https://github.com/bitcoin-core/gui/pull/631/files#diff-2ecf8cbf369cf3d2f3d2b1cf5cfe4c1a647d63e11e2885d2fd0ac11fb5f7a804> (6) - *M* src/qt/walletmodel.cpp <https://github.com/bitcoin-core/gui/pull/631/files#diff-3c5f55f5118243d479461d5445315d9d27772fde1f1d86d828f26d113d1ba8ac> (5) - *M* src/qt/walletmodel.h <https://github.com/bitcoin-core/gui/pull/631/files#diff-22574d424404ec3586264be2b71b9fa33c6e5a039f1209fb7693aaa2ae25f1db> (1) Patch Links: - https://github.com/bitcoin-core/gui/pull/631.patch - https://github.com/bitcoin-core/gui/pull/631.diff — Reply to this email directly, view it on GitHub <#631>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMRS4W42QLK6KZCP3LLO2A3VUGBR3ANCNFSM53V7YK3A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[kristapsk]

Concept ACK

[luke-jr]

tACK, 1 nit

[luke-jr]

nit: Unsupported seems like it would fit better here

[luke-jr]

Note: This could be rebased as far back as the v0.5.0 tag (or at the latest for 0.21 compatibility, 831675c) to cleanly merge to older branches.

[hebasto]

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

[hebasto]

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:

{
  "walletname": "220718d-v22-DisablePK.1",
  "walletversion": 169900,
  "format": "sqlite",
  "balance": 0.00100000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 1,
  "keypoolsize": 1000,
  "keypoolsize_hd_internal": 0,
  "paytxfee": 0.00000000,
  "private_keys_enabled": false,
  "avoid_reuse": false,
  "scanning": false,
  "descriptors": true,
  "external_signer": false
}

@achow101

The crash is for descriptor wallet specifically.

Could you provide steps to reproduce the issue reliably?

[shaavan]

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:

• However, I was also unable to reproduce the error on the master branch.

[achow101]

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.

[hebasto]

ACK 4c49541, tested on Ubuntu 22.04.

[hebasto]

Backported to the 23.x branch in bitcoin/bitcoin#25828.

[hebasto]

Backported to the 22.x branch in bitcoin/bitcoin#26521.