descriptors: Be able to specify change and receiving in a single descriptor string #22838
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Strong concept ACK! Glad this is getting a fix. As discussed in 17190 and OP, there's more improvements that can come after this. |
|
Concept and Approach ACK. I plan on updating the test and docs introduced in #22067 to take advantage of this in a follow up PR. So I will try to do a more thorough review and testing of this after that is merged and I start on the followup based on this branch. (or if it looks like this will get merged first I will do that earlier) |
As an example Allowing a mix of hardened and unhardened like |
The conflict is actually with the |
|
I'm doing some more review/testing of this PR, but when I checkout this PR and rebase to master, I get build error: wallet/rpcwallet.cpp:3387:45: error: no viable conversion from 'std::pair<std::unique_ptr<Descriptor>, std::unique_ptr<Descriptor> >' to 'std::unique_ptr<Descriptor>'
std::unique_ptr<Descriptor> desc = Parse(desc_str, desc_out, error, true); |
|
Fixed the silent merge conflict. |
achow101
force-pushed
the
multipath-descs
branch
from
e8a6785
to
f4a9ed5
Compare
|
So in my tests, from this multipath descriptor string: I have a descriptor that looks like this: {
'descriptor': 'wsh(sortedmulti(2,tpubDCcZnBitiPCBqsdLWx8Lkc2BrsvVTdZU2Gcioi8yY76HrGSvp4oWyxZ5GUKGVcjdv4wzWbfntfLdcEEfVRamhSgwi1ZxgfNtayQ2QEEjfTv/0/*,tpubDCMtV5vMS9Zn2paRcw7o83ytLY7WHMCGYRSDxhz7HWnZ4ix19EQpi7g3wvYxEvuoXRfLy5XpXJP5VEr7qPzS92TWnVrBMPJYzfupdACC6vP/0/*,tpubDDBi5pEGAmG7k8t2udh8vbYb1Zpwnt5E9bxd86z6xR66r6tYH1aXy5J3kxR2NDBWhVzgcyiAjFw3kCbkf8YcxDx4wjsnvKjKu3SfPVi4dzf/0/*))#shfp6kvs',
'descriptor_change': 'wsh(sortedmulti(2,tpubDCcZnBitiPCBqsdLWx8Lkc2BrsvVTdZU2Gcioi8yY76HrGSvp4oWyxZ5GUKGVcjdv4wzWbfntfLdcEEfVRamhSgwi1ZxgfNtayQ2QEEjfTv/1/*,tpubDCMtV5vMS9Zn2paRcw7o83ytLY7WHMCGYRSDxhz7HWnZ4ix19EQpi7g3wvYxEvuoXRfLy5XpXJP5VEr7qPzS92TWnVrBMPJYzfupdACC6vP/1/*,tpubDDBi5pEGAmG7k8t2udh8vbYb1Zpwnt5E9bxd86z6xR66r6tYH1aXy5J3kxR2NDBWhVzgcyiAjFw3kCbkf8YcxDx4wjsnvKjKu3SfPVi4dzf/1/*))#ghxep260',
'checksum': 'e82r9xv3',
'isrange': True,
'issolvable': True,
'hasprivatekeys': False
}But now, 2021-10-18T22:26:50.595000Z TestFramework (INFO): Check that every participant's multisig generates the same addresses...
2021-10-18T22:26:50.602000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/michaeldietz/Documents/bitcoin/test/functional/wallet_multisig_descriptor_psbt.py", line 95, in run_test
change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
File "/Users/michaeldietz/Documents/bitcoin/test/functional/wallet_multisig_descriptor_psbt.py", line 95, in <listcomp>
change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Error: This wallet has no available keys (-4)Any ideas? I am updating #22067 to use multipath descriptors and using it as a chance to test this PR more |
|
@mjdietzx Is your branch pushed so I can have a look? |
|
In my code I actually have: result = multisig.importdescriptors([
{
"desc": descriptor["descriptor"],
"active": True,
"timestamp": "now",
},
])
assert all(r["success"] for r in result)And that doesn't catch any errors. I will push my branch shortly and reply w/ it |
|
I had an error in my tests - it seems everything in this PR is working properly. This works: result = multisig.importdescriptors([
{
"desc": descsum_create(f"wsh(sortedmulti({self.M},{f'/<0;1>/*,'.join(xpubs)}/<0;1>/*))"),
"active": True,
"timestamp": "now",
},
])re the error I posted eariler, I was trying to do this (using descriptor = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/<0;1>/*,'.join(xpubs)}/<0;1>/*))")
result = multisig.importdescriptors([
{
"desc": descriptor["descriptor"],
"active": True,
"timestamp": "now",
},
])But I was not importing or doing anything with We used {
descriptor // still the multipath descriptor, similar to output of descsum_create
// now the broken out descriptors:
descriptor_receiving
descriptor_change
// ... and the other fields are unchanged
} |
The |
src/rpc/misc.cpp
Outdated
| @@ -170,6 +170,7 @@ static RPCHelpMan getdescriptorinfo() | |||
| RPCResult::Type::OBJ, "", "", | |||
| { | |||
| {RPCResult::Type::STR, "descriptor", "The descriptor in canonical form, without private keys"}, | |||
| {RPCResult::Type::STR, "descriptor_change", "The change descriptor in canonical form, without private keys. Only if the provided descriptor specifies derivation paths for both receiving and change."}, | |||
There was a problem hiding this comment.
I'm still not convinced this is the best interface for getdescriptorinfo
I'd expect descriptor to always be the full descriptor (whether multipath or not). In the case of multipath, I'd then expect two optional attributes descriptor_receiving and descriptor_change to be present in the response.
There was a problem hiding this comment.
nit: descriptor_change should be optional
There was a problem hiding this comment.
descriptor cannot currently be the multipath descriptor as generating the string output for one is not yet implemented. I'm not sure on a good approach for doing that yet.
There was a problem hiding this comment.
Are you saying something like:
- The multipath
descriptorstring input togetdescriptorinfois parsed/expanded to the two descriptors - Now each of these descriptors is converted back
ToStringand returned asdescriptor(first) anddescriptor_change(second)
And that we currently don't have a way to go from the two parsed/expanded descriptors back to the single multipath descriptor string?
That does sound tricky, just trying to make sure I understand
There was a problem hiding this comment.
Ok, so what's the downside of something like:
- Each of these parsed descriptors is converted back
ToStringand returned asdescriptor_receiving(first) and descriptor_change (second) - The same multipath
descriptorstring input togetdescriptorinfois just returned for thedescriptoroutput (with a checksum added)
There was a problem hiding this comment.
@achow101 could you entertain me on this, if nothing else it'll help me understand this better as I finish up my review
There was a problem hiding this comment.
#15986 provides a bit more context for this.
There are a few reasons I would prefer to not change the behavior of the descriptor result. The first is for backwards compatibility. This RPC may already be used in a way that expects the canonicalized descriptor produced by getdescriptorinfo in the descriptor field. Additionally, if a descriptor with a private key were provided, if we were to just output that same string again, then we would be echoing private keys which is something that we want to avoid doing.
There was a problem hiding this comment.
descriptorcannot currently be the multipath descriptor as generating the string output for one is not yet implemented. I'm not sure on a good approach for doing that yet.
Mmm, that has me a bit worried. If a user imports a multipath descriptor, I think we should remember it in that form. And if we embrace multipath descriptors, we should probably also use them for new wallets.
Perhaps a better design would be a MultipathDescriptor subclass that only has a receive and change method, or more neutral: a pair method that returns a pair of Descriptors that then work as usual.
In the future std::pair<Descriptor> can be generalized to std::map<uint32, Descriptor> to allow <NUM,NUM,...,NUM>, etc. That may even be appropriate now: we could treat <0;1> as a typical receive/change setup, and refuse to import any other combination (though that's an RPC level thing, and we might as well use std::pair<Descriptor> given such an import restriction).
achow101
force-pushed
the
multipath-descs
branch
from
f4a9ed5
to
796c439
Compare
src/rpc/misc.cpp
Outdated
| {RPCResult::Type::STR, "address", "the derived addresses"}, | ||
| } | ||
| { | ||
| RPCResult{"for single derivation descriptors", |
There was a problem hiding this comment.
I've been thinking through the response returned here. It doesn't seem ideal that the shape of this response differs between a single descriptor and a multipath descriptor. Ie this returns an array of addresses if we pass in a descriptor, but it returns an object if we pass in a multipath descriptor.
Have you thought through an api where the response is more uniform and "feels" the same for both types? I don't necessarily know a better way to do it, wondering if you've thought this through though
There was a problem hiding this comment.
The primary reason for having the two different return results is to maintain backwards compatibility with anyone who may be using the RPC now.
There was a problem hiding this comment.
Yeah, I figured there'd need to be a deprecation process if we wanted to do this. Anyways, if you think it's enough of an improvement to warrant that I'd be happy to do give it a go as a followup PR. Just lmk
src/rpc/misc.cpp
Outdated
| obj.pushKV("change", DeriveAddresses(descs.second.get(), range_begin, range_end, key_provider)); | ||
| return obj; | ||
| } else { | ||
| return addresses; |
There was a problem hiding this comment.
Related to my above comment, if the response is uniform we could potentially simplify this a bit and have a single return at the end of the function
|
I think there may be another silent merge conflict w/ master: wallet/test/util.cpp:33:37: error: no viable conversion from 'std::pair<std::unique_ptr<Descriptor>, std::unique_ptr<Descriptor> >' to 'std::unique_ptr<Descriptor>'
std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);Can you try rebasing again? |
achow101
requested review from
darosior and
laanwj
and removed request for
mjdietzx
achow101
force-pushed
the
multipath-descs
branch
from
9f3632a
to
114dcbd
Compare
To prepare for returning multipath descriptors which will be a shorthand for specifying multiple descriptors, change ParseScript's signature to return a vector.
achow101
force-pushed
the
multipath-descs
branch
from
114dcbd
to
d2b43c3
Compare
Multipath specifiers are derivation path indexes of the form `<i;j;k;...>` used for specifying multiple derivation paths for a descriptor. Only one multipath specifier is allowed per PubkeyProvider. This is syntactic sugar which is parsed into multiple distinct descriptors. One descriptor will have all of the `i` paths, the second all of the `j` paths, the third all of the `k` paths, and so on. ParseKeypath will always return a vector of keypaths with the same size as the multipath specifier. The callers of this function are updated to deal with this case and return multiple PubkeyProviders. Their callers have also been updated to handle vectors of PubkeyProviders.
When given a descriptor which contins a multipath derivation specifier, a vector of descriptors will be returned.
When given a multipath descriptor, derive all of the descriptors. The derived addresses will be returned in an object consisting of multiple arrays. For compatibility, when given a single path descriptor, the addresses are provided in a single array as before.
Instead of applying internal-ness to all keys being imported at the same time, apply it on a per key basis. So each key that is imported will carry with it whether it is for the change keypool.
Multipath descriptors will be imported as multiple separate descriptors. When there are exactly 2 multipath items, the first descriptor will be for receiving addreses, and the second for change addresses. When importing a multipath descriptor, 'internal' cannot be specified.
Multipath descriptors will be imported as multiple separate descriptors. When there are 2 multipath items, the first descriptor will be for receiving addresses and the second for change. This mirrors importmulti.
Test that both importmulti and importdescriptors behave as expected when importing a multipath descriptor.
achow101
force-pushed
the
multipath-descs
branch
from
d2b43c3
to
cb41e26
Compare
src/script/descriptor.cpp
Outdated
| ||||||| parent of 657176b834d (descriptors: Add DescriptorImpl::Clone) | ||
| ======= | ||
|
|
||
| virtual std::unique_ptr<DescriptorImpl> Clone() const = 0; | ||
| >>>>>>> 657176b834d (descriptors: Add DescriptorImpl::Clone) |
|
Tested ACK cb41e26. Note: I did not do a thorough code review What I've tested:
diff --git a/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py b/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
index 4f50ae191c..e5f58eb4d2 100755
--- a/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
+++ b/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py
@@ -32,35 +32,26 @@ class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):
self.skip_if_no_sqlite()
@staticmethod
- def _get_xpub(wallet, internal):
+ def _get_xpub(wallet):
"""Extract the wallet's xpubs using `listdescriptors` and pick the one from the `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses)."""
- pkh_descriptor = next(filter(lambda d: d["desc"].startswith("pkh(") and d["internal"] == internal, wallet.listdescriptors()["descriptors"]))
+ pkh_descriptor = next(filter(lambda d: d["desc"].startswith("pkh("), wallet.listdescriptors()["descriptors"]))
# keep all key origin information (master key fingerprint and all derivation steps) for proper support of hardware devices
# see section 'Key origin identification' in 'doc/descriptors.md' for more details...
return pkh_descriptor["desc"].split("pkh(")[1].split(")")[0]
- def create_multisig(self, external_xpubs, internal_xpubs):
+ def create_multisig(self, xpubs):
"""The multisig is created by importing the following descriptors. The resulting wallet is watch-only and every signer can do this."""
self.node.createwallet(wallet_name=f"{self.name}", blank=True, descriptors=True, disable_private_keys=True)
multisig = self.node.get_wallet_rpc(f"{self.name}")
# spending policy: `thresh(4,pk(key_1),pk(key_2),pk(key_3),pk(key_4),after(t1),after(t2),after(t3))`
# IMPORTANT: when backing up your descriptor, the order of key_1...key_4 must be correct!
- external = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(external_xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
- internal = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(internal_xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
- result = multisig.importdescriptors([
- { # receiving addresses (internal: False)
- "desc": external["descriptor"],
- "active": True,
- "internal": False,
- "timestamp": "now",
- },
- { # change addresses (internal: True)
- "desc": internal["descriptor"],
- "active": True,
- "internal": True,
- "timestamp": "now",
- },
- ])
+ descriptors = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(xpubs)}),sln:after({f'),sln:after('.join(map(str, self.locktimes))})))")
+ result = multisig.importdescriptors([{
+ "desc": desc,
+ "active": True,
+ "internal": "/1/*" in desc,
+ "timestamp": "now",
+ } for desc in descriptors["multipath_expansion"]])
assert all(r["success"] for r in result)
return multisig
@@ -77,10 +68,10 @@ class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):
self.log.info("Create the signer wallets and get their xpubs...")
signers = [self.node.get_wallet_rpc(self.node.createwallet(wallet_name=f"signer_{i}", descriptors=True)["name"]) for i in range(self.N)]
- external_xpubs, internal_xpubs = [[self._get_xpub(signer, internal) for signer in signers] for internal in [False, True]]
+ xpubs = [self._get_xpub(signer).replace("/0/*", "/<0;1>/*") for signer in signers]
self.log.info("Create the watch-only decaying multisig using signers' xpubs...")
- multisig = self.create_multisig(external_xpubs, internal_xpubs)
+ multisig = self.create_multisig(xpubs)
self.log.info("Get a mature utxo to send to the multisig...")
coordinator_wallet = self.node.get_wallet_rpc(self.node.createwallet(wallet_name=f"coordinator", descriptors=True)["name"]) |
|
Will re-review from scratch next week to finally get this over the finish line. |
|
I'm not sure if it's the intended behavior, but passing the same self.log.info("Multipath descriptors: duplicate NUM")
self.nodes[1].createwallet(wallet_name="multipath3", descriptors=True, blank=True)
w_multipath = self.nodes[1].get_wallet_rpc("multipath3")
self.test_importdesc({"desc": descsum_create(f"wpkh({xpriv}/<1;1>/*)"),
"active": True,
"range": 10,
"timestamp": "now"},
success=True,
wallet=w_multipath)
result = w_multipath.listdescriptors()
self.log.info(f"{result=}")
assert_equal(len(result["descriptors"]), 1)
assert_equal(result["descriptors"][0]["desc"],"wpkh(tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H/1/*)#hf9h03qd" )
assert_equal(result["descriptors"][0]["internal"], True)
Edit: is there any reason to accept duplictaes |
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in #17190 (comment), it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor
wpkh(xpub.../0/0/*)andwpkh(xpub.../0/1/*)to represent receive and change addresses, this could be written aswpkh(xpub.../0/<0;1>/*). The multipath specifier is of the form<NUM;NUM>. EachNUMcan have its own hardened specifier, e.g.<0;1h>is valid. The multipath specifier can also only appear in one path index in the derivation path.This results in the parser returning two descriptors. The first descriptor uses the first
NUMin all pairs present, and the second uses the secondNUM. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is justnullptr.The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes #17190