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

Descriptor checksums #15368

Merged
merged 4 commits into from Feb 16, 2019

Conversation

@sipa
Copy link
Member

commented Feb 7, 2019

This adds support for a descriptor-specific 8-character checksum.

Descriptors may optionally be suffixed with a # plus these 8 checksum characters. Any descriptor that contains a # at the end must be followed by a valid checksum. If the # is missing entirely, it is valid without checksum.

All RPCs are updated to report descriptors that include the checksum. On input, they are optional except in deriveaddress and importmulti, which require descriptors which include a checksum.

A new RPC is also added to analyse descriptors (getdescriptorinfo), which can be used to compute the checksum for a descriptor without.

@sipa sipa force-pushed the sipa:201902_descsum branch from 353ed93 to cc8066f Feb 7, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Can a motivation for the placement be given? Accidentally eliding it for whatever reason neuters the protection while still maintaining a valid descriptor, but clearly it seems simpler from an implementation and compatibility perspective.

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

@gsanders Well for critical RPCs the plan is that the checksum won't be optional. I just haven't included that in this PR as it means adapting a bunch of tests, which I only want to do once the checksum algorithm is final.

@meshcollider meshcollider added the Wallet label Feb 8, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Concept ACK

@meshcollider meshcollider added this to the 0.18.0 milestone Feb 8, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Concept ACK.

It'd be nice to read a draft to update doc/descriptors.md.

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

Added a section to doc/descriptors.md.

Show resolved Hide resolved doc/descriptors.md Outdated

@sipa sipa force-pushed the sipa:201902_descsum branch from 975a4c4 to 141c965 Feb 8, 2019

@Sjors
Copy link
Member

left a comment

Concept ACK, will review shortly. Agree that deriveaddress and importmulti should require a checksum. The introduction of getdescriptorinfo means there's no need to make that opt-out.

Do I understand correctly that the checksum is either based on the canonical form, i.e. based on public keys, or skips keys altogether (but that seems suboptimal when these keys are not in checksummed xpub form)? Otherwise the result of getdescriptorinfo could be confusing if you feed it an xpriv.

Maybe return a warning if a user does provide an xpriv that they should clear their shell command history (and generally recommend either not doing that, or providing a safer method like #15346).

Show resolved Hide resolved src/rpc/misc.cpp Outdated
@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

The surrounding code looks good other than the comments above, haven't reviewed the actual checksum code itself yet

Show resolved Hide resolved src/rpc/misc.cpp
@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

lightly tested ACK, code changes look good to me, haven't checked any of the magic numbers in PolyMod

@sipa sipa force-pushed the sipa:201902_descsum branch 6 times, most recently from 1f4aad9 to 5a04daf Feb 13, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Several changes:

  • Addressed all comments
  • Finalized the checksum design (and switched to a slightly better generator)
  • Added explanation (incl. Sage code) of the checksum
  • Made checksums mandatory in deriveaddresses and importmulti
  • Expanded and updated tests, including a Python implementation of the checksum

@sipa sipa force-pushed the sipa:201902_descsum branch 3 times, most recently from 36694bb to d62fba0 Feb 13, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Breaks Travis due to #14918.

@Sjors
Copy link
Member

left a comment

tACK 4f95087 modulo RPC help syntax

I can't vouch for the actual math, but the documentation, Sage code, tests and Python re-implementation are comforting. One way, perhaps overkill, to sanity check that the checksum works as intended is to generate a whole bunch of deterministic typos and see that they are indeed detected.

Show resolved Hide resolved src/script/descriptor.cpp
Show resolved Hide resolved src/script/descriptor.cpp Outdated
Show resolved Hide resolved src/script/descriptor.cpp
Show resolved Hide resolved src/rpc/misc.cpp

@sipa sipa force-pushed the sipa:201902_descsum branch from d62fba0 to 28af119 Feb 13, 2019

@sipa sipa force-pushed the sipa:201902_descsum branch from 28af119 to d2ac7e5 Feb 13, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Rebased.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

ACK

@instagibbs
Copy link
Member

left a comment

Please add a single functional test for deriveaddresses and importmulti lacking the checksum, I compiled out the mandatory flag for them and tests seem to pass.

Show resolved Hide resolved src/rpc/misc.cpp

@sipa sipa force-pushed the sipa:201902_descsum branch from d2ac7e5 to a086e84 Feb 14, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Rebased and added a test to deriveaddresses and importmulti to test for missing checksum.

@instagibbs
Copy link
Member

left a comment

tests now fail when I allow no checksum

tACK 3416454

Show resolved Hide resolved test/functional/rpc_deriveaddresses.py Outdated
@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Can the deriveaddresses RPC example be updated with a checksum, otherwise it will no-longer work.

Looks like the descriptor should be (from getdescriptorinfo):

"wpkh([d34db33f/84'/0'/0']xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#trd0mf0l"
@promag
Copy link
Member

left a comment

Needs release for the new RPC. deriveaddress release notes already points to descriptors.md.

Show resolved Hide resolved src/script/descriptor.h

@sipa sipa force-pushed the sipa:201902_descsum branch from a086e84 to a19b3f9 Feb 15, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@promag I've just added a "needs release notes" label for now, as it intersects with the notes added for deriveaddresses and importmulti.

@fanquake Done.

@promag
Copy link
Member

left a comment

Could have an "empty checksum" test. Feel free to ignore my nits. LGTM code wise.

Show resolved Hide resolved src/script/descriptor.h Outdated
Show resolved Hide resolved src/script/descriptor.cpp Outdated
Show resolved Hide resolved src/script/descriptor.cpp Outdated
int cls = 0;
int clscount = 0;
for (auto ch : span) {
auto pos = INPUT_CHARSET.find(ch);

This comment has been minimized.

Copy link
@promag

promag Feb 16, 2019

Member

nit, could avoid linear search by having an array to map to pos

static int CHAR_POS[] = { ... }; // -1 if invalid
...
int pos = CHAR_POS[ch];
if (pos == -1) return "";

This comment has been minimized.

Copy link
@sipa

sipa Feb 16, 2019

Author Member

I don't think the extra code is worth the performance gain.

Show resolved Hide resolved src/rpc/misc.cpp
Show resolved Hide resolved src/script/descriptor.cpp Outdated
Show resolved Hide resolved src/script/descriptor.cpp Outdated

@sipa sipa force-pushed the sipa:201902_descsum branch from a19b3f9 to f1d207b Feb 16, 2019

@sipa sipa force-pushed the sipa:201902_descsum branch from f1d207b to fd637be Feb 16, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 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:

  • #15414 ([wallet] allow adding pubkeys from imported private keys to keypool by Sjors)
  • #14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)

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.

@meshcollider
Copy link
Member

left a comment

utACK fd637be

* - As many alphabetic characters are in the same group (while following the above restrictions).
*
* If p(x) gives the position of a character c in this character set, every group of 3 characters
* (a,b,c) is encoded as the 4 symbols (p(a) & 31, p(b) & 31, p(c) & 31, (p(a) / 32) + 3 * (p(b) / 32) + 9 * (p(c) / 32).

This comment has been minimized.

Copy link
@meshcollider

meshcollider Feb 16, 2019

Member

I'm not sure if I'm reading this wrong, but isn't the fourth symbol in the wrong order, shouldn't this be (p(c) / 32) + 3 * (p(b) / 32) + 9 * (p(a) / 32)?

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

utACK fd637be

@laanwj laanwj merged commit fd637be into bitcoin:master Feb 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 Feb 16, 2019

Merge #15368: Descriptor checksums
fd637be Add checksums to descriptors.md (Pieter Wuille)
be62903 Make descriptor checksums mandatory in deriveaddresses and importmulti (Pieter Wuille)
b52cb63 Add getdescriptorinfo to compute checksum (Pieter Wuille)
3b40bff Descriptor checksum (Pieter Wuille)

Pull request description:

  This adds support for a descriptor-specific 8-character checksum.

  Descriptors may optionally be suffixed with a `#` plus these 8 checksum characters. Any descriptor that contains a `#` at the end must be followed by a valid checksum. If the `#` is missing entirely, it is valid without checksum.

  All RPCs are updated to report descriptors that include the checksum. On input, they are optional except in `deriveaddress` and `importmulti`, which require descriptors which include a checksum.

  A new RPC is also added to analyse descriptors (`getdescriptorinfo`), which can be used to compute the checksum for a descriptor without.

Tree-SHA512: a8294b09155eb6c67fbc178b5e2d3fbc0e9bec8b6de57a13f8835550d51c2cb32a428b3c9a188ded42b454d594e9305edbd4797906b755de77a8f33c79165f6b

@achow101 achow101 referenced this pull request Feb 17, 2019

Closed

Descriptor checksums #129

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.