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 checksum to getdescriptorinfo #15986

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@sipa
Copy link
Member

commented May 8, 2019

No description provided.

@sipa sipa force-pushed the sipa:201905_justchecksum branch from 2100293 to 15c482e May 8, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

So the only reason I didn't write it this way, is that people have said they don't want private keys to be echoed in the output.

Personally I think this is fine -- the only way to get a private key in the output, is if you include it in the input. And in fact, there has to be SOME way to get private keys echoed back, if you want to be able to get checksums for descriptors with private keys included.

So I'd advocate for this PR the way it is, but if people aren't comfortable with it, I'd suggest adding a flag, and not echoing descriptors with private keys unless the flag is set.

Thanks!

(Tagging #15740 for the record.)

@meshcollider

This comment has been minimized.

Copy link
Member

commented May 9, 2019

utACK 15c482e

@Sjors

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Concept ACK.

Light preference for returning only the checksum rather than echoing a private key. But I do agree with @gwillen's point:

the only way to get a private key in the output, is if you include it in the input.

I find the extra with_checksum confusing. Maybe we can add an argument keep_private_keys which defaults to false. I would also prefer sticking to canonical form (but with private keys). An additional argument canonical can be true by default. I know it makes the change a bit more complicated, but it seems more robust in the long run.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16542 (Return more specific errors about invalid descriptors by achow101)

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.

@jb55

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I'm confused by this, getdescriptorinfo already adds a checksum if it's missing?

@sipa

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@jb55 It also normalizes the descriptor (which among other things turns WIP into pubkey, and xprv into xpub)

@jb55

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

ACK 15c482e, only reviewed code change. Looks like returning only the checksum, like @Sjors suggested, is a simple solution to not echo back private key. A small release note could be added.

Refactor commit best viewed with git show --color-words 7976dd65f6503082370a40ac189b925322c29918.

@fanquake fanquake requested a review from achow101 Jul 30, 2019

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Code Review ACK 15c482e

I would prefer if there were also a test where the descriptor were slightly modified (e.g. replacing ' with h in origin info) and that the result of getdescriptorinfo gives the correct canonical form and the modified form with the checksum.

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Tested ACK 15c482e

@sipa

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

I like the suggestion of instead just reporting the checksum for the unmodified descriptor, and not echoing anything back. Since this has been reviewed already, I'm happy to go either way. What do reviewers think?

@achow101

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

I like the suggestion of instead just reporting the checksum for the unmodified descriptor, and not echoing anything back. Since this has been reviewed already, I'm happy to go either way. What do reviewers think?

I prefer this because it is unambiguous as to what the checksum belongs to and what the descriptor with with checksum will look like.

@sipa sipa force-pushed the sipa:201905_justchecksum branch from 15c482e to 26d3fad Aug 7, 2019

@sipa sipa changed the title Add unmodified-descriptor-with-checksum to getdescriptorinfo Add checksum to getdescriptorinfo Aug 7, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

Updated to instead add a "checksum" to getdescriptorinfo that returns the checksum for the (unmodified) descriptor input.

@Sjors
Sjors approved these changes Aug 7, 2019
Copy link
Member

left a comment

ACK 26d3fad

@@ -136,6 +136,7 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
RPCResult{
"{\n"
" \"descriptor\" : \"desc\", (string) The descriptor in canonical form, without private keys\n"
" \"checksum\" : \"chksum\", (string) The checksum for the input descriptor\n"

This comment has been minimized.

Copy link
@Sjors

Sjors Aug 7, 2019

Member

nit: did you really mean chksum?

@achow101

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Code Review ACK 26d3fad

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Two wallet reviewer ACKs here; @meshcollider want to take a final look? I've compiled to check that there aren't any silent merge conflicts etc.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

re-Code Review ACK 26d3fad

meshcollider added a commit that referenced this pull request Aug 16, 2019
Merge #15986: Add checksum to getdescriptorinfo
26d3fad Add unmodified-but-with-checksum to getdescriptorinfo (Pieter Wuille)
104b3a5 Factor out checksum checking from descriptor parsing (Pieter Wuille)

Pull request description:

ACKs for top commit:
  achow101:
    Code Review ACK 26d3fad
  meshcollider:
    re-Code Review ACK 26d3fad
  Sjors:
    ACK 26d3fad

Tree-SHA512: b7a7f89b64a184927d6f9a0c183a087609983f0c5d5593f78e12db4714e930a4af655db9da4b0c407ea2e24d3b926cef6e1f2a15de502d0d1290a6e046826b99

@meshcollider meshcollider merged commit 26d3fad into bitcoin:master Aug 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2019
Merge bitcoin#15986: Add checksum to getdescriptorinfo
26d3fad Add unmodified-but-with-checksum to getdescriptorinfo (Pieter Wuille)
104b3a5 Factor out checksum checking from descriptor parsing (Pieter Wuille)

Pull request description:

ACKs for top commit:
  achow101:
    Code Review ACK 26d3fad
  meshcollider:
    re-Code Review ACK 26d3fad
  Sjors:
    ACK 26d3fad

Tree-SHA512: b7a7f89b64a184927d6f9a0c183a087609983f0c5d5593f78e12db4714e930a4af655db9da4b0c407ea2e24d3b926cef6e1f2a15de502d0d1290a6e046826b99
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.