wallet: Give deprecation warning when loading a legacy wallet #27869

1. 
   achow101 commented at 10:15 pm on June 12, 2023: member

   Next step in legacy wallet deprecation.
2. 
   DrahtBot commented at 10:15 pm on June 12, 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	jonatack, S3RK
   Concept ACK	fanquake, glozow, darosior
   Stale ACK	dimitaracev

   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:

   • #28027 (test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets by achow101)

   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. DrahtBot added the label Wallet on Jun 12, 2023
4. 
   dimitaracev commented at 7:40 am on June 13, 2023: contributor

   ACK 956d05b
5. 
   fanquake commented at 11:03 am on June 13, 2023: member

   Concept ACK
6. 
   glozow commented at 1:44 pm on June 13, 2023: member

   Concept ACK
7. in src/wallet/wallet.cpp:250 in 956d05bad2 outdated

   244@@ -245,6 +245,11 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
   245             return nullptr;
   246         }
   247 
   248+        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
   249+        if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
   250+            warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));
   

   ---

   ---

   

   darosior commented at 2:15 pm on June 13, 2023:

   Maybe hint at how to migrate?

   ---

   

   achow101 commented at 4:15 pm on June 13, 2023:

   Added a hint.
8. 
   darosior commented at 2:16 pm on June 13, 2023: member

   Concept ACK. How about also logging for wallets that are loaded on startup?
9. achow101 force-pushed on Jun 13, 2023
10. DrahtBot added the label CI failed on Jun 13, 2023
11. 
    S3RK commented at 6:22 am on June 14, 2023: contributor

    ACK 2bf7183ea9b42a9edf76bccdf00708341a1d2c0a
12. in test/functional/wallet_backwards_compatibility.py:267 in 2bf7183ea9 outdated

    266@@ -267,7 +267,8 @@ def run_test(self):
    267             # Make sure this wallet opens without warnings. See https://github.com/bitcoin/bitcoin/pull/19054
    

    ---

    ---

    

    jonatack commented at 3:11 pm on June 14, 2023:

    It looks like this line could be updated or removed.

    ---

    

    achow101 commented at 8:37 pm on June 23, 2023:

    updated
13. 
    jonatack commented at 3:21 pm on June 14, 2023: contributor

    ACK 2bf7183ea9b42a9edf76bccdf00708341a1d2c0a

    Indeed, there is no debug log warning yet as suggested in #27869#pullrequestreview-1477293036.

    Sanity check:

     0$ ./src/bitcoin-cli -signet listwallets                 
     1[
     2  "sqlite3",
     3  "bdb"
     4]
     5$ ./src/bitcoin-cli -signet unloadwallet bdb            
     6{
     7}
     8$ ./src/bitcoin-cli -signet loadwallet bdb            
     9{
    10  "name": "bdb",
    11  "warnings": [
    12    "Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."
    13  ]
    14}
    

14. wallet: Give deprecation warning when loading a legacy wallet 8fbb6e99bf
15. achow101 force-pushed on Jun 23, 2023
16. DrahtBot removed the label CI failed on Jun 28, 2023
17. fanquake added this to the milestone 26.0 on Jun 30, 2023
18. achow101 requested review from S3RK on Jul 5, 2023
19. achow101 requested review from jonatack on Jul 5, 2023
20. achow101 requested review from darosior on Jul 5, 2023
21. 
    jonatack commented at 4:43 pm on July 5, 2023: contributor

    re-ACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd

    modulo #27869#pullrequestreview-1477293036

    Tested on the command line and with the GUI

22. in src/wallet/wallet.cpp:250 in 8fbb6e99bf

    244@@ -245,6 +245,11 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
    245             return nullptr;
    246         }
    247 
    248+        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
    249+        if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
    250+            warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."));
    

    ---

    ---

    

    jonatack commented at 4:49 pm on July 5, 2023:

    nit, the similar warning at the end of CreateWallet() could be updated to use this expanded one
23. 
    S3RK commented at 7:03 am on July 6, 2023: contributor

    reACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd
24. DrahtBot removed review request from S3RK on Jul 6, 2023
25. glozow merged this on Jul 6, 2023
26. glozow closed this on Jul 6, 2023

    ---

27. sidhujag referenced this in commit 1c9cc3f201 on Jul 6, 2023

Contributors
achow101 DrahtBot dimitaracev fanquake glozow darosior S3RK jonatack

Review Requested
darosior

Labels
Wallet

Milestone
26.0