Skip to content

Conversation

@theStack
Copy link
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.

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`.
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 23, 2022

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.

Copy link
Member

@furszy furszy left a 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."

@achow101
Copy link
Member

achow101 commented Jan 3, 2023

Could add a test?

@theStack
Copy link
Contributor Author

theStack commented Jan 3, 2023

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.

Copy link
Contributor

@aureleoules aureleoules left a 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.

@achow101
Copy link
Member

achow101 commented Jan 4, 2023

ACK 21ad4e2

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 21ad4e2

@achow101 achow101 merged commit 360e047 into bitcoin:master Jan 4, 2023
@theStack theStack deleted the 202212-wallet-fix_restorewallet_bdb_crosschain branch January 4, 2023 23:04
@bitcoin bitcoin locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants