-
Notifications
You must be signed in to change notification settings - Fork 38.7k
wallet: fix confusing error / GUI crash on cross-chain legacy wallet restore #26747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: fix confusing error / GUI crash on cross-chain legacy wallet restore #26747
Conversation
Restoring a wallet backup from another chain should obviously result in a dedicated error message (we have "Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override." for that). Unfortunately this is currently not the case for legacy wallet restores, as in the course of cleaning up the newly created wallet directory a `filesystem_error` exception is thrown due to the directory not being empty; the wallet database did indeed load successfully (otherwise we wouldn't know that the chain doesn't match) and hence BDB-related files and directories are created in the wallet directory. For bitcoind, this leads to a very confusing error message: ``` $ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat error code: -1 error message: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/test123"] ``` Even worse, the GUI crashes in such a scenario: ``` libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/foobar"] Abort trap (core dumped) ``` Fix this by simply deleting the whole folder via `fs::remove_all`.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
furszy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 8c7222b
With a note:
The error is different when you restore a regtest wallet on mainnet than when you restore a testnet wallet on regtest.
The latter one does not crash, it shows up a dialog with the following error message:
"Wallet file verification failed. Failed to load database path <path_to_file>. Data is not in recognized format."
|
Could add a test? |
Could it be that you did the second test with a descriptor wallet instead of a legacy one? What message appears and whether a crash happens on master seems to be only dependent on the wallet type, and not on the chains being involved (actual chain vs. chain of restored wallet).
Indeed, done. |
aureleoules
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 21ad4e2
I verified the test fails on master with python test/functional/wallet_crosschain.py --legacy-wallet.
|
ACK 21ad4e2 |
furszy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 21ad4e2
Restoring a wallet backup from another chain should result in a dedicated error message (we have "Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override." for that). Unfortunately this is currently not the case for legacy wallet restores, as in the course of cleaning up the newly created wallet directory a
filesystem_errorexception is thrown due to the directory not being empty; the wallet database did indeed load successfully (otherwise we wouldn't know that the chain doesn't match) and hence BDB-related files and directories are already created in the wallet directory.For bitcoind, this leads to a very confusing error message:
Even worse, the GUI crashes in such a scenario:
Fix this by simply deleting the whole folder via
fs::remove_all. With this, the expected error message appears both for therestorewalletRPC call and in the GUI (as a message-box):