Check for private keys disabled before attempting unlock #773

pull achow101 wants to merge 1 commits into bitcoin-core:master from achow101:gui-skip-encryption-check-for-watchonly changing 1 files +7 −0
  1. achow101 commented at 9:25 pm on October 24, 2023: member

    Before trying to unlock a wallet, first check if it has private keys disabled. If so, there is no need to unlock.

    Note that such wallets are not expected to occur in typical usage. However bugs in previous versions allowed such wallets to be created, and so we need to handle them.

    Fixes #772

    For some additional context, see #631

  2. gui: Check for private keys disabled before attempting unlock
    Before trying to unlock a wallet, first check if it has private keys
    disabled. If so, there is no need to unlock.
    
    Note that such wallets are not expected to occur in typical usage.
    However bugs in previous versions allowed such wallets to be created,
    and so we need to handle them.
    517c7f9cba
  3. DrahtBot commented at 9:25 pm on October 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
    ACK hebasto, BrandonOdiwuor

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

  4. DrahtBot added the label CI failed on Oct 24, 2023
  5. hebasto renamed this:
    gui: Check for private keys disabled before attempting unlock
    Check for private keys disabled before attempting unlock
    on Oct 25, 2023
  6. in src/qt/walletmodel.cpp:456 in 517c7f9cba
    448@@ -449,6 +449,13 @@ void WalletModel::unsubscribeFromCoreSignals()
    449 // WalletModel::UnlockContext implementation
    450 WalletModel::UnlockContext WalletModel::requestUnlock()
    451 {
    452+    // Bugs in earlier versions may have resulted in wallets with private keys disabled to become "encrypted"
    453+    // (encryption keys are present, but not actually doing anything).
    454+    // To avoid issues with such wallets, check if the wallet has private keys disabled, and if so, return a context
    455+    // that indicates the wallet is not encrypted.
    456+    if (m_wallet->privateKeysDisabled()) {
    


    hebasto commented at 10:35 am on October 25, 2023:

    It’s a pity that getEncryptionStatus() == NoKeys does not work here.

    Is it worth to warn the user when they load a watch-only wallet with an encryption keys?


    achow101 commented at 2:26 pm on October 25, 2023:

    It’s a pity that getEncryptionStatus() == NoKeys does not work here.

    It could, but #631 states that NoKeys is not returned for encrypted watchonly so that other users of getEncryptionStatus() get this status correctly. Perhaps that isn’t necessary, but I wanted to keep this change targeted for just this specific issue and changing getEncryptionStatus()’s behavior could have unanticipated side effects.

    Is it worth to warn the user when they load a watch-only wallet with an encryption keys?

    https://github.com/bitcoin/bitcoin/pull/28724 proposes to just delete them for the user on loading.

  7. hebasto approved
  8. hebasto commented at 10:41 am on October 25, 2023: member

    ACK 517c7f9cba306292e12e166b9dbc6c0838f05b27, I have reviewed the code and it looks OK.

    Do we want to backport this PR into the 26.x branch?

  9. hebasto added the label Wallet on Oct 25, 2023
  10. DrahtBot removed the label CI failed on Oct 25, 2023
  11. achow101 commented at 2:24 pm on October 25, 2023: member

    Do we want to backport this PR into the 26.x branch?

    It’s not a regression, although I think it would make sense to be backported.

  12. hebasto commented at 2:36 pm on October 25, 2023: member
    cc @furszy
  13. furszy commented at 7:51 pm on October 25, 2023: member

    Doesn’t mean that the migration process will also be blocked when this happen? –> look at this line getEncryptionStatus() should be returning Locked, which here would trigger the askpassphrase dialog in the same way described on #772.

    Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn’t a problem anymore.

  14. achow101 commented at 8:01 pm on October 25, 2023: member

    Doesn’t mean that the migration process will also be blocked when this happen? –> look at this line getEncryptionStatus() should be returning Locked, which here would trigger the askpassphrase dialog in the same way described on #772.

    That’s a good point. Perhaps getEncryptionStatus() should just move to returning NoKeys for all private key disabled wallets.

    Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn’t a problem anymore.

    28724 is a much scarier change since it deletes things from the wallet database and I wasn’t sure if people were comfortable with that. If we move forward with it, then that can supersede this PR.

  15. BrandonOdiwuor commented at 9:57 am on November 17, 2023: contributor

    ACK 517c7f9cba306292e12e166b9dbc6c0838f05b27

    looks good to me

  16. DrahtBot added the label CI failed on Jan 14, 2024
  17. hebasto commented at 2:09 pm on February 12, 2024: member

    Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn’t a problem anymore.

    28724 is a much scarier change since it deletes things from the wallet database and I wasn’t sure if people were comfortable with that. If we move forward with it, then that can supersede this PR.

    I’m going to merge this to get this bugfix into 27.0 as https://github.com/bitcoin/bitcoin/pull/28724 remains untagged.

  18. hebasto merged this on Feb 12, 2024
  19. hebasto closed this on Feb 12, 2024


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-06 22:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me