[wallet] `createwallet` RPC - create new wallet at runtime #13058

pull jnewbery wants to merge 3 commits into bitcoin:master from jnewbery:create_wallet changing 3 files +77 −5
  1. jnewbery commented at 5:24 PM on April 23, 2018: member

    Adds a createwallet RPC to dynamically create a new wallet at runtime.

    Includes tests and release notes.

  2. jnewbery commented at 5:24 PM on April 23, 2018: member

    Builds on top of #10740. Please review that first.

  3. jnewbery added the label Wallet on Apr 23, 2018
  4. in src/wallet/wallet.cpp:3994 in f8b7016b52 outdated 
    3989 | +            wallet_file, GetWalletDir());
3990 | +        return false;
3991 | +    }
3992 | +
3993 | +    // Make sure that the wallet path doesn't clash with an existing wallet path
3994 | +    std::set<fs::path> wallet_paths;
    jonasschnelli commented at 6:20 PM on April 23, 2018:

    As mentioned in #13028, I think this does not have sufficient concurrency protection (maybe okay since edge case). AFAIK, callers calling CWallet::Verify as well as inside the call, there is no lock being held, thus, I think, it is possible that there could be a race in loadwallet and createwallet bypassing the filename check.

  5. in src/wallet/rpcwallet.cpp:3091 in 70277d2948 outdated 
    2954 | +
2955 | +    UniValue obj(UniValue::VSTR);
2956 | +    obj = wallet_name;
2957 | +
2958 | +    return obj;
2959 | +
    promag commented at 10:41 AM on April 29, 2018:

    Empty line.

  6. promag commented at 10:41 AM on April 29, 2018: member

    Concept ACK. Needs rebase.

  7. jnewbery added this to the "In progress" column in a project
  8. jnewbery force-pushed on May 16, 2018
  9. jnewbery commented at 9:56 PM on May 16, 2018: member

    rebased

  10. in src/wallet/rpcwallet.cpp:3140 in 6e6c113e7f outdated 
    3057 | +        );
3058 | +    std::string wallet_name = request.params[0].get_str();
3059 | +    std::string error;
3060 | +
3061 | +    fs::path wallet_path = fs::absolute(wallet_name, GetWalletDir());
3062 | +    if (fs::symlink_status(wallet_path).type() != fs::file_not_found) {
    promag commented at 3:58 PM on May 17, 2018:

    This doesn't prevent concurrency which is very unlikely and so can be improved later.

    jnewbery commented at 8:24 PM on May 17, 2018:

    CWallet::Verify() below takes the cs_wallets lock, so I think the worst that can happen is the first wallet creation succeeds and the second fails Verify().

  11. in test/functional/wallet_multiwallet.py:226 in 6e6c113e7f outdated 
     218 | +
 219 | +        # Successfully create a wallet with a new name
 220 | +        self.nodes[0].createwallet('w9')
 221 | +
 222 | +        assert 'w9' in self.nodes[0].listwallets()
 223 | +
    promag commented at 4:01 PM on May 17, 2018:

    Also call some method in w9, like get_wallet_rpc('w9').getwalletinfo()?

    jnewbery commented at 8:32 PM on May 17, 2018:

    sure. Added

  12. in test/functional/wallet_multiwallet.py:215 in 6e6c113e7f outdated 
     210 | @@ -211,5 +211,15 @@ def run_test(self):
 211 |          # Fail to load if wallet file is a symlink
 212 |          assert_raises_rpc_error(-4, "Wallet file verification failed: Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink')
 213 |  
 214 | +        self.log.info("Test dynamic wallet creation.")
 215 | +
    promag commented at 4:02 PM on May 17, 2018:

    Could test invalid argument.

    jnewbery commented at 8:27 PM on May 17, 2018:

    We don't exhaustively test invalid args for RPCs (since it's just hitting the univalue type checking code).

  13. in src/wallet/rpcwallet.cpp:3053 in 6e6c113e7f outdated 
    3048 | +            "createwallet \"filename\"\n"
3049 | +            "\nCreates and loads a new wallet.\n"
3050 | +            "\nArguments:\n"
3051 | +            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"
3052 | +            "\nResult:\n"
3053 | +            "<walletname>     (string) The wallet name if created successfully.\n"
    promag commented at 4:02 PM on May 17, 2018:

    Return same format as loadwallet?

    jnewbery commented at 8:33 PM on May 17, 2018:

    Done

  14. promag commented at 4:03 PM on May 17, 2018: member

    Tested ACK, but left a couple of comments for your appreciation.

  15. in test/functional/wallet_multiwallet.py:220 in 6e6c113e7f outdated 
     215 | +
 216 | +        # Fail to create a wallet if it already exists.
 217 | +        assert_raises_rpc_error(-4, "Wallet w2 already exists.", self.nodes[0].createwallet, 'w2')
 218 | +
 219 | +        # Successfully create a wallet with a new name
 220 | +        self.nodes[0].createwallet('w9')
    promag commented at 4:04 PM on May 17, 2018:

    Assert result.

    jnewbery commented at 8:33 PM on May 17, 2018:

    Done

  16. jnewbery force-pushed on May 17, 2018
  17. jnewbery commented at 8:34 PM on May 17, 2018: member

    Addressed @promag comments.

  18. promag commented at 9:06 AM on May 18, 2018: member

    Needs rebase.

  19. jonasschnelli commented at 1:37 PM on May 18, 2018: contributor

    Tested ACK 53e71dc4474db083f99ca0d30ca3abd5a492b1a6. Conceptually, I think adding an option for load/not-load could make sense,... or is the use care of pure wallet creation useless? Eventually create should never load since it should not hurt on RPC level to call a chain of commands to complete a task.

  20. jnewbery force-pushed on May 21, 2018
  21. jnewbery commented at 5:52 PM on May 21, 2018: member

    I think adding an option for load/not-load could make sense,... or is the use care of pure wallet creation useless? Eventually create should never load.

    My first thoughts would be that walletcreate should always load the wallet on creation. It seems unintuitive to have to call loadwallet immediately after creating a new wallet.

    If there's a requirement for 'create without load', then I think that should be provided by a bitcoin-wallet-tool. There's no need to have a full node running if you just want to create a new wallet file.

  22. jonasschnelli commented at 6:41 PM on May 21, 2018: contributor

    My first thoughts would be that walletcreate should always load the wallet on creation. It seems unintuitive to have to call loadwallet immediately after creating a new wallet.

    Not sure here. We do design an RPC interface/API and not end user function representations. An additional step seems acceptable if it widens the usability of the API.... though the question of what use cases a pure create could fulfil is one that may not have concrete answers today.

  23. jnewbery commented at 7:12 PM on May 21, 2018: member

    @jonasschnelli - I'm happy to take wider feedback on this. My preference would be to keep the current branch's behaviour (always load when creating) and decide whether to add create-without-load later with a new argument. I'm happy to change that though if others think we should have create-without-load now.

  24. jonasschnelli commented at 7:15 PM on May 21, 2018: contributor

    Yes. Lets try to get more opinions... I guess it could also be added later (an option to make create not load). However, this is a great PR and I'd like to underline my tested ACK #13058 (comment)

  25. promag commented at 7:18 PM on May 21, 2018: member

    Well if createwallet creates and loads the wallet and there is no use case for just create, then a valid alternative would be to add an option to loadwallet to create in case it doesn't exist?

  26. jnewbery commented at 8:03 PM on May 21, 2018: member

    a valid alternative would be to add an option to loadwallet to create in case it doesn't exist.

    I'd prefer to keep these two methods separate. It seems to me that reloading an existing wallet and creating a new wallet are completely different operations and should have their own RPC methods.

  27. jonasschnelli added this to the milestone 0.17.0 on May 25, 2018
  28. promag commented at 2:27 PM on May 25, 2018: member

    Needs rebase after #13063.

  29. in src/wallet/rpcwallet.cpp:3091 in e058f0d79e outdated 
    3086 | +    UniValue obj(UniValue::VOBJ);
3087 | +    obj.pushKV("name", wallet->GetName());
3088 | +    obj.pushKV("warning", warning);
3089 | +
3090 | +    return obj;
3091 | +
    promag commented at 2:27 PM on May 25, 2018:

    nit, remove empty line.

    jnewbery commented at 3:19 PM on May 25, 2018:

    removed

  30. in src/wallet/rpcwallet.cpp:3124 in e058f0d79e outdated 
    3049 | +    if (request.fHelp || request.params.size() != 1)
3050 | +        throw std::runtime_error(
3051 | +            "createwallet \"filename\"\n"
3052 | +            "\nCreates and loads a new wallet.\n"
3053 | +            "\nArguments:\n"
3054 | +            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"
    promag commented at 2:31 PM on May 25, 2018:

    Here wallet_name, above filename 🙄

    jnewbery commented at 3:19 PM on May 25, 2018:

    fixed

  31. jnewbery force-pushed on May 25, 2018
  32. jnewbery commented at 3:20 PM on May 25, 2018: member

    Addressed @promag's comments.

  33. [wallet] [rpc] Add `createwallet` RPC
    Add a `createwallet` RPC to allow wallets to be created dynamically at
runtime. This functionality is currently only available through RPC and
newly created wallets will not be displayed in the GUI.
    9421317740
  34. in src/wallet/rpcwallet.cpp:3078 in eb01ebd601 outdated 
    3073 | +    // Wallet::Verify will check if we're trying to create a wallet with a duplication name.
3074 | +    if (!CWallet::Verify(wallet_name, false, error, warning)) {
3075 | +        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error);
3076 | +    }
3077 | +
3078 | +    CWallet * const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()));
    promag commented at 3:24 PM on May 25, 2018: 
    std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(...);
    jnewbery commented at 4:12 PM on May 25, 2018:

    Thanks promag. Rebased and fixed

  35. jnewbery force-pushed on May 25, 2018
  36. laanwj added this to the "Blockers" column in a project
  37. in doc/release-notes-pr10740.md:7 in 8266c84615 outdated 
       6 | -Previously, wallets could only be loaded at startup, by specifying `-wallet` parameters on the command line or in the bitcoin.conf file. It is now possible to load wallets dynamically at runtime by calling the `loadwallet` RPC.
   7 | +Previously, wallets could only be loaded or created at startup, by specifying `-wallet` parameters on the command line or in the bitcoin.conf file. It is now possible to load and create wallets dynamically at runtime:
   8 |  
   9 | -The wallet can be specified as file/directory basename (which must be located in the `walletdir` directory), or as an absolute path to a file/directory.
  10 | +- Existing wallets can be loaded by calling the `loadwallet` RPC. The wallet can be specified as file/directory basename (which must be located in the `walletdir` directory), or as an absolute path to a file/directory.
  11 | +- New wallets can be created by calling the `createwallet` RPC. The provided name must not match a wallet file in the `walletdir` directory or the name of a wallet that is currently loaded.
    promag commented at 9:55 PM on May 30, 2018:

    nit, can be created (and loaded) by calling ...?

    jnewbery commented at 8:55 PM on May 31, 2018:

    Sure. Changed

  38. promag commented at 9:56 PM on May 30, 2018: member

    Tested ACK 8266c84.

  39. in src/wallet/rpcwallet.cpp:3132 in 8266c84615 outdated 
    3127 | +            "  \"name\" :    <wallet_name>,        (string) The wallet name if created successfully.\n"
3128 | +            "  \"warning\" : <warning>,            (string) Warning message if wallet was not loaded cleanly.\n"
3129 | +            "}\n"
3130 | +            "\nExamples:\n"
3131 | +            + HelpExampleCli("createwallet", "\"test.dat\"")
3132 | +            + HelpExampleRpc("createwallet", "\"test.dat\"")
    ken2812221 commented at 4:39 AM on May 31, 2018:

    Are these .dat suffixes neccessary? It would create a folder named test.dat on my Windows PC.

    jnewbery commented at 8:55 PM on May 31, 2018:

    You're right. I've changed these to testwallet

  40. fanquake deleted a comment on May 31, 2018
  41. in src/wallet/rpcwallet.cpp:3124 in 8266c84615 outdated 
    3119 | +    if (request.fHelp || request.params.size() != 1)
3120 | +        throw std::runtime_error(
3121 | +            "createwallet \"wallet_name\"\n"
3122 | +            "\nCreates and loads a new wallet.\n"
3123 | +            "\nArguments:\n"
3124 | +            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"
    MarcoFalke commented at 7:21 PM on May 31, 2018:

    It seems confusing to call this wallet_name and instead accept also accept a path. Something in the named arg that indicates that this is a file would help. Also please make sure the doc string matches the named arg and then test for the name of the argument in the functional tests.

    jnewbery commented at 9:11 PM on May 31, 2018:

    I can't think of a better argument name, so I've just updated the help text.

    jnewbery commented at 9:11 PM on May 31, 2018:

    Also added a test to create a wallet using a full path.

  42. in src/wallet/rpcwallet.cpp:3127 in 8266c84615 outdated 
    3122 | +            "\nCreates and loads a new wallet.\n"
3123 | +            "\nArguments:\n"
3124 | +            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"
3125 | +            "\nResult:\n"
3126 | +            "{\n"
3127 | +            "  \"name\" :    <wallet_name>,        (string) The wallet name if created successfully.\n"
    MarcoFalke commented at 7:21 PM on May 31, 2018:

    Same here

    jnewbery commented at 9:11 PM on May 31, 2018:

    help text updated.

  43. in src/wallet/rpcwallet.cpp:3119 in 8266c84615 outdated 
    3113 | @@ -3114,6 +3114,52 @@ UniValue loadwallet(const JSONRPCRequest& request)
3114 |      return obj;
3115 |  }
3116 |  
3117 | +UniValue createwallet(const JSONRPCRequest& request)
3118 | +{
3119 | +    if (request.fHelp || request.params.size() != 1)
    MarcoFalke commented at 7:23 PM on May 31, 2018:

    Style-nit: Curly braces for multi-line-then statements

    jnewbery commented at 9:04 PM on May 31, 2018:

    Changed.

  44. MarcoFalke commented at 7:23 PM on May 31, 2018: member

    some doc-nits (Can safely be ignored, I volunteer to fix them up post-merge)

  45. [wallet] [tests] Add tests for `createwallet` RPC. 32167e8300
  46. [wallets] [docs] Add release notes for createwallet RPC. f7e153e95f
  47. jnewbery force-pushed on May 31, 2018
  48. jnewbery commented at 9:11 PM on May 31, 2018: member

    Addressed all @promag and @MarcoFalke comments.

  49. jimpo commented at 9:27 PM on May 31, 2018: contributor

    utACK f7e153e

  50. promag commented at 10:52 PM on May 31, 2018: member

    utACK f7e153e.

  51. MarcoFalke commented at 10:56 PM on May 31, 2018: member

    utACK f7e153e95f

  52. ken2812221 commented at 5:55 AM on June 1, 2018: contributor

    Tested ACK f7e153e95f08e662427e8e4abcac8f53ab58281b

  53. jonasschnelli merged this on Jun 1, 2018
  54. jonasschnelli closed this on Jun 1, 2018
  55. jonasschnelli referenced this in commit 343d4e44ef on Jun 1, 2018
  56. fanquake removed this from the "Blockers" column in a project
  57. fanquake moved this from the "In progress" to the "Done" column in a project
  58. jasonbcox referenced this in commit 0f435ad7cd on Oct 11, 2019
  59. jonspock referenced this in commit 000b2467b1 on Feb 26, 2020
  60. jonspock referenced this in commit 505920e2a4 on Mar 2, 2020
  61. jonspock referenced this in commit b3e414d279 on Mar 7, 2020
  62. jonspock referenced this in commit 644fc85363 on Mar 14, 2020
  63. jonspock referenced this in commit 9b27fda833 on Mar 21, 2020
  64. jonspock referenced this in commit ba7b3b5583 on Mar 24, 2020
  65. jonspock referenced this in commit 6ebdca6325 on Apr 6, 2020
  66. jonspock referenced this in commit c120c56202 on Apr 8, 2020
  67. jonspock referenced this in commit d1da27a0d9 on Apr 8, 2020
  68. jonspock referenced this in commit dedd4621bc on Apr 8, 2020
  69. jonspock referenced this in commit 3d4b118251 on Apr 8, 2020
  70. jonspock referenced this in commit 6c733da039 on Apr 9, 2020
  71. jonspock referenced this in commit 283fe5d0f8 on Apr 17, 2020
  72. jonspock referenced this in commit 8b4e96096e on May 23, 2020
  73. jonspock referenced this in commit bdc134127e on May 25, 2020
  74. PastaPastaPasta referenced this in commit 11915948b2 on Jun 13, 2020
  75. jonspock referenced this in commit 26f2668d3e on Jul 9, 2020
  76. jonspock referenced this in commit 61e80759c6 on Jul 10, 2020
  77. jonspock referenced this in commit 6b7f264ea0 on Jul 17, 2020
  78. jonspock referenced this in commit af9d9651e2 on Jul 17, 2020
  79. jonspock referenced this in commit 707f9bf0d7 on Jul 20, 2020
  80. jonspock referenced this in commit f47d149824 on Jul 29, 2020
  81. jonspock referenced this in commit 546b58eb03 on Jul 31, 2020
  82. jonspock referenced this in commit ec7c86c38e on Aug 5, 2020
  83. jonspock referenced this in commit 22aa29f697 on Aug 6, 2020
  84. jonspock referenced this in commit ed2410e760 on Aug 7, 2020
  85. proteanx referenced this in commit 811ec4629c on Aug 8, 2020
  86. PastaPastaPasta referenced this in commit 858a64f23d on Dec 16, 2020
  87. PastaPastaPasta referenced this in commit c44501f4d0 on Dec 18, 2020
  88. MarcoFalke locked this on Sep 8, 2021
Contributors
jnewberyjonasschnellipromagken2812221MarcoFalkejimpo
Labels
Wallet

Milestone
0.17.0

Linked (view graph)
#13059 Dynamic wallet load / create / unload
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: 2026-04-21 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me