wallettool: Always be able to dump a wallet’s database #29117

1. 
   achow101 commented at 10:05 pm on December 19, 2023: member

   #29109 (comment) reports that a wallet with noncritical errors cannot be dumped with bitcoin-wallet dump. This was caused by an erroneous reset of the wallet pointer when the loading the wallet returns something other than LOAD_OK. Not all errors are errors that require aborting, so unilaterally resetting the pointer at that time is incorrect. The first commit resolves this issue.

   Furthermore, if a wallet has loading errors, that should not prevent the wallet tool from dumping the wallet. The wallet application logic should not get in the way of performing such a low level database operation, especially when it’s primary usage is for debugging potentially corrupted wallets. The 2nd commit is taken from #28710 and changes the dump to stop at making a WalletDatabase rather than making a CWallet only to retrieve the underlying WalletDatabase.

2. wallettool: Don't unilaterally reset wallet_instance if loading error

   When there is a wallet loading error, it could be a noncritical one so
   it is not necessary to make wallet_instance a nullptr. The wallet can
   still go on with normal operation in that case, as we do for loading in
   bitcoind and bitcoin-qt.

   40c80e36b1
3. wallettool: Don't create CWallet when dumping DB

   It's not necessary to set up an entire CWallet just so we can get access
   to the WalletDatabase and read the records. Instead we can go one level
   lower and make just a WalletDatabase.

   d83bea42d1
4. 
   DrahtBot commented at 10:05 pm on December 19, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, BrandonOdiwuor

   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:

   • #26606 (wallet: Implement independent BDB parser by achow101)
   • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB 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.

5. in src/wallet/dump.cpp:27 in d83bea42d1

   20@@ -20,7 +21,7 @@ namespace wallet {
   21 static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP";
   22 uint32_t DUMP_VERSION = 1;
   23 
   24-bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
   25+bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error)
   26 {
   27     // Get the dumpfile
   28     std::string dump_filename = args.GetArg("-dumpfile", "");
   

   ---

   ---

   

   furszy commented at 10:31 pm on December 19, 2023:

   Not for this PR but could also provide the -dumpfile path arg instead of the entire ArgsManager and remove <common/args.h> dependency.
6. 
   furszy commented at 10:34 pm on December 19, 2023: member

   Code review ACK d83bea42d1
7. BrandonOdiwuor approved
8. 
   BrandonOdiwuor commented at 2:01 pm on December 21, 2023: contributor

   Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995
9. fanquake merged this on Jan 5, 2024
10. fanquake closed this on Jan 5, 2024

    ---

Contributors
achow101 DrahtBot furszy BrandonOdiwuor