descriptors: Introduce sortedmulti descriptor #17056
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
promag
reviewed
Oct 5, 2019
Member
|
Concept ACK @peter-conalgo does this match the BIP67 implementation in ColdCard? (assuming compressed public keys). E.g. https://github.com/Coldcard/firmware/blob/master/testing/test_multisig.py#L232-L255 and https://github.com/Coldcard/firmware/blob/master/shared/multisig.py#L412-L414 |
Sjors
suggested changes
Oct 6, 2019
Member
Sjors
left a comment
There was a problem hiding this 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.
d5a925b to
a6dd441
Compare
Member
|
ACK a6dd441cb2d8325dd968897776e7f8389cda359c |
a6dd441 to
9214dd6
Compare
promag
reviewed
Oct 7, 2019
Contributor
promag
left a comment
There was a problem hiding this comment.
ACK 9214dd6f91, add release notes and call it a day.
9214dd6 to
9ce9adb
Compare
Member
Author
|
Added a release note. |
9ce9adb to
0080a85
Compare
8ece050 to
2638791
Compare
Member
|
utACK 2638791 |
Contributor
|
ACK 2638791a4f25394dc241367c933a2d27ed981197, only changes are the suggestion in the test and added the release note. |
Sjors
approved these changes
Oct 8, 2019
Member
Sjors
left a comment
There was a problem hiding this comment.
re-ACK 2638791a4f25394dc241367c933a2d27ed981197
Member
|
ACK 2638791a4f25394dc241367c933a2d27ed981197. Agree on calling it |
sipa
reviewed
Oct 8, 2019
2638791 to
4bb660b
Compare
Sjors
approved these changes
Oct 8, 2019
Member
|
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
2 tasks
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Nov 15, 2019
Github-Pull: bitcoin#17056 Rebased-From: 6f588fd
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Nov 15, 2019
Github-Pull: bitcoin#17056 Rebased-From: 80be78e
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 8, 2020
Summary: 4bb660be90a2811b53855bf1fd33a8dd9ba3db47 Add release note (Andrew Chow) ed96b295d747738334459490c79b7360ab85aaf7 Update descriptors.md to include sortedmulti (Andrew Chow) 80be78ea75ac9833ee3db3d468ed09fc4fe6274c Test sortedmulti descriptor using BIP 67 tests (Andrew Chow) 6f588fd2276e5b713c6d36e3b01288484ddb59c0 Add sortedmulti descriptor and unit tests (Andrew Chow) Pull request description: Adds a `sortedmulti()` descriptor as mentioned in bitcoin/bitcoin#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 bitcoin/bitcoin@4bb660b Sjors: re-ACK 4bb660be90a2811b53855bf1fd33a8dd9ba3db47 --- Depends on D7828 Backport of Core [[bitcoin/bitcoin#17056 | PR17056]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7832
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
sortedmulti()descriptor as mentioned in #17023 (comment).sortedmulti()works in the same way asmultidoes 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 namedmulti67()) as it does not require compressed pubkeys.Tests from BIP67 were added and documentation was updated.