Descriptor checksums #15368

sipa · 2019-02-07T23:16:38Z

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.

instagibbs · 2019-02-08T02:21:06Z

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 · 2019-02-08T02:30:08Z

@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 · 2019-02-08T03:18:08Z

Concept ACK

promag · 2019-02-08T14:44:12Z

Concept ACK.

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

sipa · 2019-02-08T20:04:55Z

Added a section to doc/descriptors.md.

doc/descriptors.md

src/test/descriptor_tests.cpp

src/script/descriptor.cpp

Sjors

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).

src/rpc/misc.cpp

meshcollider · 2019-02-10T20:09:10Z

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

src/rpc/misc.cpp

laanwj · 2019-02-12T12:22:28Z

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

sipa · 2019-02-13T03:17:28Z

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

src/script/descriptor.cpp

Sjors · 2019-02-13T09:56:59Z

Breaks Travis due to #14918.

Sjors

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.

src/script/descriptor.cpp

src/rpc/misc.cpp

DrahtBot · 2019-02-16T07:59:15Z

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

utACK fd637be

meshcollider · 2019-02-16T11:12:30Z

src/script/descriptor.cpp

+     *  - 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).


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 · 2019-02-16T20:38:59Z

utACK fd637be

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

cbf2d75 qa: Add getdescriptorinfo functional test (João Barbosa) Pull request description: The `getdescriptorinfo` RPC was added in #15368, this PR adds some tests. Top commit has no ACKs. Tree-SHA512: 5bf3fb5842b975089821c7ac52202ecb23df255f655862646eb532e38e335ff963f8973bcf5b8bba386183281dc9bfe7279ba1cf25fd518c9a45fb45a9243e4d

Summary: bitcoin/bitcoin@3b40bff --- Partial backport of Core [[bitcoin/bitcoin#15368 | PR15368]] Test Plan: ninja check test_runner.py rpc_scantxoutset.py Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6600

Summary: bitcoin/bitcoin@b52cb63 --- Depends on D6600 Partial backport of Core [[bitcoin/bitcoin#15368 | PR15368]] Test Plan: ninja test_runner.py wallet_address_types Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6601

…es and importmulti Summary: bitcoin/bitcoin@be62903 --- Depends on D6601 Partial backport of Core [[bitcoin/bitcoin#15368 | PR15368]] Test Plan: ninja test_runner.py rpc_deriveaddresses wallet_importmulti Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6602

Summary: bitcoin/bitcoin@fd637be --- Depends on D6602 Concludes backport of Core [[bitcoin/bitcoin#15368 | PR15368]] Test Plan: none Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6609

merge bitcoin#14150, bitcoin#14477, bitcoin#14646, bitcoin#14667, bitcoin#14886, bitcoin#14565, bitcoin#14491, bitcoin#15368: descriptors

sipa force-pushed the 201902_descsum branch from 353ed93 to cc8066f Compare February 7, 2019 23:23

meshcollider added the Wallet label Feb 8, 2019

meshcollider added this to the 0.18.0 milestone Feb 8, 2019

promag reviewed Feb 8, 2019

View reviewed changes

doc/descriptors.md Outdated Show resolved Hide resolved

sipa force-pushed the 201902_descsum branch from 975a4c4 to 141c965 Compare February 8, 2019 20:08

practicalswift reviewed Feb 9, 2019

View reviewed changes

src/test/descriptor_tests.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Feb 9, 2019

View reviewed changes

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

Sjors reviewed Feb 10, 2019

View reviewed changes

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

laanwj reviewed Feb 12, 2019

View reviewed changes

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

sipa force-pushed the 201902_descsum branch 6 times, most recently from 1f4aad9 to 5a04daf Compare February 13, 2019 03:14

sipa commented Feb 13, 2019

View reviewed changes

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

sipa force-pushed the 201902_descsum branch 3 times, most recently from 36694bb to d62fba0 Compare February 13, 2019 08:47

Sjors reviewed Feb 13, 2019

View reviewed changes

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

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

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

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

sipa force-pushed the 201902_descsum branch from d62fba0 to 28af119 Compare February 13, 2019 17:54

meshcollider reviewed Feb 16, 2019

View reviewed changes

laanwj merged commit fd637be into bitcoin:master Feb 16, 2019

achow101 mentioned this pull request Feb 17, 2019

Descriptor checksums bitcoin-core/HWI#129

Closed

promag mentioned this pull request Feb 19, 2019

qa: Add getdescriptorinfo functional test #15443

Merged

fanquake mentioned this pull request Mar 9, 2019

Update documentation and change name to Hardware Wallet Interface bitcoin-core/HWI#138

Merged

fanquake removed the Needs release note label May 17, 2019

kwvg mentioned this pull request Oct 7, 2021

merge bitcoin#14150, #14477, #14646, #14667, #14886, #14565, #14491, #15368: descriptors dashpay/dash#4494

Merged

7 tasks

kwvg added a commit to kwvg/dash that referenced this pull request Oct 28, 2021

merge bitcoin#15368: Descriptor checksums

3c63ffa

UdjinM6 added a commit to dashpay/dash that referenced this pull request Nov 1, 2021

Merge pull request #4494 from kittywhiskers/descaddr

bbe9b3d

merge bitcoin#14150, bitcoin#14477, bitcoin#14646, bitcoin#14667, bitcoin#14886, bitcoin#14565, bitcoin#14491, bitcoin#15368: descriptors

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#15368: Descriptor checksums

30a790a

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Descriptor checksums #15368

Descriptor checksums #15368

Uh oh!

Conversation

sipa commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Feb 8, 2019

Uh oh!

sipa commented Feb 8, 2019

Uh oh!

meshcollider commented Feb 8, 2019

Uh oh!

promag commented Feb 8, 2019

Uh oh!

sipa commented Feb 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meshcollider commented Feb 10, 2019

Uh oh!

Uh oh!

laanwj commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sjors commented Feb 13, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Feb 16, 2019

Conflicts

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sipa commented Feb 7, 2019 •

edited

Loading

Sjors left a comment •

edited

Loading

laanwj commented Feb 12, 2019 •

edited

Loading

sipa commented Feb 13, 2019 •

edited

Loading