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

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

Merged
merged 1 commit into from Aug 1, 2019

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jul 15, 2019

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 achow101 force-pushed the fix-born-enc branch 2 times, most recently from 9d06635 to 8d0b940 Jul 15, 2019
Copy link
Member

@promag promag left a comment

Concept ACK.

Loading

// 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
Member

@promag promag Jul 16, 2019

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": [...].

Loading

Copy link
Member Author

@achow101 achow101 Jul 17, 2019

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.

Loading

Copy link
Member

@promag promag Jul 17, 2019

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 🙉

Loading

Copy link
Member

@jnewbery jnewbery Jul 26, 2019

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.

Loading

Copy link
Member Author

@achow101 achow101 Jul 29, 2019

Choose a reason for hiding this comment

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

Joined them

Loading

# 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
Member

@promag promag Jul 16, 2019

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?

Loading

Copy link
Member Author

@achow101 achow101 Jul 17, 2019

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.

Loading

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jul 16, 2019

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

Conflicts

No conflicts as of last run.

Loading

Copy link
Contributor

@ryanofsky ryanofsky left a comment

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

Loading

Copy link
Member

@meshcollider meshcollider left a comment

Code review ACK ebaf3cc, tests looks good, I like the suggestion by promag/jnewbery too

Loading

@achow101 achow101 force-pushed the fix-born-enc branch 2 times, most recently from 0be40a8 to 00b822c Jul 29, 2019
Allow createwallet to take the empty string as a password and interpret that
as leaving the wallet unencrypted. Also warn when that happens.
@achow101
Copy link
Member Author

@achow101 achow101 commented Jul 29, 2019

Rebased and added a release note. Also fixed a bug in the test.

Loading

Copy link
Member

@meshcollider meshcollider left a comment

re-utACK c5d3787

Loading

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK c5d3787. 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.

Loading

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

@ryanofsky ryanofsky Jul 31, 2019

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.

Loading

Copy link
Member Author

@achow101 achow101 Jul 31, 2019

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.

Loading

Copy link
Member

@promag promag Jul 31, 2019

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.

Loading

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jul 31, 2019

code review ACK c5d3787

Loading

@meshcollider meshcollider merged commit c5d3787 into bitcoin:master Aug 1, 2019
2 checks passed
Loading
meshcollider added a commit that referenced this issue Aug 1, 2019
…crypted wallets

c5d3787 Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    code review ACK c5d3787
  meshcollider:
    re-utACK c5d3787
  ryanofsky:
    utACK c5d3787. 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.

Tree-SHA512: 146737a728dd614ba94d4b166b27e8c9e195badd1709ccab2315afe59176d9b493dfba9b61c3ed81090f059c7e464d709deb06d99451b9a3fff667f527d6f7c9
sidhujag added a commit to syscoin/syscoin that referenced this issue Aug 1, 2019
…ke unencrypted wallets

c5d3787 Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    code review ACK c5d3787
  meshcollider:
    re-utACK c5d3787
  ryanofsky:
    utACK c5d3787. 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.

Tree-SHA512: 146737a728dd614ba94d4b166b27e8c9e195badd1709ccab2315afe59176d9b493dfba9b61c3ed81090f059c7e464d709deb06d99451b9a3fff667f527d6f7c9
@luke-jr
Copy link
Member

@luke-jr luke-jr commented Aug 20, 2019

Could have just passed null?

Loading

vansergen added a commit to vansergen/rpc-bitcoin that referenced this issue Mar 26, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Aug 1, 2020
…nencrypted wallets

Summary:
Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)

Pull request description:

  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.

bitcoin/bitcoin@c5d3787

---

Backport of Core [[bitcoin/bitcoin#16394 | PR16394]]

Test Plan:
  ninja check
  ./test_runner.py wallet_createwallet

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7108
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants