wallettool: Always be able to dump a wallet's database #29117
Conversation
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.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error) | ||
| { | ||
| // Get the dumpfile | ||
| std::string dump_filename = args.GetArg("-dumpfile", ""); |
There was a problem hiding this comment.
Not for this PR but could also provide the -dumpfile path arg instead of the entire ArgsManager and remove <common/args.h> dependency.
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. Github-Pull: bitcoin#29117 Rebased-From: d83bea4
#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 thanLOAD_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
dumpto stop at making aWalletDatabaserather than making aCWalletonly to retrieve the underlyingWalletDatabase.