Add a new RPC command: restorewallet #22541

pull lsilva01 wants to merge 2 commits into bitcoin:master from lsilva01:restore_wallet_command changing 3 files +125 −40
  1. lsilva01 commented at 11:21 pm on July 24, 2021: contributor

    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

  2. DrahtBot added the label RPC/REST/ZMQ on Jul 24, 2021
  3. DrahtBot added the label Wallet on Jul 24, 2021
  4. Imanuelc112 approved
  5. lsilva01 force-pushed on Jul 26, 2021
  6. lsilva01 force-pushed on Jul 26, 2021
  7. lsilva01 force-pushed on Jul 26, 2021
  8. in src/wallet/rpcwallet.cpp:2726 in b1b6a4f8e5 outdated 
    2721+        },
2722+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2723+{
2724+
2725+    WalletContext& context = EnsureWalletContext(request.context);
2726+
    unknown commented at 5:50 am on July 27, 2021:
  9. in src/wallet/rpcwallet.cpp:2737 in b1b6a4f8e5 outdated 
    2732+
2733+    std::string wallet_name = request.params[0].get_str();
2734+
2735+    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), wallet_name);
2736+    fs::file_type path_type = fs::symlink_status(wallet_path).type();
2737+
    unknown commented at 5:51 am on July 27, 2021:
  10. in src/wallet/rpcwallet.cpp:2771 in b1b6a4f8e5 outdated 
    2766+
2767+    std::vector<bilingual_str> warnings;
2768+
2769+    std::optional<bool> load_on_start = request.params[2].isNull() ? std::nullopt : std::optional<bool>(request.params[2].get_bool());
2770+    std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, wallet_name, load_on_start, options, status, error, warnings);
2771+
    unknown commented at 5:51 am on July 27, 2021:
  11. unknown changes_requested
  12. unknown commented at 6:01 am on July 27, 2021: none

    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

  13. lsilva01 force-pushed on Jul 27, 2021
  14. Rspigler commented at 7:57 pm on July 27, 2021: contributor
    Concept ACK!
  15. laanwj commented at 2:45 pm on July 28, 2021: member

    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.

  16. in src/wallet/rpcwallet.cpp:2850 in 7b1b40a055 outdated 
    2845+        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Failed to create database path '%s'. Database already exists.", wallet_path.string()));
2846+    }
2847+
2848+    auto wallet_file = wallet_path / "wallet.dat";
2849+
2850+    std::ifstream  src(backup_file, std::ios::binary);
    laanwj commented at 2:49 pm on July 28, 2021:
    fs::copy_file?
    lsilva01 commented at 8:49 pm on July 29, 2021:
    Done.
  17. ghost commented at 1:24 am on July 29, 2021: none

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

    This gives error:

    0$ bitcoin-cli loadwallet "B1.dat"
1
2error code: -4
3error message:
4BerkeleyDatabase: Can't open database B1.dat (duplicates fileid b6050000000024007aceafe40000000000000000 from wallet.dat)
  18. S3RK commented at 6:56 am on July 29, 2021: member
    @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
  19. ghost commented at 12:14 pm on July 29, 2021: none

    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?

  20. in src/wallet/rpcwallet.cpp:2795 in 7b1b40a055 outdated 
    2791@@ -2792,6 +2792,107 @@ static RPCHelpMan createwallet()
2792     };
2793 }
2794 
2795+static RPCHelpMan restorewalletfrombackup()
    achow101 commented at 5:11 pm on July 29, 2021:
    I would really prefer a shorter command name. I think calling it just restorewallet is good enough.
    lsilva01 commented at 8:49 pm on July 29, 2021:
    Changed to restorewallet.
  21. in src/wallet/rpcwallet.cpp:2826 in 7b1b40a055 outdated 
    2821+    WalletContext& context = EnsureWalletContext(request.context);
2822+
2823+    std::string backup_file = request.params[1].get_str();
2824+
2825+    if (!fs::exists(backup_file)) {
2826+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Inexistent backup file");
    achow101 commented at 6:03 pm on July 29, 2021:

    Inexistent is not a word.

    0        throw JSONRPCError(RPC_INVALID_PARAMETER, "Backup file does not exist");
    lsilva01 commented at 8:50 pm on July 29, 2021:
    Fixed.
  22. in src/wallet/rpcwallet.cpp:2842 in 7b1b40a055 outdated 
    2837+        throw JSONRPCError(RPC_WALLET_ERROR, Untranslated(strprintf(
2838+              "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and "
2839+              "database/log.?????????? files can be stored, a location where such a directory could be created, "
2840+              "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)",
2841+              wallet_name, GetWalletDir())).original);
2842+    }
    achow101 commented at 6:15 pm on July 29, 2021:

    This if block is entirely unnecessary. It is largely copied from MakeWalletDatabase. However this is incorrect since MakeWalletDatabase is used for both creating new wallets, and opening existing ones. In a restore RPC, we do not want the restored wallet to be copied over an existing one. So allowing existing directories and files are not correct.

    This should only check to see if a something exists in the wallet dir with the name that we want. If something does, that is an error and we should fail. But it is unnecessary to implement this check by ourselves, TryCreateDirectories below will do that for us and return false if something does exist. So this entire if block is not doing anything useful, and is potentially dangerous.

    lsilva01 commented at 8:50 pm on July 29, 2021:
    if block removed.
  23. in src/wallet/rpcwallet.cpp:2801 in 7b1b40a055 outdated 
    2796+{
2797+    return RPCHelpMan{
2798+        "restorewalletfrombackup",
2799+        "\nRestore and loads a wallet from backup.\n",
2800+        {
2801+            {"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name for the wallet. If this is a path, the wallet will be restored at the path location."},
    achow101 commented at 6:18 pm on July 29, 2021:

    The wallet will not be restored at an arbitrary path as below we are combining this with GetWalletDir(). I don’t think we should mention paths at all for the help for wallet_name

    0            {"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name that will be applied to the restored wallet"},
    lsilva01 commented at 8:52 pm on July 29, 2021:
    Changed to wallet_name only.
  24. achow101 commented at 6:23 pm on July 29, 2021: member
    Concept ACK
  25. lsilva01 force-pushed on Jul 29, 2021
  26. lsilva01 force-pushed on Jul 29, 2021
  27. lsilva01 renamed this:
    Add a new RPC command: restorewalletfrombackup
    Add a new RPC command: restorewallet
    on Jul 29, 2021
  28. in src/wallet/rpcwallet.cpp:2854 in 2f70a30cf3 outdated 
    2838+        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Failed to create database path '%s'. Database already exists.", wallet_path.string()));
2839+    }
2840+
2841+    auto wallet_file = wallet_path / "wallet.dat";
2842+
2843+    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
    achow101 commented at 5:57 pm on July 30, 2021:
    This will return a bool, we should throw an error if it is false.
    lsilva01 commented at 6:13 pm on July 30, 2021:

    According to the documentation, the return type is void, but throws an error if it cannot copy. So try/catch can be used instead.

    0    try
1    {
2        fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
3    } catch (const boost::filesystem::filesystem_error& e)
4    {
5        throw JSONRPCError(RPC_MISC_ERROR, "Unable to copy backup file.");
6    }
    achow101 commented at 6:29 pm on July 30, 2021:

    Oh, we still use boost..

    It’s fine to not try/catch as the RPC server will catch those itself.

  29. lsilva01 force-pushed on Jul 30, 2021
  30. lsilva01 force-pushed on Jul 30, 2021
  31. lsilva01 commented at 7:32 pm on July 30, 2021: contributor
    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.
  32. in src/wallet/rpcwallet.cpp:4727 in e7c86cdb6b outdated 
    4723@@ -4636,6 +4724,7 @@ static const CRPCCommand commands[] =
4724     { "wallet",             &bumpfee,                        },
4725     { "wallet",             &psbtbumpfee,                    },
4726     { "wallet",             &createwallet,                   },
4727+    { "wallet",             &restorewallet,        },
    achow101 commented at 7:25 pm on August 9, 2021:

    In commit e7c86cdb6b0f1b50816b9aec7a9ce1bfeedd32f5 “Add a new RPC command: restorewallet”

    nit: whitespace, } should line up with the rest.

    lsilva01 commented at 2:37 am on August 10, 2021:
    Done.
  33. in src/wallet/rpcwallet.cpp:2862 in e7c86cdb6b outdated 
    2872+
2873+    UniValue obj(UniValue::VOBJ);
2874+    obj.pushKV("name", wallet->GetName());
2875+    obj.pushKV("warning", Join(warnings, Untranslated("\n")).original);
2876+
2877+    return obj;
    achow101 commented at 7:27 pm on August 9, 2021:

    In commit e7c86cdb6b0f1b50816b9aec7a9ce1bfeedd32f5 “Add a new RPC command: restorewallet”

    Since all of this code is basically just copied from the loadwallet RPC, I think it would be better to refactor it into a helper function that both restorewallet and loadwallet can call.

    lsilva01 commented at 2:37 am on August 10, 2021:
    Great suggestion. Done.
  34. lsilva01 force-pushed on Aug 10, 2021
  35. lsilva01 commented at 2:37 am on August 10, 2021: contributor

    https://github.com/bitcoin/bitcoin/pull/22541/commits/5b0a506962fe4388db13062e105201773954e4ee: Adds a helper function to load the wallet which is used by restorewallet and loadwallet.

    https://github.com/bitcoin/bitcoin/pull/22541/commits/8f87ef44013caf78e1405ca89f8097cbde1a11b0: Increases coverage ofrestorewallet test. Adds restore_nonexistent_wallet and restore_wallet_existent_name functions.

  36. achow101 commented at 8:20 pm on August 10, 2021: member
    ACK 8f87ef44013caf78e1405ca89f8097cbde1a11b0
  37. Add a new RPC command: restorewallet ae23faba6f
  38. Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. 5fe8100ff3
  39. in src/wallet/rpcwallet.cpp:2575 in 5b0a506962 outdated 
    2571@@ -2572,6 +2572,37 @@ static RPCHelpMan listwallets()
2572     };
2573 }
2574 
2575+static std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWallet(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name)
    achow101 commented at 8:21 pm on August 10, 2021:

    In 5b0a506962fe4388db13062e105201773954e4ee “Add a new RPC command: restorewallet”

    nit: I would prefer for this to not be named the same as the LoadWallet function in wallet.cpp

    lsilva01 commented at 2:31 am on August 11, 2021:
    Done. Changed the name to LoadWalletHelper.
  40. lsilva01 force-pushed on Aug 11, 2021
  41. achow101 commented at 8:38 pm on August 13, 2021: member
    re-ACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
  42. DrahtBot commented at 1:36 am on August 14, 2021: member

    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.

  43. meshcollider commented at 4:03 am on August 15, 2021: contributor
    utACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
  44. ghost commented at 10:09 am on August 15, 2021: none

    tACK https://github.com/bitcoin/bitcoin/commit/5fe8100ff36fed6d50c2a25b028f57b25af3504c

    Tested on windows with below commands:

    1. Create wallet W1:

      0bitcoin-cli createwallet W1

    2. Backup wallet:

      0bitcoin-cli -rpcwallet="w1" backupwallet "D:\Backups\backup_W1.dat"

    3. Delete wallet directory regtest\wallets\W1

    4. Restore wallet from backup:

      0bitcoin-cli restorewallet "W1" "D:\Backups\backup_W1.dat"

      0{
1 "name": "W1",
2 "warning": ""
3}

    5. Check wallet info: bitcoin-cli -rpcwallet="W1" getwalletinfo

  45. meshcollider commented at 1:53 pm on August 15, 2021: contributor
    Great first contribution, thank you! 🎉
  46. meshcollider merged this on Aug 15, 2021
  47. meshcollider closed this on Aug 15, 2021
  48. sidhujag referenced this in commit 43c7753baf on Aug 15, 2021
  49. lsilva01 deleted the branch on Aug 23, 2021
  50. DrahtBot locked this on Aug 23, 2022


lsilva01 Imanuelc112 Rspigler laanwj ghost S3RK achow101 DrahtBot meshcollider

Labels
Wallet RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me