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

jnewbery · 2018-04-23T17:24:11Z

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

Includes tests and release notes.

jnewbery · 2018-04-23T17:24:20Z

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

jonasschnelli · 2018-04-23T18:20:43Z

src/wallet/wallet.cpp

+    }
+
+    // Make sure that the wallet path doesn't clash with an existing wallet path
+    std::set<fs::path> wallet_paths;


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.

promag

Concept ACK. Needs rebase.

promag · 2018-04-29T10:41:37Z

src/wallet/rpcwallet.cpp

+    obj = wallet_name;
+
+    return obj;
+


Empty line.

jnewbery · 2018-05-16T21:56:26Z

rebased

promag

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

promag · 2018-05-17T15:58:33Z

src/wallet/rpcwallet.cpp

+    std::string error;
+
+    fs::path wallet_path = fs::absolute(wallet_name, GetWalletDir());
+    if (fs::symlink_status(wallet_path).type() != fs::file_not_found) {


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

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

promag · 2018-05-17T16:01:16Z

test/functional/wallet_multiwallet.py

+        self.nodes[0].createwallet('w9')
+
+        assert 'w9' in self.nodes[0].listwallets()
+


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

sure. Added

promag · 2018-05-17T16:02:00Z

test/functional/wallet_multiwallet.py

@@ -211,5 +211,15 @@ def run_test(self):
        # Fail to load if wallet file is a symlink
        assert_raises_rpc_error(-4, "Wallet file verification failed: Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink')

+        self.log.info("Test dynamic wallet creation.")
+


Could test invalid argument.

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

promag · 2018-05-17T16:02:23Z

src/wallet/rpcwallet.cpp

+            "\nArguments:\n"
+            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"
+            "\nResult:\n"
+            "<walletname>     (string) The wallet name if created successfully.\n"


Return same format as loadwallet?

promag · 2018-05-17T16:04:40Z

test/functional/wallet_multiwallet.py

+        assert_raises_rpc_error(-4, "Wallet w2 already exists.", self.nodes[0].createwallet, 'w2')
+
+        # Successfully create a wallet with a new name
+        self.nodes[0].createwallet('w9')


Assert result.

jnewbery · 2018-05-17T20:34:11Z

Addressed @promag comments.

promag · 2018-05-18T09:06:11Z

Needs rebase.

jonasschnelli · 2018-05-18T13:37:29Z

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.

jnewbery · 2018-05-21T17:52:07Z

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.

jonasschnelli · 2018-05-21T18:41:54Z

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.

jnewbery · 2018-05-21T19:12:16Z

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

jonasschnelli · 2018-05-21T19:15:43Z

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)

promag · 2018-05-21T19:18:18Z

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?

jnewbery · 2018-05-21T20:03:00Z

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.

promag · 2018-05-25T14:27:23Z

Needs rebase after #13063.

promag · 2018-05-25T14:27:52Z

src/wallet/rpcwallet.cpp

+    obj.pushKV("warning", warning);
+
+    return obj;
+


nit, remove empty line.

promag · 2018-05-25T14:31:41Z

src/wallet/rpcwallet.cpp

+            "createwallet \"filename\"\n"
+            "\nCreates and loads a new wallet.\n"
+            "\nArguments:\n"
+            "1. \"wallet_name\"    (string, required) The name for the new wallet.\n"


Here wallet_name, above filename 🙄

jnewbery · 2018-05-25T15:20:06Z

Addressed @promag's comments.

promag · 2018-05-25T15:24:14Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error);
+    }
+
+    CWallet * const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()));


std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(...);

Thanks promag. Rebased and fixed

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.

promag · 2018-05-30T21:55:49Z

doc/release-notes-pr10740.md


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


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

Sure. Changed

Summary: f7e153e95 [wallets] [docs] Add release notes for createwallet RPC. (John Newbery) 32167e830 [wallet] [tests] Add tests for `createwallet` RPC. (John Newbery) 942131774 [wallet] [rpc] Add `createwallet` RPC (John Newbery) Pull request description: Adds a `createwallet` RPC to dynamically create a new wallet at runtime. Includes tests and release notes. Tree-SHA512: e0d89e3ae498234e9db5b827c56804cbab64f18a1875e2b5e676172c110278ea1b9e93a8a61b8dd80e2f2a691490bf229e923e4ccb284a1d3e420b8317815866 Backport of Core PR13058 bitcoin/bitcoin#13058 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, fpelliccioni Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Subscribers: fpelliccioni Differential Revision: https://reviews.bitcoinabc.org/D4220

…at runtime f7e153e [wallets] [docs] Add release notes for createwallet RPC. (John Newbery) 32167e8 [wallet] [tests] Add tests for `createwallet` RPC. (John Newbery) 9421317 [wallet] [rpc] Add `createwallet` RPC (John Newbery) Pull request description: Adds a `createwallet` RPC to dynamically create a new wallet at runtime. Includes tests and release notes. Tree-SHA512: e0d89e3ae498234e9db5b827c56804cbab64f18a1875e2b5e676172c110278ea1b9e93a8a61b8dd80e2f2a691490bf229e923e4ccb284a1d3e420b8317815866

Summary: f7e153e95 [wallets] [docs] Add release notes for createwallet RPC. (John Newbery) 32167e830 [wallet] [tests] Add tests for `createwallet` RPC. (John Newbery) 942131774 [wallet] [rpc] Add `createwallet` RPC (John Newbery) Pull request description: Adds a `createwallet` RPC to dynamically create a new wallet at runtime. Includes tests and release notes. Tree-SHA512: e0d89e3ae498234e9db5b827c56804cbab64f18a1875e2b5e676172c110278ea1b9e93a8a61b8dd80e2f2a691490bf229e923e4ccb284a1d3e420b8317815866 Backport of Core PR13058 bitcoin/bitcoin#13058 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, fpelliccioni Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Subscribers: fpelliccioni Differential Revision: https://reviews.bitcoinabc.org/D4220

…at runtime f7e153e [wallets] [docs] Add release notes for createwallet RPC. (John Newbery) 32167e8 [wallet] [tests] Add tests for `createwallet` RPC. (John Newbery) 9421317 [wallet] [rpc] Add `createwallet` RPC (John Newbery) Pull request description: Adds a `createwallet` RPC to dynamically create a new wallet at runtime. Includes tests and release notes. Tree-SHA512: e0d89e3ae498234e9db5b827c56804cbab64f18a1875e2b5e676172c110278ea1b9e93a8a61b8dd80e2f2a691490bf229e923e4ccb284a1d3e420b8317815866

jnewbery added the Wallet label Apr 23, 2018

jnewbery mentioned this pull request Apr 23, 2018

Dynamic wallet load / create / unload #13059

Closed

10 tasks

jonasschnelli reviewed Apr 23, 2018

View reviewed changes

promag reviewed Apr 29, 2018

View reviewed changes

src/wallet/rpcwallet.cpp Outdated

obj = wallet_name;

return obj;

Copy link

Contributor

promag Apr 29, 2018

Choose a reason for hiding this comment

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

Empty line.

jnewbery force-pushed the create_wallet branch from f8b7016 to 6e6c113 Compare May 16, 2018 21:56

promag reviewed May 17, 2018

View reviewed changes

jnewbery force-pushed the create_wallet branch from 6e6c113 to 53e71dc Compare May 17, 2018 20:33

jnewbery force-pushed the create_wallet branch from 53e71dc to e058f0d Compare May 21, 2018 15:04

jonasschnelli added this to the 0.17.0 milestone May 25, 2018

promag reviewed May 25, 2018

View reviewed changes

jnewbery force-pushed the create_wallet branch from e058f0d to eb01ebd Compare May 25, 2018 15:19

promag reviewed May 25, 2018

View reviewed changes

[wallet] [rpc] Add createwallet RPC

9421317

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.

jnewbery force-pushed the create_wallet branch from eb01ebd to 8266c84 Compare May 25, 2018 16:12

promag reviewed May 30, 2018

View reviewed changes

jsarenik mentioned this pull request Oct 1, 2020

pyln: Add a fixture for createwallet ElementsProject/lightning#4088

Merged

PastaPastaPasta mentioned this pull request Oct 5, 2020

rpc call to create a wallet? dashpay/dash#3760

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		self.nodes[0].createwallet('w9')

		assert 'w9' in self.nodes[0].listwallets()

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

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

Uh oh!

Conversation

jnewbery commented Apr 23, 2018

Uh oh!

jnewbery commented Apr 23, 2018 • edited by jonasschnelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 16, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 17, 2018

Uh oh!

promag commented May 18, 2018

Uh oh!

jonasschnelli commented May 18, 2018

Uh oh!

jnewbery commented May 21, 2018

Uh oh!

jonasschnelli commented May 21, 2018

Uh oh!

jnewbery commented May 21, 2018

Uh oh!

jonasschnelli commented May 21, 2018

Uh oh!

promag commented May 21, 2018

Uh oh!

jnewbery commented May 21, 2018

Uh oh!

promag commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

jnewbery commented Apr 23, 2018 •

edited by jonasschnelli

Loading

promag left a comment •

edited

Loading