wallet: Fix wallet interface detection of encrypted wallets #32620

1. 
   achow101 commented at 10:06 pm on May 26, 2025: member

   The GUI uses WalletLoader::isEncrypted() to detect whether a wallet file is encrypted so that it knows whether to prompt for a passphrase when migrating a legacy wallet. However, legacy wallets need to be opened with options.require_format = BERKELEY_RO. Since this wasn’t being provided, following #28710, encrypted legacy wallets could not be migrated.

   This fixes the issue by detecting when a wallet file is for a legacy wallet, and re-attempting with options.require_format = BERKELEY_RO in that case.

   Depends on #32449 for DatabaseStatus::FAILED_LEGACY_DISABLED

2. 
   DrahtBot commented at 10:06 pm on May 26, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32620.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, pablomartin4btc, w0xlt, davidgumberg, rkrux

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

3. achow101 marked this as a draft on May 26, 2025
4. DrahtBot added the label Wallet on May 26, 2025
5. achow101 force-pushed on Jun 4, 2025
6. achow101 marked this as ready for review on Jun 4, 2025
7. 
   achow101 commented at 11:51 pm on June 4, 2025: member

   Ready for review now.
8. wallet, interfaces: Use BERKELEY_RO in isEncrypted

   Since isEncrypted opens a file to search for a particular record, it
   needs to use BERKELEY_RO when opening a legacy wallet.

   130a922980
9. maflcko added the label GUI on Jun 5, 2025
10. 
    furszy commented at 2:22 pm on June 5, 2025: member

    utACK 130a922980778b293b22169d5e5649afde3ba33b
11. pablomartin4btc approved
12. 
    pablomartin4btc commented at 3:21 am on June 10, 2025: member

    tACK 130a922980778b293b22169d5e5649afde3ba33b

    

    

13. 
    w0xlt commented at 6:33 pm on June 11, 2025: contributor

    Code review ACK https://github.com/bitcoin/bitcoin/pull/32620/commits/130a922980778b293b22169d5e5649afde3ba33b
14. 
    davidgumberg commented at 11:50 pm on June 11, 2025: contributor

    Tested ACK https://github.com/bitcoin/bitcoin/commit/130a922980778b293b22169d5e5649afde3ba33b

    Created a legacy wallet on v29.0 as follows:

    0git checkout v29.0
    1cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
    2./build/bin/bitcoind -signet -daemonwait -deprecatedrpc=create_bdb
    3./build/bin/bitcoin-cli -signet -named createwallet wallet_name=encrypted_legacy descriptors=false passphrase="encrypted_legacy"
    

    and tested similarly to @pablomartin4btc:

    When migrating the legacy wallet in bitcoin-qt by going to File->Migrate Wallet I am prompted with whether or not I want to migrate my wallet.

    

    After clicking Yes on master (5757de4ddd37), I see a dialog informing me that wallet decryption failed because my password was incorrect.

    .

    On this branch’s https://github.com/bitcoin/bitcoin/commit/130a922980778b293b22169d5e5649afde3ba33b, after clicking Yes, I am prompted for my passphrase, and migration succeeds!

    

15. maflcko added this to the milestone 30.0 on Jun 12, 2025
16. in src/wallet/interfaces.cpp:649 in 130a922980

    642@@ -643,6 +643,10 @@ class WalletLoaderImpl : public WalletLoader
    643         DatabaseStatus status;
    644         bilingual_str error;
    645         auto db = MakeWalletDatabase(wallet_name, options, status, error);
    646+        if (!db && status == wallet::DatabaseStatus::FAILED_LEGACY_DISABLED) {
    647+            options.require_format = wallet::DatabaseFormat::BERKELEY_RO;
    648+            db = MakeWalletDatabase(wallet_name, options, status, error);
    649+        }
    

    ---

    ---

    

    rkrux commented at 10:12 am on June 12, 2025:

    While adding this BERKELEY_RO database format fallback in this seemingly generic isEncrypted function looks unsafe, it is fine because the only usage of this function lies in the wallet migration functionality in the GUI. The FAILED_LEGACY_DISABLED error from MakeDatabase also enforces the flow towards wallet migration.

    Using the require_format = DatabaseFormat::BERKELEY_RO checks seems like an indirect way of validating the migration flow though when the MakeWalletDatabase function already decides to pick BERKELEY_RO format for BDB wallet files, perhaps a boolean might replace it in the future.

    ---

    

    achow101 commented at 6:41 pm on June 12, 2025:

    Even if this was not limited to migration, this would still be correct because we need to be able to open legacy wallets in order to determine whether the wallet is encrypted. The database is closed once this function exits. The only reason to do this is because MakeWalletDatabase needs to be explicitly told when to use BERKELEY_RO so that we don’t accidentally try to use legacy wallets in normal operation.
17. rkrux approved
18. 
    rkrux commented at 10:19 am on June 12, 2025: contributor

    utACK 130a922980778b293b22169d5e5649afde3ba33b

    In #31250, the check was added for require_format to be equal to BERKELEY_RO, which occurs only in migration, so for encrypted legacy wallets to be migrated from the GUI this format fallback is required. The migration flow from the RPC accepts the passphrase argument for encrypted wallets, and this isEncrypted function is not called from there.

19. glozow added the label Needs backport (29.x) on Jun 13, 2025
20. glozow merged this on Jun 13, 2025
21. glozow closed this on Jun 13, 2025

    ---

22. glozow removed the label Needs backport (29.x) on Jun 17, 2025

Contributors
achow101 DrahtBot furszy pablomartin4btc w0xlt davidgumberg rkrux

Labels
GUI Wallet

Milestone
30.0