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

1. 
   achow101 commented at 12: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 12:42 AM on May 23, 2025: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

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

   <!--021abf342d371248e50ceaed478a90ca-->

   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.

   <!--174a7506f384e20aa4161008e828411d-->

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

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 12: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

    ---

21. sedited referenced this in commit f236a50dc1 on Jun 14, 2025
22. yuvicc referenced this in commit 069643f094 on Jul 6, 2025
23. yuvicc referenced this in commit 22f55cf11d on Aug 26, 2025
24. bug-castercv502 referenced this in commit cfdba67027 on Sep 28, 2025
25. stickies-v referenced this in commit a19c56cd7c on Nov 4, 2025
26. Kino1994 referenced this in commit b7f8451756 on Jun 28, 2026
27. BigcoinBGC referenced this in commit 38d3a7d654 on Jun 30, 2026
28. bitcoin locked this on Jul 30, 2026

Contributors

achow101DrahtBotfurszyrkruxjonatackadyshimonydavidgumbergSjors

Review Requested
jonatack