walletdb: Log additional exception error messages for corrupted wallets #32598

1. 
   achow101 commented at 0:42 am on May 23, 2025: member

   Many exceptions thrown for corruption are std::runtime_error; we should catch those and log the message to help with debugging.

   Split from #32489

2. 
   DrahtBot commented at 0:42 am on May 23, 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/32598.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, rkrux, davidgumberg, Sjors
   Concept ACK	jonatack

   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:

   • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
   • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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. 
   furszy commented at 2:04 pm on May 23, 2025: member

   Should also change HasLegacyRecords() as it would log the same error twice with this change.
4. 
   rkrux commented at 2:32 pm on May 23, 2025: contributor

   ACK 62ad4d80883191073ea39f8b8d5ccef0748414a1

   Should also change HasLegacyRecords() as it would log the same error twice with this change.

   That^ log can be removed as I checked that the only other usage of HasLegacyRecords is directly inside the MigrateLegacyToDescriptor function that comes after the corresponding LoadWallet call. So, there should not be any informative log missing after its removal.

5. walletdb: Log additional exception error messages for corrupted wallets ad9a13fc42
6. 
   achow101 commented at 6:31 pm on May 23, 2025: member

   Should also change HasLegacyRecords() as it would log the same error twice with this change.

   Done

7. achow101 force-pushed on May 23, 2025
8. 
   jonatack commented at 7:02 pm on May 23, 2025: member

   Concept ACK, would be great to add test coverage for this change (if feasible).
9. 
   furszy commented at 9:16 pm on May 23, 2025: member

   ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
10. DrahtBot requested review from jonatack on May 23, 2025
11. DrahtBot requested review from rkrux on May 23, 2025
12. rkrux approved
13. 
    rkrux commented at 6:03 am on May 24, 2025: contributor

    ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
14. in src/wallet/walletdb.cpp:1196 in ad9a13fc42

    1192@@ -1194,9 +1193,15 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
    1193 
    1194         // Load decryption keys
    1195         result = std::max(LoadDecryptionKeys(pwallet, *m_batch), result);
    1196-    } catch (...) {
    1197+    } catch (std::runtime_error& e) {
    

    ---

    ---

    

    adyshimony commented at 0:48 am on May 28, 2025:

    nit: const std::runtime_error& e
15. in src/wallet/walletdb.cpp:1201 in ad9a13fc42

    1197+    } catch (std::runtime_error& e) {
    1198         // Exceptions that can be ignored or treated as non-critical are handled by the individual loading functions.
    1199         // Any uncaught exceptions will be caught here and treated as critical.
    1200+        // Catch std::runtime_error specifically as many functions throw these and they at least have some message that
    1201+        // we can log
    1202+        pwallet->WalletLogPrintf("%s\n", e.what());
    

    ---

    ---

    

    adyshimony commented at 2:00 am on May 28, 2025:

    nit: pwallet->WalletLogPrintf(“Error loading wallet - runtime error: %s\n”, e.what());

    ---

    

    achow101 commented at 7:37 pm on May 28, 2025:

    There is no need to log the exception type, especially since runtime_error is extraordinarily generic.
16. 
    davidgumberg commented at 1:03 am on May 30, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
17. Sjors approved
18. 
    Sjors commented at 10:23 am on May 30, 2025: member

    utACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
19. fanquake merged this on May 30, 2025
20. fanquake closed this on May 30, 2025

    ---

Contributors
achow101 DrahtBot furszy rkrux jonatack adyshimony davidgumberg Sjors

Review Requested
jonatack