Skip to content

Add option to create an encrypted wallet #15006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 16, 2019
Merged

Add option to create an encrypted wallet #15006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/release-notes-15006.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Miscellaneous RPC changes
------------

- `createwallet` can now create encrypted wallets if a non-empty passphrase is specified.
71 changes: 55 additions & 16 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2641,26 +2641,29 @@ static UniValue loadwallet(const JSONRPCRequest& request)

static UniValue createwallet(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 3) {
throw std::runtime_error(
RPCHelpMan{"createwallet",
"\nCreates and loads a new wallet.\n",
{
{"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name for the new wallet. If this is a path, the wallet will be created at the path location."},
{"disable_private_keys", RPCArg::Type::BOOL, /* default */ "false", "Disable the possibility of private keys (only watchonlys are possible in this mode)."},
{"blank", RPCArg::Type::BOOL, /* default */ "false", "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed."},
},
RPCResult{
const RPCHelpMan help{
"createwallet",
"\nCreates and loads a new wallet.\n",
{
{"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name for the new wallet. If this is a path, the wallet will be created at the path location."},
{"disable_private_keys", RPCArg::Type::BOOL, /* default */ "false", "Disable the possibility of private keys (only watchonlys are possible in this mode)."},
{"blank", RPCArg::Type::BOOL, /* default */ "false", "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed."},
{"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Encrypt the wallet with this passphrase."},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this could help text could be improved by adding a second sentence: "If no passphrase is provided, the wallet will not be encrypted on creation."

},
RPCResult{
"{\n"
" \"name\" : <wallet_name>, (string) The wallet name if created successfully. If the wallet was created using a full path, the wallet_name will be the full path.\n"
" \"warning\" : <warning>, (string) Warning message if wallet was not loaded cleanly.\n"
"}\n"
},
RPCExamples{
HelpExampleCli("createwallet", "\"testwallet\"")
},
RPCExamples{
HelpExampleCli("createwallet", "\"testwallet\"")
+ HelpExampleRpc("createwallet", "\"testwallet\"")
},
}.ToString());
},
};

if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
throw std::runtime_error(help.ToString());
}
std::string error;
std::string warning;
Expand All @@ -2670,7 +2673,20 @@ static UniValue createwallet(const JSONRPCRequest& request)
flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
}

bool create_blank = false; // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted
if (!request.params[2].isNull() && request.params[2].get_bool()) {
create_blank = true;
flags |= WALLET_FLAG_BLANK_WALLET;
}
SecureString passphrase;
Copy link
Member

Choose a reason for hiding this comment

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

nit: move to a shared function so it's more clear this is identical to encryptwallet

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not identical. There isn't enough shared here to warrant that IMO

passphrase.reserve(100);
if (!request.params[3].isNull()) {
passphrase = request.params[3].get_str().c_str();
if (passphrase.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IDK if it should trim?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so.

// Empty string is invalid
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Cannot encrypt a wallet with a blank password");
}
// Born encrypted wallets need to be blank first so that wallet creation doesn't make any unencrypted keys
flags |= WALLET_FLAG_BLANK_WALLET;
}

Expand All @@ -2688,6 +2704,29 @@ static UniValue createwallet(const JSONRPCRequest& request)
if (!wallet) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed.");
}

// Encrypt the wallet if there's a passphrase
if (!passphrase.empty() && !(flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It'd be a better user experience if we error'ed out when the user provides disable_private_keys=true and passphrase=something. If they provide that input, they're expecting the wallet to be encrypted somehow. Better to error out and say "wallets without private keys cannot be encrypted" than just swallow the passphrase silently.

if (!wallet->EncryptWallet(passphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Wallet created but failed to encrypt.");
}

if (!create_blank) {
// Unlock the wallet
if (!wallet->Unlock(passphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Wallet was encrypted but could not be unlocked");
}

// Set a seed for the wallet
CPubKey master_pub_key = wallet->GenerateNewSeed();
wallet->SetHDSeed(master_pub_key);
wallet->NewKeyPool();

// Relock the wallet
wallet->Lock();
}
}

AddWallet(wallet);

wallet->postInitProcess();
Expand Down Expand Up @@ -4140,7 +4179,7 @@ static const CRPCCommand commands[] =
{ "wallet", "addmultisigaddress", &addmultisigaddress, {"nrequired","keys","label","address_type"} },
{ "wallet", "backupwallet", &backupwallet, {"destination"} },
{ "wallet", "bumpfee", &bumpfee, {"txid", "options"} },
{ "wallet", "createwallet", &createwallet, {"wallet_name", "disable_private_keys", "blank"} },
{ "wallet", "createwallet", &createwallet, {"wallet_name", "disable_private_keys", "blank", "passphrase"} },
{ "wallet", "dumpprivkey", &dumpprivkey, {"address"} },
{ "wallet", "dumpwallet", &dumpwallet, {"filename"} },
{ "wallet", "encryptwallet", &encryptwallet, {"passphrase"} },
Expand Down
23 changes: 23 additions & 0 deletions test/functional/wallet_createwallet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,28 @@ def run_test(self):
assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w5.getnewaddress)
assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w5.getrawchangeaddress)

self.log.info('New blank and encrypted wallets can be created')
self.nodes[0].createwallet(wallet_name='wblank', disable_private_keys=False, blank=True, passphrase='thisisapassphrase')
wblank = node.get_wallet_rpc('wblank')
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", wblank.signmessage, "needanargument", "test")
Copy link
Contributor

Choose a reason for hiding this comment

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

micronit: I always prefer named arguments in tests, so there's no ambiguity about what the RPC method call should be. In that case it'd be ...wblank.signmessage, address="fakeaddress", message="test" here.

EDIT: assert_raises_rpc_error() itself takes an argument called message, so this doesn't work. We should fix that (in a different PR) by making assert_raises_rpc_error's argument something like _error_message so it doesn't collide with any RPC arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Or use a lambda to bind the named args?

wblank.walletpassphrase('thisisapassphrase', 10)
assert_raises_rpc_error(-4, "Error: This wallet has no available keys", wblank.getnewaddress)
assert_raises_rpc_error(-4, "Error: This wallet has no available keys", wblank.getrawchangeaddress)

self.log.info('Test creating a new encrypted wallet.')
# Born encrypted wallet is created (has keys)
self.nodes[0].createwallet(wallet_name='w6', disable_private_keys=False, blank=False, passphrase='thisisapassphrase')
w6 = node.get_wallet_rpc('w6')
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", w6.signmessage, "needanargument", "test")
w6.walletpassphrase('thisisapassphrase', 10)
w6.signmessage(w6.getnewaddress('', 'legacy'), "test")
w6.keypoolrefill(1)
# There should only be 1 key
walletinfo = w6.getwalletinfo()
assert_equal(walletinfo['keypoolsize'], 1)
assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
# Empty passphrase, error
assert_raises_rpc_error(-16, 'Cannot encrypt a wallet with a blank password', self.nodes[0].createwallet, 'w7', False, False, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

micronit: again, I'd prefer named arguments here (and dropping the unneeded optional arguments):

assert_raises_rpc_error(-16, 'Cannot encrypt a wallet with a blank password', self.nodes[0].createwallet, wallet_name='w7', passphrase='')


if __name__ == '__main__':
CreateWalletTest().main()