Skip to content

Allow createwallet to take empty passwords to make unencrypted wallets #16394

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
Aug 1, 2019
Merged

Allow createwallet to take empty passwords to make unencrypted wallets #16394

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-16394.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
RPC changes
-----------
`createwallet` now returns a warning if an empty string is used as an encryption password, and does not encrypt the wallet, instead of raising an error.
This makes it easier to disable encryption but also specify other options when using the `bitcoin-cli` tool.
15 changes: 11 additions & 4 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2676,11 +2676,12 @@ static UniValue createwallet(const JSONRPCRequest& request)
}
SecureString passphrase;
passphrase.reserve(100);
std::string warning;
if (!request.params[3].isNull()) {
passphrase = request.params[3].get_str().c_str();
if (passphrase.empty()) {
// Empty string is invalid
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Cannot encrypt a wallet with a blank password");
// Empty string means unencrypted
warning = "Empty string given as passphrase, wallet will not be encrypted.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't happen at the moment but in the future this warning could be cleared/overwritten after CreateWallet(...). Maybe the response should have "warnings": [...].

Copy link
Member Author

Choose a reason for hiding this comment

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

The only warnings come from wallets that already exist, so this shouldn't effect CreateWallet. I guess if such a warning is added that would be part of CreateWallet we could change it them. However it would be a breaking change to the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it's still possible with concurrent createwallets. You could have 2 std::string warn1, warn2; and at the end join them 🙉

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with promag that we shouldn't just throw away the first warning string. If you're worried about breaking the API, then joining the strings seems like good solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Joined them

}
}

Expand All @@ -2689,9 +2690,9 @@ static UniValue createwallet(const JSONRPCRequest& request)
}

std::string error;
std::string warning;
std::string create_warning;
std::shared_ptr<CWallet> wallet;
WalletCreationStatus status = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, warning, wallet);
WalletCreationStatus status = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, create_warning, wallet);
switch (status) {
case WalletCreationStatus::CREATION_FAILED:
throw JSONRPCError(RPC_WALLET_ERROR, error);
Expand All @@ -2702,6 +2703,12 @@ static UniValue createwallet(const JSONRPCRequest& request)
// no default case, so the compiler can warn about missing cases
}

if (warning.empty()) {
warning = create_warning;
} else if (!warning.empty() && !create_warning.empty()){
warning += "; " + create_warning;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would probably be better to join with newlines to increase visibility or the create warning and avoid ambiguity in case a current or future warning includes a semicolon. Also I think we join warning and error strings elsewhere with newlines, so newlines here would be more consistent.

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 feel like changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I suggested "join them" I also thought of multiple lines.

}

UniValue obj(UniValue::VOBJ);
obj.pushKV("name", wallet->GetName());
obj.pushKV("warning", warning);
Expand Down
15 changes: 12 additions & 3 deletions test/functional/wallet_createwallet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,20 @@ def run_test(self):
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, '')
# Allow empty passphrase, but there should be a warning
resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='')
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use named arguments or also use positional arguments - to cover the fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test for it.

assert_equal(resp['warning'], 'Empty string given as passphrase, wallet will not be encrypted.')
w7 = node.get_wallet_rpc('w7')
assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 10)

self.log.info('Test making a wallet with avoid reuse flag')
self.nodes[0].createwallet('w8', False, False, '', True) # Use positional arguments to check for bug where avoid_reuse could not be set for wallets without needing them to be encrypted
w8 = node.get_wallet_rpc('w8')
assert_raises_rpc_error(-15, 'Error: running with an unencrypted wallet, but walletpassphrase was called.', w7.walletpassphrase, '', 10)
assert_equal(w8.getwalletinfo()["avoid_reuse"], True)

self.log.info('Using a passphrase with private keys disabled returns error')
assert_raises_rpc_error(-4, '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.', self.nodes[0].createwallet, wallet_name='w8', disable_private_keys=True, passphrase='thisisapassphrase')
assert_raises_rpc_error(-4, '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.', self.nodes[0].createwallet, wallet_name='w9', disable_private_keys=True, passphrase='thisisapassphrase')

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