Skip to content
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.

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

Add option to create an encrypted wallet #15006

Merged
merged 1 commit into from May 16, 2019

Conversation

@achow101
Copy link
Member

commented Dec 19, 2018

This PR adds a new passphrase argument to createwallet which will create a wallet that is encrypted with that passphrase.

This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.

@fanquake fanquake added the Wallet label Dec 19, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15937 (WIP: Add loadwallet and createwallet load_on_startup options by ryanofsky)
  • #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Concept ACK

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch Dec 20, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Concept ACK.

Today on IRC:
schermafbeelding 2018-12-20 om 11 17 16

@bitcoin bitcoin deleted a comment from achow101 Dec 20, 2018

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch Dec 20, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Concept ACK

test/functional/wallet_createwallet.py Outdated
self.log.info("Test disableprivatekeys creation.")
self.nodes[0].createwallet('w1', True)
w1 = node.get_wallet_rpc('w1')
assert_raises_rpc_error(-4,"Error: Private keys are disabled for this wallet", w1.getnewaddress)

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 21, 2018

Member

Missing whitespace after ,. Please run this newly added file through black to make sure formatting is correct :-)

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 21, 2018

Member

@practicalswift that's an upstream test from #14938, where this has been fixed.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

This fails travis:

Assertion failed: lock cs_wallet not held in wallet/wallet.cpp:1389; locks held:
FAIL qt/test/test_bitcoin-qt (exit status: 134)

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch 2 times, most recently Jan 21, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

Fixed by rebasing

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch Jan 22, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

I've decided to base this on #15226 instead of #14938.

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch 2 times, most recently to b7918ad Jan 22, 2019

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch from b7918ad to 51a477f Jan 29, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Rebased

@DrahtBot DrahtBot removed the Needs rebase label Jan 29, 2019

@@ -488,6 +488,10 @@ class WalletImpl : public Wallet
{
return MakeHandler(m_wallet.NotifyWatchonlyChanged.connect(fn));
}
std::unique_ptr<Handler> handleCanGetAddressesChanged(CanGetAddressesChangedFn fn) override

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 31, 2019

Member

handleCanGetAddressesChanged is a terrible method name in itself, took me at least 5 minutes to decode it, please add a comment saying it's a handler for changes in whether addresses can be generated or not :-)

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

That's a change from #15225 and out of scope for this pr. sorry.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 31, 2019

Member

Fwiw I find it easy enough to read, but I'm used to similar nomenclature from iOs :-)

@@ -434,7 +434,7 @@ def batch(self, requests):
def send_cli(self, command=None, *args, **kwargs):
"""Run bitcoin-cli command. Deserializes returned string as python object."""
pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args]
named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()]
named_args = [str(key) + "=" + (str(value).lower() if type(value) is bool else str(value)) for (key, value) in kwargs.items()]

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 31, 2019

Member

I remember I had a comment on this (please factor out the value→cli functionality to a function instead of duplicating it) but somehow it got lost …

This comment has been minimized.

Copy link
@achow101

achow101 Jan 31, 2019

Author Member

Since this issue affects multiple PRs, I've split the fix into a separate PR: #15301

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 31, 2019

Member

hmmm, bizarre. You definitely did have a comment here because I responded to it!

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch from 51a477f to f2bed74 Jan 31, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Jan 31, 2019

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch from f2bed74 to 5e3d2b9 Feb 1, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

Concept ACK on that change, but you broke wallet_createwallet.py :-)

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch from df390e5 to 2fb944a Feb 16, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2019

Should be fixed now.

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

tACK 2fb944a

I'm tempted to make passphrase the second argument, although that breaks backwards compatibility, so users can more easily ignore the more advanced disable_private_keys and blank arguments.

Maybe we should just start making the RPC examples use named arguments.

@laanwj laanwj added this to Blockers in High-priority for review Mar 14, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK 2fb944a. This is great. I love how the blank wallet support makes this change very simple.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

I think it would be safer raise an error if the passphrase is non-null and empty instead of pretending the passphrase wasn't provided. I'm imagining a buggy shell script with bitcoin-cli createwallet "$NAME" false false "$PASS" that forgot to set the PASS variable or spelled it differently. Better to fail in this case than go on like nothing's wrong.

This comment has been minimized.

Copy link
@achow101

achow101 Mar 19, 2019

Author Member

Done

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch 3 times, most recently from e75ac27 to 341a142 Mar 19, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK 341a142. Only change since last review is throwing an error if the passphrase is empty. (Thanks!)

Show resolved Hide resolved src/wallet/rpcwallet.cpp
Show resolved Hide resolved src/wallet/rpcwallet.cpp
Show resolved Hide resolved doc/release-notes-15006.md Outdated
passphrase.reserve(100);
if (!request.params[3].isNull()) {
passphrase = request.params[3].get_str().c_str();
if (passphrase.empty()) {

This comment has been minimized.

Copy link
@promag

promag Apr 30, 2019

Member

IDK if it should trim?

This comment has been minimized.

Copy link
@achow101

achow101 May 14, 2019

Author Member

I don't think so.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2019

Add loadwallet and createwallet RPC load_on_startup options
This maintains a persistent list of wallets stored in settings, that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.
@jnewbery
Copy link
Member

left a comment

A few comments inline.

@promag's comment here: https://github.com/bitcoin/bitcoin/pull/15006/files#r275171693 still needs to be addressed

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp
Show resolved Hide resolved test/functional/wallet_createwallet.py
Show resolved Hide resolved test/functional/wallet_createwallet.py Outdated
Show resolved Hide resolved test/functional/wallet_createwallet.py Outdated
Show resolved Hide resolved test/functional/wallet_createwallet.py

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 3, 2019

Add loadwallet and createwallet RPC load_on_startup options
This maintains a persistent list of wallets stored in settings, that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 7, 2019

Add loadwallet and createwallet RPC load_on_startup options
This maintains a persistent list of wallets stored in settings, that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 8, 2019

Add loadwallet and createwallet RPC load_on_startup options
This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.
@sipa
Copy link
Member

left a comment

Concept ACK

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@achow101 achow101 force-pushed the achow101:create-encrypted-wallet branch from 341a142 to 662d117 May 14, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Addressed comments. Also rebased so that IsValidNumArgs is available.

@jnewbery
Copy link
Member

left a comment

Looks great. utACK 662d117

I've left a few nits. Feel free to take or leave.

{"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."},

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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

@@ -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)) {

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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.

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 16, 2019

Member

Or use a lambda to bind the named args?

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, '')

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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='')
@laanwj

This comment has been minimized.

Copy link
Member

commented May 16, 2019

utACK 662d117
Going to merge this, I think the micro-nits can be addressed later and don't need to hold thi sup.

@laanwj laanwj merged commit 662d117 into bitcoin:master May 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 16, 2019

Merge #15006: Add option to create an encrypted wallet
662d117 Add option to create an encrypted wallet (Andrew Chow)

Pull request description:

  This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase.

  This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.

ACKs for commit 662d11:
  laanwj:
    utACK 662d117
  jnewbery:
    Looks great. utACK 662d117

Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a

@laanwj laanwj removed this from Blockers in High-priority for review May 16, 2019

@LitecoinZ LitecoinZ referenced this pull request May 31, 2019

Open

Backports from upstream #1

44 of 244 tasks complete

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 8, 2019

Add loadwallet and createwallet RPC load_on_startup options
This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.