Move wallet creation out of the createwallet rpc into its own function

achow101 commented at 9:58 pm on June 19, 2019: member

Moves the wallet creation logic from within the createwallet rpc and into its own function within wallet.cpp.

DrahtBot added the label RPC/REST/ZMQ on Jun 19, 2019

DrahtBot added the label Wallet on Jun 19, 2019

ryanofsky approved

ryanofsky commented at 11:10 pm on June 19, 2019: member

utACK e50573eb097660dc9cd8800990b3f93bdb8f6dd7. I reviewed moved code to verify behavior shouldn’t be changing. I do think the new create wallet function would be a little cleaner if it took separate option arguments instead of wallet flags, so flags would be initialized all in one place instead of across two functions, and the caller wouldn’t have to be aware of them.

Also, but not directly related to this PR, I think the createwallet RPC interface could be improved:

I think blank wallet and disable private keys options could be combined into a single, more self-explanatory “generate keys” option with values “immediately”, “on-demand”, and “disabled”.
It would be good to at least return a warning (if not an error) if an ignored passphrase is provided with disable private keys option.

promag commented at 11:37 pm on June 19, 2019: member

Concept ACK.

I think blank wallet and disable private keys options could be combined into a single, more self-explanatory “generate keys” option with values “immediately”, “on-demand”, and “disabled”. @ryanofsky do you suggest the same for the GUI? I think we could keep both interfaces consistent.

DrahtBot commented at 0:47 am on June 20, 2019: 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:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

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.

practicalswift commented at 7:45 am on June 20, 2019: contributor

This looks like a bug? :-)

https://github.com/bitcoin/bitcoin/blob/e50573eb097660dc9cd8800990b3f93bdb8f6dd7/src/wallet/rpcwallet.cpp#L2786-L2789

achow101 force-pushed on Jun 20, 2019

achow101 commented at 3:06 pm on June 20, 2019: member

@practicalswift Fixed.

Also, but not directly related to this PR, I think the createwallet RPC interface could be improved:

I think that can be done in a followup PR.

in src/wallet/rpcwallet.cpp:2704 in 6551c47df9 outdated

2791-    }
2792-
2793-    std::shared_ptr<CWallet> const wallet = CWallet::CreateWalletFromFile(*g_rpc_interfaces->chain, location, flags);
2794+    std::string error;
2795+    std::string warning;
2796+    WalletCreationStatus status;

promag commented at 10:42 pm on June 20, 2019:

Maybe pack these like:

0struct WalletCreationResult
1{
2    WalletCreationStatus status;
3    std::string error;
4    std::string warning;
5    std::shared_ptr<CWallet> wallet;
6}

achow101 commented at 5:59 pm on June 21, 2019:

I don’t think that is necessary.

in src/wallet/rpcwallet.cpp:2790 in 6551c47df9 outdated

2820-            wallet->Lock();
2821+        if (status == WalletCreationStatus::CREATION_FAILED) {
2822+            throw JSONRPCError(RPC_WALLET_ERROR, error);
2823+        } else if (status == WalletCreationStatus::ENCRYPTION_FAILED) {
2824+            throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error);
2825+        } else {

promag commented at 10:43 pm on June 20, 2019:

👀 what else?

promag commented at 10:44 pm on June 20, 2019:

Maybe else if (status != WalletCreationStatus::SUCCESS) { and ditch if (!wallet) { in L2785.

achow101 commented at 6:11 pm on June 21, 2019:

Done

achow101 force-pushed on Jun 21, 2019

ryanofsky approved

ryanofsky commented at 6:32 pm on June 21, 2019: member

utACK 49e47c019b63ea06fd2265ad16fa3245bf2093a3. Only changes since last review are cleaning up error code handling (credit to @practicalswift for spotting a bug there #16244 (comment))

re: #16244 (comment)

@ryanofsky do you suggest the same for the GUI? I think we could keep both interfaces consistent.

Unless there’s a common use-case for or one or both of these options, these seem like advanced settings that would only create confusion in the GUI. But I agree that if these are exposed one way or another in the GUI, the RPC should be updated to be consistent.

in src/wallet/wallet.h:52 in 49e47c019b outdated

48@@ -49,6 +49,14 @@ std::vector<std::shared_ptr<CWallet>> GetWallets();
49 std::shared_ptr<CWallet> GetWallet(const std::string& name);
50 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
51 
52+enum WalletCreationStatus {

Empact commented at 4:50 am on June 22, 2019:

enum class

achow101 commented at 2:56 pm on June 25, 2019:

I think I’ll leave it as is to avoid invalidating existing ACKs.

in src/wallet/wallet.h:58 in 49e47c019b outdated

53+    SUCCESS,
54+    CREATION_FAILED,
55+    ENCRYPTION_FAILED
56+};
57+
58+std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status, const SecureString& passphrase, uint64_t wallet_creation_flags);

Empact commented at 4:51 am on June 22, 2019:

nit: out args at the end is a nice convention IMO

achow101 commented at 2:56 pm on June 25, 2019:

I think I’ll leave it as is to avoid invalidating existing ACKs.

meshcollider commented at 10:16 am on June 22, 2019: contributor

Code-review utACK https://github.com/bitcoin/bitcoin/pull/16244/commits/49e47c019b63ea06fd2265ad16fa3245bf2093a3

Agree with Russ’s comments too.

jnewbery commented at 1:53 pm on July 9, 2019: member

utACK 49e47c019b63ea06fd2265ad16fa3245bf2093a3

This is a nice, tidy refactor PR that simplifies #15450.

There are some excellent follow-up suggestions in this PR:

#16244 (review)
#16244 (review)
“new create wallet function [could take] separate option arguments instead of wallet flags (https://github.com/bitcoin/bitcoin/pull/16244#pullrequestreview-252015195)
“blank wallet and disable private keys options could be combined into a single option” (https://github.com/bitcoin/bitcoin/pull/16244#pullrequestreview-252015195)
return a warning (if not an error) if an ignored passphrase is provided with disable private keys option (https://github.com/bitcoin/bitcoin/pull/16244#pullrequestreview-252015195) @achow101 , @Empact, @ryanofsky : I’ll happily review a PR that does those things if any of you want to open it.

DrahtBot added the label Needs rebase on Jul 9, 2019

Move wallet creation out of the createwallet rpc into its own function 1aecdf2063

achow101 commented at 11:50 pm on July 9, 2019: member

Rebased

achow101 force-pushed on Jul 9, 2019

jnewbery commented at 8:55 am on July 10, 2019: member

ACK 1aecdf2063cbe28d4715ae5ae1a7e51b860c9f4d

Ran the rebase myself and verified our merge resolves were the same.

DrahtBot removed the label Needs rebase on Jul 10, 2019

in src/wallet/rpcwallet.cpp:2705 in 1aecdf2063

2737-            wallet->Lock();
2738-        }
2739+    std::string error;
2740+    std::string warning;
2741+    WalletCreationStatus status;
2742+    std::shared_ptr<CWallet> wallet = CreateWallet(*g_rpc_interfaces->chain, request.params[0].get_str(), error, warning, status, passphrase, flags);

Sjors commented at 2:49 pm on July 10, 2019:

Light preference for making status the return variable, similar to how the TransactionStatus enum is used.

achow101 commented at 3:46 pm on July 10, 2019:

Maybe for a follow up. Trying to avoid invalidating ACKs.

in src/wallet/rpcwallet.cpp:2706 in 1aecdf2063

2738-        }
2739+    std::string error;
2740+    std::string warning;
2741+    WalletCreationStatus status;
2742+    std::shared_ptr<CWallet> wallet = CreateWallet(*g_rpc_interfaces->chain, request.params[0].get_str(), error, warning, status, passphrase, flags);
2743+    if (status == WalletCreationStatus::CREATION_FAILED) {

Sjors commented at 2:57 pm on July 10, 2019:

Use switch without default so we don’t forget anything?

achow101 commented at 3:46 pm on July 10, 2019:

Maybe for a follow up. Trying to avoid invalidating ACKs.

Sjors approved

Sjors commented at 3:03 pm on July 10, 2019: member

ACK 1aecdf2 with some suggestions for followup.

MarcoFalke commented at 5:51 pm on July 10, 2019: member

ACK 1aecdf2063cbe28d4715ae5ae1a7e51b860c9f4d

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 1aecdf2063cbe28d4715ae5ae1a7e51b860c9f4d
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhv2Qv+Pql8nxt70ITGbhfE0oD5zRZnD7wBrj50x1Ax+akPccN7i0kYvPitddVR
 8YAT2ybxKuSNymCMDO97KkHHYGV15bePInlJchIvZFLOW04Gi2KVSgC6+OUkwACVA
 9pPBSdsIUAkc3YnQ+itQYhvLFhCEAaherswBj2RYmHR35sDSjkBwUKcTZLCs14BsG
10ml3eLUJlGGW2Rv9Tyy0M+/bCpwc3mBnNPRT8KB5eNfR/L2lRSbkAkCB36p1vRpYt
11ctquEsHXPpgVBHFtxl+4NqQteB+nE0A/0ewHZ94ARag5+l9mHvJLmms/1fpWy3WC
12eEZ410b0M8lBtPTmgLECeLwKo6LXJsgpNLfhjtLRDmadyNrBmbFabh66uCT5qc3f
131xhRiFZSbMi6QomAImP1FqrPEGAr7Uic8/yEnCGOgw9kiU52TugI9meFwG0LZEAr
14620FcBTW+SBGzIQC4AP4MiATAjg9Lauhdx/cZ1UBKkek2jY0sG8yEOLX7hRVNfju
15S/4xqbWn
16=CqUZ
17-----END PGP SIGNATURE-----

Timestamp of file with hash 68c42fd65f782cac7ed08b8a0c706ca6a49acecbeb4fbda853ab984f9e459a8f -

MarcoFalke merged this on Jul 10, 2019

MarcoFalke closed this on Jul 10, 2019

MarcoFalke referenced this in commit 4fcccdac78 on Jul 10, 2019

MarcoFalke referenced this in commit 74ea1f3b0f on Jul 29, 2019

sidhujag referenced this in commit f795cbf921 on Jul 30, 2019

deadalnix referenced this in commit 49478a3292 on Jun 7, 2020

ShengguangXiao referenced this in commit 20a50723dc on Aug 28, 2020

monstrobishi referenced this in commit 0bd95fdfab on Sep 6, 2020

vijaydasmp referenced this in commit aea6f7ee7f on Dec 7, 2021

vijaydasmp referenced this in commit 87cbdca71d on Dec 10, 2021

MarcoFalke locked this on Dec 16, 2021

Move wallet creation out of the createwallet rpc into its own function #16244

Conflicts