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

descriptors: Introduce sortedmulti descriptor #17056

Merged
merged 4 commits into from Oct 8, 2019

Conversation

@achow101
Copy link
Member

achow101 commented Oct 4, 2019

Adds a sortedmulti() descriptor as mentioned in #17023 (comment).

sortedmulti() works in the same way as multi does but sorts the pubkeys in the resulting scripts in lexicographic order as described in BIP67. Note that this does not add support for BIP67 nor is BIP67 fully supported by this descriptor (which is why it is not named multi67()) as it does not require compressed pubkeys.

Tests from BIP67 were added and documentation was updated.

@fanquake fanquake added the Descriptors label Oct 4, 2019
@fanquake fanquake requested a review from sipa Oct 4, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 4, 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:

  • #17023 (doc: warn that ranged multi() descriptors are not BIP67 compatible by Sjors)
  • #16889 (Add some general std::vector utility functions by sipa)
  • #16800 (Basic Miniscript support in output descriptors by sipa)
  • #15590 (Descriptor: add GetAddressType() and IsSegWit() 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.

src/script/descriptor.cpp Outdated Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke changed the title Introduce sortedmulti descriptor descriptors: Introduce sortedmulti descriptor Oct 5, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 5, 2019

Copy link
Member

Sjors left a comment

I tested with cryptoadvance/specter-desktop#8 that ColdCard simulator no longer complains about a change address that it complained about before.

Code review ACK d5a925b except for the functional test issue.

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch from d5a925b to a6dd441 Oct 6, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 6, 2019

ACK a6dd441

Copy link
Contributor

bitcoinhodler left a comment

Love this.

doc/descriptors.md Outdated Show resolved Hide resolved
doc/descriptors.md Outdated Show resolved Hide resolved
doc/descriptors.md Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch from a6dd441 to 9214dd6 Oct 7, 2019
Copy link
Member

promag left a comment

ACK 9214dd6, add release notes and call it a day.

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch from 9214dd6 to 9ce9adb Oct 7, 2019
@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Oct 7, 2019

Added a release note.

@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch from 9ce9adb to 0080a85 Oct 7, 2019
@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch 2 times, most recently from 8ece050 to 2638791 Oct 8, 2019
@laanwj laanwj added the Feature label Oct 8, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 8, 2019

utACK 2638791

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 8, 2019

ACK 2638791, only changes are the suggestion in the test and added the release note.

@fanquake fanquake requested a review from Sjors Oct 8, 2019
@Sjors
Sjors approved these changes Oct 8, 2019
Copy link
Member

Sjors left a comment

re-ACK 2638791

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 8, 2019

ACK 2638791. Agree on calling it sortedmulti as it's indeed more generic than bip67.

test/functional/rpc_createmultisig.py Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:sortedmulti-desc branch from 2638791 to 4bb660b Oct 8, 2019
@fanquake fanquake requested review from sipa, Sjors, promag and instagibbs Oct 8, 2019
@Sjors
Sjors approved these changes Oct 8, 2019
Copy link
Member

Sjors left a comment

re-ACK 4bb660b

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 8, 2019

re-ACK 4bb660b

fanquake added a commit that referenced this pull request Oct 8, 2019
4bb660b Add release note (Andrew Chow)
ed96b29 Update descriptors.md to include sortedmulti (Andrew Chow)
80be78e Test sortedmulti descriptor using BIP 67 tests (Andrew Chow)
6f588fd Add sortedmulti descriptor and unit tests (Andrew Chow)

Pull request description:

  Adds a `sortedmulti()` descriptor as mentioned in #17023 (comment).

  `sortedmulti()` works in the same way as `multi` does but sorts the pubkeys in the resulting scripts in lexicographic order as described in [BIP67](https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki). Note that this does not add support for BIP67 nor is BIP67 fully supported by this descriptor (which is why it is not named `multi67()`) as it does not require compressed pubkeys.

  Tests from BIP67 were added and documentation was updated.

ACKs for top commit:
  instagibbs:
    re-ACK 4bb660b
  Sjors:
    re-ACK 4bb660b

Tree-SHA512: 93b21112a74ebe0bf316d8f3e0291f69fd975cf0a29332f9728e7b880cad312b8b14007e86adcd7899f117b9303cbcf4cb35f3bb2f2f648d1a446f83f75a70a5
@fanquake fanquake merged commit 4bb660b into bitcoin:master Oct 8, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Sjors Sjors mentioned this pull request Oct 29, 2019
0 of 2 tasks complete
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 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.