Skip to content
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

Add a new RPC command: restorewallet #22541

Merged
merged 2 commits into from
Aug 15, 2021

Conversation

lsilva01
Copy link
Contributor

@lsilva01 lsilva01 commented Jul 24, 2021

As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the wallet.dat of a newly created wallet with the backup file, which is hardly an intuitive way.

This PR implements the restorewallet RPC command which restores the wallet from the backup file.

To test:
First create a backup file:
$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak

Then restore it in another wallet:
$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak

@lsilva01 lsilva01 force-pushed the restore_wallet_command branch 3 times, most recently from 587d0d3 to b1b6a4f Compare July 26, 2021 21:06
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

CI is failing because of whitespace at few places https://cirrus-ci.com/task/6700329929539584

You can automatically remove this pre commit by using "files.trimTrailingWhitespace": true in VS Code

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@Rspigler
Copy link
Contributor

Concept ACK!

@laanwj
Copy link
Member

laanwj commented Jul 28, 2021

Concept ACK. I think this is more of an importwallet than restorewallet in the sense that an external wallet file is being imported into the data directory, which is slightly more general. But no strong opinion on RPC naming (ofc, importwallet already exists and does something else,).

The only way to do this is to replace the wallet.dat of a newly created wallet with the backup file, which is hardly an intuitive way.

I don't think the replacement part is necessary (or desirable). As I understand, you can place it in the wallets directory then do loadwallet.

Needs a RPC test.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jul 29, 2021

As I understand, you can place it in the wallets directory then do loadwallet

This gives error:

$ bitcoin-cli loadwallet "B1.dat"

error code: -4
error message:
BerkeleyDatabase: Can't open database B1.dat (duplicates fileid b6050000000024007aceafe40000000000000000 from wallet.dat)

@S3RK
Copy link
Contributor

S3RK commented Jul 29, 2021

@prayank23 the error says that you are trying to load a copy of already existing wallet. Try the same with the wallet created on another node

@ghost
Copy link

ghost commented Jul 29, 2021

the error says that you are trying to load a copy of already existing wallet. Try the same with the wallet created on another node

Right. Wallet will be different when users restore from backup. Because if there is already a wallet, there is no need to restore.

Or it can be same in few cases?

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

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

Concept ACK

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@lsilva01 lsilva01 force-pushed the restore_wallet_command branch 2 times, most recently from 22f6d7b to 2f70a30 Compare July 29, 2021 20:47
@lsilva01 lsilva01 changed the title Add a new RPC command: restorewalletfrombackup Add a new RPC command: restorewallet Jul 29, 2021
@lsilva01 lsilva01 force-pushed the restore_wallet_command branch 2 times, most recently from 98c291c to bc241c7 Compare July 30, 2021 19:23
@lsilva01
Copy link
Contributor Author

lsilva01 commented Jul 30, 2021

test/functional/wallet_backup.py has been changed to use the restorewallet command instead of manually inserting the files.
The original test stopped nodes before inserting them. As this is no longer necessary, the backup files are used to restore the wallets on the fourth node without restarting it.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@lsilva01
Copy link
Contributor Author

5b0a506: Adds a helper function to load the wallet which is used by restorewallet and loadwallet.

8f87ef4: Increases coverage ofrestorewallet test. Adds restore_nonexistent_wallet and restore_wallet_existent_name functions.

@achow101
Copy link
Member

ACK 8f87ef4

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@achow101
Copy link
Member

re-ACK 5fe8100

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)

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.

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

utACK 5fe8100

@ghost
Copy link

ghost commented Aug 15, 2021

tACK 5fe8100

Tested on windows with below commands:

  1. Create wallet W1:

    bitcoin-cli createwallet W1
    
  2. Backup wallet:

    bitcoin-cli -rpcwallet="w1" backupwallet "D:\Backups\backup_W1.dat"
    
  3. Delete wallet directory regtest\wallets\W1

  4. Restore wallet from backup:

    bitcoin-cli restorewallet "W1" "D:\Backups\backup_W1.dat"
    
    {
     "name": "W1",
     "warning": ""
    }
    
  5. Check wallet info: bitcoin-cli -rpcwallet="W1" getwalletinfo

@meshcollider
Copy link
Contributor

Great first contribution, thank you! 🎉

@meshcollider meshcollider merged commit 502d22c into bitcoin:master Aug 15, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 15, 2021
5fe8100 Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23fab Add a new RPC command: restorewallet (lsilva01)

Pull request description:

  As far as I know, there is no command to restore the wallet from a backup file.
  The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.

  This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.

  To test:
  First create a backup file:
  `$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`

  Then restore it in another wallet:
  `$ bitcoin-cli  restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`

ACKs for top commit:
  achow101:
    re-ACK 5fe8100
  prayank23:
    tACK bitcoin@5fe8100
  meshcollider:
    utACK 5fe8100

Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
@lsilva01 lsilva01 deleted the restore_wallet_command branch August 23, 2021 13:11
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants