wallet: fix confusing error / GUI crash on cross-chain legacy wallet restore #26747

theStack commented at 2:46 AM on December 23, 2022: contributor

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_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 already 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. With this, the expected error message appears both for the restorewallet RPC call and in the GUI (as a message-box):

$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -4
error message:
Wallet loading failed. Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override.

wallet: fix GUI crash on cross-chain legacy wallet restore

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`.

8c7222bda3

DrahtBot commented at 2:46 AM on December 23, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, achow101, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Wallet on Dec 23, 2022

furszy approved

furszy commented at 12:53 PM on December 27, 2022: member

Tested ACK 8c7222bd

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."

achow101 commented at 8:22 PM on January 3, 2023: member

Could add a test?

test: add coverage for cross-chain wallet restore 21ad4e26ec

theStack commented at 11:17 PM on January 3, 2023: contributor

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 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).

Could add a test?

Indeed, done.

aureleoules approved

aureleoules commented at 10:47 AM on January 4, 2023: member

ACK 21ad4e26ec320dcecc8961888bc82d0bb72d5ed3

I verified the test fails on master with python test/functional/wallet_crosschain.py --legacy-wallet.

achow101 commented at 5:23 PM on January 4, 2023: member

ACK 21ad4e26ec320dcecc8961888bc82d0bb72d5ed3

furszy approved

furszy commented at 7:44 PM on January 4, 2023: member

utACK 21ad4e26

achow101 merged this on Jan 4, 2023

achow101 closed this on Jan 4, 2023

theStack deleted the branch on Jan 4, 2023

bitcoin locked this on Jan 4, 2024