wallet: Don't generate keys for wallets with private keys disabled during upgradewallet #24365

1. 
   achow101 commented at 3:58 AM on February 17, 2022: member

   When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.

   Fixes #23610

2. wallet: Don't generate keys when privkeys disabled when upgrading

   When private keys are disabled, we should not be trying to generate new
   keys during upgradewallet.

   3d985d4f43
3. DrahtBot added the label Wallet on Feb 17, 2022
4. 
   laanwj commented at 8:45 AM on February 17, 2022: member

   Concept ACK

5. fanquake requested review from meshcollider on Feb 17, 2022
6. in src/wallet/scriptpubkeyman.cpp:473 in 34afde023a outdated

    468 | @@ -469,6 +469,12 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal) const
    469 |  bool LegacyScriptPubKeyMan::Upgrade(int prev_version, int new_version, bilingual_str& error)
    470 |  {
    471 |      LOCK(cs_KeyStore);
    472 | +
    473 | +    if (m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
   

   ---

   ---

   

   w0xlt commented at 2:37 PM on February 17, 2022:

   nit: If the user is expecting the wallet upgrade, shouldn't this condition return false and inform the reason in error?

       if (m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
               error = _("Private keys disabled");
               return false;
       }
   

   ---

   

   achow101 commented at 4:02 PM on February 17, 2022:

   No, the wallet is still being upgraded - the version number will change, and in the future (perhaps in CWallet), there may actually be an upgrade. It is fine for an upgrade to do nothing - that is what we do for descriptor wallets.

7. w0xlt approved
8. 
   w0xlt commented at 2:46 PM on February 17, 2022: contributor

   tACK 34afde0 on Ubuntu 21.10

9. MarcoFalke added this to the milestone 23.0 on Feb 17, 2022
10. fanquake requested review from instagibbs on Feb 21, 2022
11. in test/functional/wallet_upgradewallet.py:353 in 34afde023a outdated

     344 | @@ -345,5 +345,11 @@ def copy_split_hd():
     345 |              desc_wallet = self.nodes[0].get_wallet_rpc("desc_upgrade")
     346 |              self.test_upgradewallet(desc_wallet, previous_version=169900, expected_version=169900)
     347 |  
     348 | +        if self.is_bdb_compiled():
    

    ---

    ---

    

    instagibbs commented at 1:34 AM on February 22, 2022:

    can we also test descriptor+disable_private_keys while we're here?

    ---

    

    achow101 commented at 6:53 PM on February 22, 2022:

    Done

12. tests: Test upgrading wallet with privkeys disabled c7376cc8d7
13. achow101 force-pushed on Feb 22, 2022
14. benthecarman approved
15. 
    benthecarman commented at 6:56 AM on February 23, 2022: contributor

    tACK c7376cc8d728f3a7c40f79bd57e7cef685def723 this fixed the issue for me

16. fanquake requested review from instagibbs on Feb 23, 2022
17. 
    fanquake commented at 10:56 AM on February 23, 2022: member

    @w0xlt want to take another look here?

18. 
    laanwj commented at 12:10 PM on February 28, 2022: member

    Code review ACK c7376cc8d728f3a7c40f79bd57e7cef685def723

19. laanwj merged this on Feb 28, 2022
20. laanwj closed this on Feb 28, 2022

    ---

21. DrahtBot locked this on Feb 28, 2023

Contributors

achow101laanwjw0xltinstagibbsbenthecarmanfanquake

Review Requested
meshcolliderinstagibbs

Milestone
23.0