This is based on #14491, review the last 3 commits only.
Currently, descriptors have an Expand() function which returns public keys and scripts for a specific index of a ranged descriptor. But the private key for a specific index is not given. This allows private keys for specific indices to be derived. This also allows those keys to be imported through the importmulti RPC rather than having to provide them separately.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Concept ACK. Code changes look simple enough at first glance, will review more once upstream is merged.
I suggest dropping the keys argument for importmulti when a descriptor is used. At least assuming this makes it into the same release as the previous PR.
ACK with some testing.
I tested one ranged and one non-ranged descriptor, verified with deriveaddresses and getaddressinfo that I get the correct keypaths and such displayed everywhere, and is_mine is true where expected. I did not test actually spending.
I get a slightly weird result from getaddressinfo which I assume is probably not exactly the fault of this PR: I get an “hdseedid” of “0000000000000000000000000000000000000000” for all keys derived this way. It seems like, if we don’t have an hdseedid, that key should instead be left out, as it is optional. I suspect before this change we may rarely or never have had derived private keys without an hdseedid available, so it never came up.
I am interested in whether a key imported this way would be expected to be used by the wallet, in the case where we are trying to sign for some other descriptor where the key appeared, other than the one imported using importmulti. (In particular, a multisig descriptor where we only have one key and are creating a partial signature using walletprocesspsbt.)
583@@ -584,12 +584,12 @@ def run_test(self):
584 self.test_importmulti({"desc": descsum_create(desc),
585 "timestamp": "now",
586 "range": 1},
587- success=True,
588- warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
589+ success=True)
# Test importing of a ranged descriptor without keys) needs to be updated to something like # Test importing of a ranged descriptor with xpriv
583@@ -584,12 +584,12 @@ def run_test(self):
584 self.test_importmulti({"desc": descsum_create(desc),
585 "timestamp": "now",
586 "range": 1},
587- success=True,
588- warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
589+ success=True)
590 for address in addresses:
address loop variable in not being used, and the address tested is left over from the test above)
@jnewbery not sure what you mean, it calls self.test_address(address, ...) with each address to ensure they imported correctly from the xpriv after failing to import in the test before?
EDIT: sorry, I was looking at an older branch, will fix
Concept ACK.
I’ve left a couple of comments on the test. The last two commits should be squashed (currently the penultimate commit fails because the RPC has changed but test hasn’t been updated).
1153@@ -1154,12 +1154,12 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1154
1155 const UniValue& priv_keys = data.exists("keys") ? data["keys"].get_array() : UniValue();
1156
1157- // Expand all descriptors to get public keys and scripts.
1158- // TODO: get private keys from descriptors too
1159+ // Expand all descriptors to get public keys and scripts, and private keys if available.
importmulti as a supporting RPC in descriptors.md.
You may also add a descriptor import example in importmulti
Tested the following descriptors :
For each, I took master key from wallet dump, then created a blank wallet and importmulti descriptor then diff expanded private keys from blank wallet dump against master wallet dump ones.
I had to turn require_checksum as false in Parse due to not straighforward way to calculate checksum for descriptors with private keys (and it’s not really clear from descriptor.cpp if bip32 encoded xpub or xpriv need to be also in form SCRIPT#CHECKSUM), maybe add checksum boolean in importmulti args ?
Also while testing wsh(multi(2, …, …)) I got descriptor parsed as p2sh-segwit address instead of a bech32 one, is this normal (with addresstype=p2sh-segwit) ?
Also while testing wsh(multi(2, …, …)) I got descriptor parsed as p2sh-segwit address instead of a bech32 one, is this normal (with addresstype=p2sh-segwit) ?
I don’t know what you mean by this. What RPC/function calls did you make, and what did you observe?
I did :
bitcoin-cli importmulti '[{ "desc" : "wsh(multi(2,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf/0h/1h/0h, tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf/0h/1h/1h))", "timestamp" : 0 }]'
Got "success" : true
Then, with address from wallet dump
bitcoin-cli getaddressinfo 2N35sQ7r7C2nZT7r3qiZHMYPB8b2PKaNczv
Result :
{
“address”: “2N35sQ7r7C2nZT7r3qiZHMYPB8b2PKaNczv”,
“scriptPubKey”: “a9146bec53ca4b8b49e6581aec6d741962595dc6954487”,
“ismine”: true,
“solvable”: true,
“desc”: “sh(multi(2,[f330b5bd]02d1c3cd9ee8feede98f825120b806c515ac334358363678b9f4cde840ba4f2b83,[84f1c397]03e4382dd7a9cb5c77929504ed8c8dfd3527fa044c766a57c0001bfb222ab1ecad))#e34pekd5”,
“iswatchonly”: false,
“isscript”: true,
“iswitness”: false,
“script”: “multisig”,
“hex”: “522102d1c3cd9ee8feede98f825120b806c515ac334358363678b9f4cde840ba4f2b832103e4382dd7a9cb5c77929504ed8c8dfd3527fa044c766a57c0001bfb222ab1ecad52ae”,
“sigsrequired”: 2,
“pubkeys”: [
“02d1c3cd9ee8feede98f825120b806c515ac334358363678b9f4cde840ba4f2b83”,
“03e4382dd7a9cb5c77929504ed8c8dfd3527fa044c766a57c0001bfb222ab1ecad”
],
“ischange”: true,
“labels”: [
]
}
That’s not surprising, but why are you calling getaddressinfo on a P2SH address rather than a P2WSH one? Is that the address you get from deriveaddresses?
EDIT: Oh, because that’s the address you got from the dump? Don’t rely on those, it’s very hard for the current logic to guess what addresses you actually wanted imported (it inevitably imports other versions of the same policy along with it).
Yes that’s it I was relying on address from the dump, it’s fine with deriveaddresses I got a bech32 one
So tested ACK e00c3dd minor the maybe-add-a-no-checksum option arg in RPC
I’d like to see more tests. I tried a similar example as @ariard, but having some difficulty:
0src/bitcoin-cli getdescriptorinfo "wsh(multi(2,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf))"
1{
2 "descriptor": "wsh(multi(2,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA))#458rdgch",
3 "isrange": false,
4 "issolvable": true,
5 "hasprivatekeys": true
6}
I can derive addresses using the public key version with checksum returned above:
0src/bitcoin-cli deriveaddresses "wsh(multi(2,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA))#458rdgch"
1[
2 "tb1qxhuy9m6uczvc5l5m4dyx8kvplkf0hfwru8cd82yqrc74g6e8rlmsug2jdx"
3]
But I can’t import it (in a blank wallet):
0src/bitcoin-cli importmulti '[{ "desc" : "wsh(multi(2,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf))", "timestamp" : 0, "keypool": false }]'
1[
2 {
3 "success": false,
4 "error": {
5 "code": -5,
6 "message": "Descriptor is invalid"
7 }
8 }
9]
My guess is that it needs a checksum. See related discussion in #15740.
322@@ -312,6 +323,18 @@ class BIP32PubkeyProvider final : public PubkeyProvider
323 }
324 return true;
325 }
326+ bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
327+ {
328+ CExtKey extkey;
In commit “Add private key derivation functions to descriptors” (ca039db0776b1626ce4d220a0da5ff985d93985d)
This is duplicating code already in GetPubKey above. Since this has to exist separately, it’d be good to change GetPubKey to call this function. It’d simplify GetPubKey and remove a chunk of duplicate code.
it’s unclear what other kinds of tests would be helpful here.
The added test in this PR checks that importing sh(wpkh(xpriv/path)) makes the sh(wpkh(pubkey)) spendable for paths /0 and /1.
Suggested additional tests:
wpkh(pubkey) is spendable for paths /0 and /1 (tests the if (m_script_arg) path in ExpandPrivate()).GetPrivKey() in ConstPubkeyProvider).for (auto entry : m_path) path in BIP32PubkeyProvider:: GetPrivKey().
Tested ACK 2857bc4a64cc8dc7914bc984ac878397ac64292d
Checked it compiled and passed tests. Also tested an import manually and checked that private keys are actually imported and can be exported with dumpprivkey. I would feel better if you had a test that did dumpprivkey or signmessage on the imported keys to be sure that the private keys exist.
ACK 53b7de629d3d9281dc6f8fa10e32c4cdec59c140
Squash yo commits