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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
592 | @@ -593,15 +593,23 @@ class ComboDescriptor final : public DescriptorImpl 593 | ComboDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Singleton(std::move(prov)), {}, "combo") {} 594 | }; 595 | 596 | -/** A parsed multi(...) descriptor. */ 597 | +/** A parsed multi(...) or sortedmulti(...) descriptor */ 598 | class MultisigDescriptor final : public DescriptorImpl 599 | { 600 | const int m_threshold; 601 | + bool m_sorted;
Also const?
Done
835 | @@ -827,7 +836,10 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon 836 | error = "Cannot have combo in non-top level"; 837 | return nullptr; 838 | } 839 | - if (Func("multi", expr)) { 840 | + if (Func("sortedmulti", expr)) { 841 | + sorted_multi = true; 842 | + } 843 | + if (Func("multi", expr) || sorted_multi) {
Swap operands?
Done
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
79 | + with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: 80 | + vectors = json.load(f) 81 | + 82 | + for t in vectors: 83 | + key_str = ','.join(t['keys']) 84 | + self.nodes[0].deriveaddresses(descsum_create('sh(sortedmulti(2,' + key_str + '))'))[0] == t['address']
This just returns false without failing the test. In fact all these addresses are incorrect; you either have to run the node in mainnet (like feature_config_args), or replace the test vectors for regtest. I manually verified with a mainnet node that the vectors do pass.
Whoops. Fixed. Changed the addresses to regtest ones. You can check they still map to the same scriptPubKey using getaddressinfo and decodescript.
I tested with https://github.com/cryptoadvance/specter-desktop/pull/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.
ACK a6dd441cb2d8325dd968897776e7f8389cda359c
37 | @@ -37,12 +38,14 @@ Output descriptors currently support: 38 | - `sh(wsh(pkh(02e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)))` describes an (overly complicated) P2SH-P2WSH-P2PKH output with the specified public key. 39 | - `multi(1,022f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe4,025cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc)` describes a bare *1-of-2* multisig output with keys in the specified order. 40 | - `sh(multi(2,022f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a01,03acd484e2f0c7f65309ad178a9f559abde09796974c57e714c35f110dfc27ccbe))` describes a P2SH *2-of-2* multisig output with keys in the specified order. 41 | +- `sh(sortedmulti(2,03acd484e2f0c7f65309ad178a9f559abde09796974c57e714c35f110dfc27ccbe,022f01e5e15cca351daff3843fb70f3c2f0a1bdd05e5af888a67784ef3e10a2a01))` describes a P2SH *2-of-2* multisig output with keys in the sorted lexicographically in the resulting redeemScript.
s/keys in the sorted/keys sorted/
Done
43 | - `sh(wsh(multi(1,03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8,03499fdf9e895e719cfd64e67f07d38e3226aa7b63678949e6e49b241a60e823e4,02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e)))` describes a P2SH-P2WSH *1-of-3* multisig output with keys in the specified order.
44 | - `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)` describes a P2PK output with the public key of the specified xpub.
45 | - `pkh(xpub68Gmy5EdvgibQVfPdqkBBCHxA5htiqg55crXYuXoQRKfDBFA1WEjWgP6LHhwBZeNK1VTsfTFUHCdrfp1bgwQ9xv5ski8PX9rL2dZXvgGDnw/1'/2)` describes a P2PKH output with child key *1'/2* of the specified xpub.
46 | - `pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)` describes a set of P2PKH outputs, but additionally specifies that the specified xpub is a child of a master with fingerprint `d34db33f`, and derived using path `44'/0'/0'`.
47 | - `wsh(multi(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))` describes a set of *1-of-2* P2WSH multisig outputs where the first multisig key is the *1/0/`i`* child of the first specified xpub and the second multisig key is the *0/0/`i`* child of the second specified xpub, and `i` is any number in a configurable range (`0-1000` by default).
48 | +- `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))` describes a set of *1-of-2* P2WSH multisig outputs where the one multisig key is the *1/0/`i`* child of the first specified xpub and the other multisig key is the *0/0/`i`* child of the second specified xpub, and `i` is any number in a configurable range (`0-1000` by default). The order of public keys in the resulting witnessScripts is determined by the lexicographic order of the public keys at that index.
s/where the one multisig key/where one multisig key/
Done
104 | @@ -101,11 +105,12 @@ not contain "p2" for brevity. 105 | 106 | Several pieces of software use multi-signature (multisig) scripts based 107 | on Bitcoin's OP_CHECKMULTISIG opcode. To support these, we introduce the 108 | -`multi(k,key_1,key_2,...,key_n)` function. It represents a *k-of-n* 109 | +`multi(k,key_1,key_2,...,key_n)` and `sortedmulti(k,key_1,key_2,...,key_n)` 110 | +functions. It represents a *k-of-n*
s/It represents/They represent/ (?)
Done
Love this.
79 | + with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: 80 | + vectors = json.load(f) 81 | + 82 | + for t in vectors: 83 | + key_str = ','.join(t['keys']) 84 | + assert_equal(self.nodes[0].deriveaddresses(descsum_create('sh(sortedmulti(2,' + key_str + '))'))[0], t['address'])
nit, in order to make this shorter:
keys = ','.join(t['keys'])
desc = descsum_create('sh(sortedmulti(2,{}))'.format(keys))
assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address'])
Done
ACK 9214dd6f91, add release notes and call it a day.
Added a release note.
0 | @@ -0,0 +1,4 @@ 1 | +Low-level RPC Changes 2 | +=== 3 | + 4 | +- A new descriptor type `sortedmulti(...)` has been added to support multisig scripst where the public keys are sorted lexicographically in the resulting script.
s/scripst/scripts/
Done
ACK 2638791a4f25394dc241367c933a2d27ed981197, only changes are the suggestion in the test and added the release note.
re-ACK 2638791a4f25394dc241367c933a2d27ed981197
79 | + with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: 80 | + vectors = json.load(f) 81 | + 82 | + for t in vectors: 83 | + key_str = ','.join(t['keys']) 84 | + desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str))
You could check whether a sh(multi(2,{})) descriptor with the json's "sorted_keys" gives the same result.
Done
835 | @@ -827,7 +836,10 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon 836 | error = "Cannot have combo in non-top level"; 837 | return nullptr; 838 | } 839 | - if (Func("multi", expr)) { 840 | + if (Func("sortedmulti", expr)) { 841 | + sorted_multi = true; 842 | + } 843 | + if (sorted_multi || Func("multi", expr)) {
I very slightly prefer if ((sorted_multi = Func("sortedmulti", expr)) || Func("multi", expr)) {
Done
ACK 2638791a4f25394dc241367c933a2d27ed981197. Agree on calling it sortedmulti as it's indeed more generic than bip67.
re-ACK 4bb660be90a2811b53855bf1fd33a8dd9ba3db47