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

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:fix-born-enc changing 3 files +27 −7

achow101 commented at 7:36 PM on July 15, 2019: member

Allow createwallet to take the empty string as a password and interpret that as leaving the wallet unencrypted. Also warn when that happens.

This fixes a bug where it was not possible to use the avoid_reuse option for new unencrypted wallets without using named arguments.Thus this allows more createwallet options to be added that can be set on unencrypted wallets when using positional arguments.
achow101 force-pushed on Jul 15, 2019
achow101 force-pushed on Jul 15, 2019
fanquake added the label RPC/REST/ZMQ on Jul 15, 2019
fanquake added the label Wallet on Jul 15, 2019
in src/wallet/rpcwallet.cpp:2684 in 8d0b9406f3 outdated
```
2683 |          passphrase = request.params[3].get_str().c_str();
2684 |          if (passphrase.empty()) {
2685 | -            // Empty string is invalid
2686 | -            throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Cannot encrypt a wallet with a blank password");
2687 | +            // Empty string means unencrypted
2688 | +            warning = "Empty string given as passphrase, wallet will not be encrypted.";
```
promag commented at 12:15 AM on July 16, 2019:

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

achow101 commented at 3:29 PM on July 17, 2019:

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.

promag commented at 3:35 PM on July 17, 2019:

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

jnewbery commented at 4:08 PM on July 26, 2019:

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.

achow101 commented at 3:50 PM on July 29, 2019:

Joined them

in test/functional/wallet_createwallet.py:120 in 8d0b9406f3 outdated

 115 | @@ -116,8 +116,11 @@ def run_test(self):
 116 |          walletinfo = w6.getwalletinfo()
 117 |          assert_equal(walletinfo['keypoolsize'], 1)
 118 |          assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
 119 | -        # Empty passphrase, error
 120 | -        assert_raises_rpc_error(-16, 'Cannot encrypt a wallet with a blank password', self.nodes[0].createwallet, 'w7', False, False, '')
 121 | +        # Allow empty passphrase, but there should be a warning
 122 | +        resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='')

promag commented at 12:17 AM on July 16, 2019:

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

achow101 commented at 3:27 PM on July 17, 2019:

Added a test for it.

promag commented at 12:19 AM on July 16, 2019: member

Concept ACK.
DrahtBot commented at 6:44 PM on July 16, 2019: member



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



Conflicts

No conflicts as of last run.
achow101 force-pushed on Jul 17, 2019
ryanofsky approved
ryanofsky commented at 11:31 AM on July 20, 2019: member

utACK ebaf3ccfe5407a08dae495206f384dd49ea79ee3. Release notes could say something like "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."
meshcollider commented at 10:25 AM on July 27, 2019: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/16394/commits/ebaf3ccfe5407a08dae495206f384dd49ea79ee3, tests looks good, I like the suggestion by promag/jnewbery too
DrahtBot added the label Needs rebase on Jul 29, 2019
achow101 force-pushed on Jul 29, 2019
achow101 force-pushed on Jul 29, 2019

Allow createwallet to take empty passwords to make unencrypted wallets

Allow createwallet to take the empty string as a password and interpret that
as leaving the wallet unencrypted. Also warn when that happens.

c5d3787367

achow101 force-pushed on Jul 29, 2019
achow101 commented at 3:50 PM on July 29, 2019: member

Rebased and added a release note. Also fixed a bug in the test.
DrahtBot removed the label Needs rebase on Jul 29, 2019
meshcollider commented at 8:49 AM on July 30, 2019: contributor

re-utACK c5d37873677551caac34752214dd491f5278c8d5
in src/wallet/rpcwallet.cpp:2709 in c5d3787367
```
2702 | @@ -2702,6 +2703,12 @@ static UniValue createwallet(const JSONRPCRequest& request)
2703 |          // no default case, so the compiler can warn about missing cases
2704 |      }
2705 |  
2706 | +    if (warning.empty()) {
2707 | +        warning = create_warning;
2708 | +    } else if (!warning.empty() && !create_warning.empty()){
2709 | +        warning += "; " + create_warning;
```
ryanofsky commented at 4:48 PM on July 31, 2019:

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.

achow101 commented at 5:39 PM on July 31, 2019:

I don't feel like changing it.

promag commented at 5:47 PM on July 31, 2019:

When I suggested "join them" I also thought of multiple lines.
ryanofsky approved
ryanofsky commented at 4:50 PM on July 31, 2019: member

utACK c5d37873677551caac34752214dd491f5278c8d5. Changes since last review are rebasing, concatenating warning strings to avoid discarding warnings, adding release notes, and choosing an unambiguous wallet name for the test.
jnewbery commented at 7:47 PM on July 31, 2019: member

code review ACK c5d37873677551caac34752214dd491f5278c8d5
meshcollider merged this on Aug 1, 2019
meshcollider closed this on Aug 1, 2019
meshcollider referenced this in commit 6841b01340 on Aug 1, 2019
sidhujag referenced this in commit d251d02c6b on Aug 1, 2019
luke-jr commented at 12:11 AM on August 20, 2019: member

Could have just passed null?
vansergen referenced this in commit 3950de0205 on Mar 26, 2020
jasonbcox referenced this in commit ea1ca12b52 on Aug 1, 2020
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#16670 util: Add Join helper to join a list of strings

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

Conflicts