wallet: Improve wallet creation

fjahr commented at 5:35 pm on July 16, 2019: member

This is a follow-up PR to #16244

The following suggestions are included:

Usage of enum class (https://github.com/bitcoin/bitcoin/pull/16244#discussion_r296434142)
Placing out args at the end convention (https://github.com/bitcoin/bitcoin/pull/16244#discussion_r296434172)
Return error when passphrase would be ignored because of disabled private keys (including functional test) (https://github.com/bitcoin/bitcoin/pull/16244#pullrequestreview-252015195)
Make status return variable of CreateWallet (https://github.com/bitcoin/bitcoin/pull/16244#discussion_r302107394)
Using a switch statement instead of if/else in RpcWallet (https://github.com/bitcoin/bitcoin/pull/16244#discussion_r302112502)

Not included was:

“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)

For these last two changes, I was not sure what an ideal solution could look like and/or this might be of slightly larger scope than the other changes, but I would be happy to work on these as well in this PR or another follow-up if I get positive feedback on that. Is there a place in the codebase that handles flags like these in a better way that I can refer to? Nonetheless, I would prefer keeping it in a separate PR unless it is a really simple change.

MarcoFalke added the label Refactoring on Jul 16, 2019

MarcoFalke added the label Wallet on Jul 16, 2019

in src/wallet/rpcwallet.cpp:2699 in f00e07748a outdated

2702+            throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error);
2703+            break;
2704+        case WalletCreationStatus::SUCCESS:
2705+            break;
2706+        default:
2707+            throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed");

MarcoFalke commented at 6:05 pm on July 16, 2019:

style-nit: Could remove this (similar to how other places handle this)

0        // no default case, so the compiler can warn about missing cases

See e.g. https://github.com/bitcoin/bitcoin/blob/f00e07748a39ec09fc1b4090497ed39ceb0d4454/src/wallet/rpcwallet.cpp#L3496

fjahr commented at 10:06 pm on July 16, 2019:

Fixed, thanks!

in src/wallet/rpcwallet.cpp:2695 in f00e07748a outdated

2698+        case WalletCreationStatus::CREATION_FAILED:
2699+            throw JSONRPCError(RPC_WALLET_ERROR, error);
2700+            break;
2701+        case WalletCreationStatus::ENCRYPTION_FAILED:
2702+            throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error);
2703+            break;

MarcoFalke commented at 6:06 pm on July 16, 2019:

style-nit: throw should already break? If so, could remove this.

fjahr commented at 10:06 pm on July 16, 2019:

Fixed, thanks!

MarcoFalke commented at 6:07 pm on July 16, 2019: member

ACK, just some style nits. Feel free to ignore.

MarcoFalke removed the label Refactoring on Jul 16, 2019

MarcoFalke added the label RPC/REST/ZMQ on Jul 16, 2019

MarcoFalke commented at 6:10 pm on July 16, 2019: member

Looks like https://github.com/bitcoin/bitcoin/commit/b56a5b9e63b9aad9d3d2192384748a9d981594f1 and https://github.com/bitcoin/bitcoin/commit/b8e19ec09986cd244208da0909c5f0d1cc435f19 are intermixed and one of them won’t compile on its own. Might want to unmix them or squash them completely

DrahtBot commented at 6:42 pm on July 16, 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:

#16394 (Allow createwallet to take empty passwords to make unencrypted wallets 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.

Place out args at the end for CreateWallet 3199610ad3

Use strong enum for WalletCreationStatus d6649d16b5

fjahr force-pushed on Jul 16, 2019

fjahr commented at 10:09 pm on July 16, 2019: member

Looks like b56a5b9 and b8e19ec are intermixed and one of them won’t compile on its own. Might want to unmix them or squash them completely

Right, I untangled them. Thanks!

in src/wallet/rpcwallet.cpp:2694 in 07df41c56b outdated

2689-    if (status == WalletCreationStatus::CREATION_FAILED) {
2690-        throw JSONRPCError(RPC_WALLET_ERROR, error);
2691-    } else if (status == WalletCreationStatus::ENCRYPTION_FAILED) {
2692-        throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error);
2693-    } else if (status != WalletCreationStatus::SUCCESS) {
2694-        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed");

promag commented at 10:11 pm on July 16, 2019:

Should keep this by adding a default:?

fjahr commented at 7:05 pm on July 19, 2019:

Sjors suggestion was to go without a default so that the compiler will show a warning if we forget to handle any newly added options in the WalletCreationStatus enum. The same technique is used in the same file here: https://github.com/fjahr/bitcoin/blob/07df41c56bc3c0ccb4463083febf39fcad9749e2/src/wallet/rpcwallet.cpp#L3485

But I will add a comment, same as in the example mentioned above.

promag commented at 7:10 pm on July 19, 2019:

A warning can be unnoticed.

MarcoFalke commented at 7:12 pm on July 19, 2019:

Then, it should be made an error

MarcoFalke commented at 7:27 pm on July 19, 2019:

0$ ./configure --help|grep enable-werror
1  --enable-werror         Treat certain compiler warnings as errors (default
2$ git grep enable-werror .travis.yml
3.travis.yml:        BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror"

MarcoFalke commented at 7:43 pm on July 19, 2019:

@promag Here you go:

build: Treat -Wswitch as error when –enable-werror #16424

in src/wallet/wallet.cpp:194 in 07df41c56b outdated

194+        return WalletCreationStatus::CREATION_FAILED;
195     }
196 
197     // Make the wallet
198-    std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(chain, location, wallet_creation_flags);
199+    wallet = CWallet::CreateWalletFromFile(chain, location, wallet_creation_flags);

promag commented at 10:15 pm on July 16, 2019:

Should not set wallet yet otherwise it leaks to the caller if something below fails. My suggestion is to rename the argument to std::shared_ptr<CWallet>& result) and below do:

0    result = wallet;
1    return WalletCreationStatus::SUCCESS;
2}

fjahr commented at 7:06 pm on July 19, 2019:

I added this, thanks!

jnewbery commented at 5:49 pm on July 22, 2019:

Looks good. I don’t love the name return for the wallet argument (since it’s a pointer that’s passed in and then updated), but I’m not going to bikeshed this PR over argument naming.

promag commented at 10:19 pm on July 16, 2019: member

Beside the new test, is this really an improvement? Maybe a matter of taste but this change has 2 small issues detailed below.

Should squash IMO.

fanquake requested review from achow101 on Jul 18, 2019

fanquake requested review from meshcollider on Jul 18, 2019

in src/wallet/wallet.cpp:189 in 07df41c56b outdated

188+        return WalletCreationStatus::CREATION_FAILED;
189+    }
190+
191+    // Do not allow a passphrase when private keys are disabled
192+    if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
193+        error = "Passphrase provided but private keys are disabled.";

jnewbery commented at 3:47 pm on July 19, 2019:

Nit: It’s probably not immediately obvious to users why a passphrase is incompatible with a watchonly wallet. Perhaps this error message could be expanded to say:

“Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.”

fjahr commented at 7:04 pm on July 19, 2019:

Thanks, I updated this.

jnewbery commented at 4:09 pm on July 19, 2019: member

Looks good. One nit comment inline.

I think promag’s comment here: #16399 (review) needs to be addressed.

fjahr force-pushed on Jul 19, 2019

Return error for ignored passphrase through disable private keys option ba1f128d6c

Use switch on status in RpcWallet e967cae8fa

fjahr force-pushed on Jul 19, 2019

MarcoFalke added the label Waiting for author on Jul 19, 2019

MarcoFalke removed the label Waiting for author on Jul 19, 2019

jnewbery commented at 5:50 pm on July 22, 2019: member

Code review utACK e967cae8fac84ec7a89a3a853a83d8193ac3308e

meshcollider commented at 1:12 pm on July 29, 2019: contributor

Changes look good. I don’t think they all need to be in separate commits as mentioned above.

e967cae8fac84ec7a89a3a853a83d8193ac3308e

MarcoFalke commented at 1:30 pm on July 29, 2019: member

ACK e967cae8fa

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e967cae8fa
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjzJAwAxTTa/NB3hNVlCGvSunqZQanRkkmtbBBVm/x7+mDQ24QiAbMdkUrsWZBc
 8RJXSRgAbX80xgEKwYv/bMy1+5cSvmS4yNJ6htemoAjaW5Mnx5GXL4TrRMKIyUflz
 9evt1Xq2TTAjUsZIRzPqCZLToI+LlR0iYfWSReTxAK+ti3NJlRLT9N8VkgU+Pypyd
104zHTApJNF+QyemS/PzG4giJPF0qavFOtTlXvXdJKz8AMn0gWgUJD/cSIdWgSDxZ/
118AsOY26asUAgv6uYVwt1LXaR8mjcBlp3irSIYTr1IzDOWwqazPK7VqyQJpL6cmAR
12kqzTfCLsv2PfDHgnC9teXeMLSxGIANi3+SVlfZTGM6HzCFM/zj56eYUcUw8NvZaG
13l5JD50UIKNO9amalQPv//z7glVJcd7ke5BaEd/Of8XN71vEEqqb3oupDL2J2/Pm7
14u8TSPgb9jHlMAIxnXca4TPJw2HAphmKjw2FhzISy404YnyYicBYLE07Ip2V5fwog
15WXKjVw7n
16=uFz2
17-----END PGP SIGNATURE-----

Timestamp of file with hash 089c56726f27dc2b594e86a9a8f2a082b4a6a0e04063aa60feaac5685ac70944 -

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

MarcoFalke merged this on Jul 29, 2019

MarcoFalke closed this on Jul 29, 2019

in src/wallet/wallet.h:58 in e967cae8fa

55     CREATION_FAILED,
56     ENCRYPTION_FAILED
57 };
58 
59-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);
60+WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, std::shared_ptr<CWallet>& result);

promag commented at 1:44 pm on July 29, 2019:

nit, could have a comment saying that

result is set when the return value is SUCCESS
error is set otherwise
warning can be set in any case.

promag commented at 1:46 pm on July 29, 2019: member

ACK e967cae8fac84ec7a89a3a853a83d8193ac3308e, only reviewed code.

sidhujag referenced this in commit f795cbf921 on Jul 30, 2019

MarcoFalke locked this on Dec 16, 2021

wallet: Improve wallet creation #16399

Conflicts