Allow specific private keys to be derived from descriptor #15024

meshcollider · 2018-12-21T22:30:37Z

~~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.

DrahtBot · 2018-12-24T12:50:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15764 (Native descriptor wallets by achow101)

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.

Sjors

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.

DrahtBot · 2019-02-01T13:44:31Z

Needs rebase

meshcollider · 2019-02-05T06:55:39Z

I'll rebase this after #14491 is merged

Sjors · 2019-02-15T09:00:53Z

Needs rebase

gwillen · 2019-04-08T22:38:41Z

I am strongly interested in this for the usability of my offline-signing work, and will give it a look.

gwillen · 2019-04-08T22:41:46Z

Could you rebase now that 14491 is in? It will clean up the diff a lot. Then I'll take a look.

meshcollider · 2019-04-09T11:35:49Z

Rebased :)

gwillen · 2019-04-09T22:17:00Z

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.)

jnewbery

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).

jnewbery · 2019-05-02T21:36:00Z

test/functional/wallet_importmulti.py

Comment above (# Test importing of a ranged descriptor without keys) needs to be updated to something like # Test importing of a ranged descriptor with xpriv

jnewbery · 2019-05-02T21:36:40Z

test/functional/wallet_importmulti.py

This isn't testing anything (the 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

sipa · 2019-05-09T23:36:16Z

ACK code changes, but I think the tests don't cover much.

ariard

Tested the following descriptors :

pkh(m/0h/0h/*h), range[1-3]
pkh(m/0h/0h/0h)
sh(wpkh(m/0h/0h/*h), range[1-3]
sh(wpkh(m/0h/0h/0h)
wpkh(m/0h/0h/0h)
wpkh(m/0h/0h/*h), range[1-3]
wsh(multi(2, m/0h/1h/0h, m/0h/1h/0h)))

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) ?

ariard · 2019-05-10T01:05:55Z

src/wallet/rpcdump.cpp

nit: Maybe take opportunity to add importmulti as a supporting RPC in descriptors.md.
You may also add a descriptor import example in importmulti

sipa · 2019-05-10T01:14:43Z

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?

ariard · 2019-05-10T02:49:02Z

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": [
]
}

sipa · 2019-05-10T03:02:31Z

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).

ariard · 2019-05-10T12:40:53Z

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

Sjors · 2019-05-10T14:28:42Z

I'd like to see more tests. I tried a similar example as @ariard, but having some difficulty:

src/bitcoin-cli getdescriptorinfo "wsh(multi(2,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf))"
{
  "descriptor": "wsh(multi(2,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA))#458rdgch",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}

I can derive addresses using the public key version with checksum returned above:

src/bitcoin-cli deriveaddresses "wsh(multi(2,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA,tpubD6NzVbkrYhZ4Y4nJe76yTeitEd6P3BuEpg7XanMcP3dWxW2Bky4QP8koTBc69APLZLNHRiJwnTqzumsjpUzDzBKpgVbQmFExramEMJRDyBA))#458rdgch"
[
  "tb1qxhuy9m6uczvc5l5m4dyx8kvplkf0hfwru8cd82yqrc74g6e8rlmsug2jdx"
]

But I can't import it (in a blank wallet):

src/bitcoin-cli importmulti  '[{ "desc" : "wsh(multi(2,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf,tprv8ZgxMBicQKsPebkWkTSP4F4mfbaSsriLFNWkJGKJxmq881mR8aEpCe8wH1V5ZGSWAj3H15TCfMWKdhG136ysuRfYRq5YuLDNYLxvPsaqzxf))", "timestamp" : 0, "keypool": false }]'
[
  {
    "success": false,
    "error": {
      "code": -5,
      "message": "Descriptor is invalid"
    }
  }
]

My guess is that it needs a checksum. See related discussion in #15740.

sipa · 2019-05-10T17:29:54Z

@Sjors Yes, importmulti requires descriptors with a checksum. See #15986.

Sjors · 2019-05-12T12:14:39Z

@sipa in particular, it requires the checksum based on the private key (added in #15986), not the checksum based on public key currently returned by getdescriptorinfo.

promag · 2019-05-13T18:05:51Z

@meshcollider please update OP as the base PR is already merged.

meshcollider · 2019-06-06T08:11:10Z

Fixed the test and amended the comment pointed out by John

ryanofsky

utACK b6dc630019dce4a0852c5c02fd6154c773ca5ebe. Previous reviews asked for more tests: #15024 (comment) and #15024 (comment), but it's unclear what other kinds of tests would be helpful here.

ryanofsky · 2019-06-06T09:14:30Z

src/script/descriptor.cpp

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.

meshcollider · 2019-06-06T10:05:04Z

@ryanofsky done, thanks!

jnewbery · 2019-06-06T10:35:55Z

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:

That the subscript wpkh(pubkey) is spendable for paths /0 and /1 (tests the if (m_script_arg) path in ExpandPrivate()).
Import a descriptor with a WIF private key and check that the address is spendable (see https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#reference for all key types supported by descriptors). (tests GetPrivKey() in ConstPubkeyProvider).
import a ranged descriptor with key origin information and check that the paths are spendable. Tests the for (auto entry : m_path) path in BIP32PubkeyProvider:: GetPrivKey().

meshcollider · 2019-06-07T08:11:19Z

Thanks for the suggestions @jnewbery, I've added the first two tests you suggested, but I'm unsure about the third. The first test already tests the BIP32PubkeyProvider derivation. Did you mean OriginPubkeyProvider? OriginPubkeyProvider just calls the BIP32 derivation though so I don't think it needs a separate test

achow101

Tested ACK 2857bc4

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.

meshcollider · 2019-06-07T13:36:03Z

@achow101 done

achow101 · 2019-06-07T13:37:13Z

ACK 53b7de6

Squash yo commits

53b7de6 Add test for dumping the private key imported from descriptor (MeshCollider) 2857bc4 Extend importmulti descriptor tests (MeshCollider) 81a884b Import private keys from descriptor with importmulti if provided (MeshCollider) a4d1bd1 Add private key derivation functions to descriptors (MeshCollider) Pull request description: ~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. ACKs for commit 53b7de: achow101: ACK 53b7de6 Tree-SHA512: c060bc01358a1adc76d3d470fefc2bdd39c837027f452e9bc4bd2e726097e1ece4af9d5627efd942a5f8819271e15ba54f010b169b50a9435a1f0f40fd1cebf3

…escriptor 53b7de6 Add test for dumping the private key imported from descriptor (MeshCollider) 2857bc4 Extend importmulti descriptor tests (MeshCollider) 81a884b Import private keys from descriptor with importmulti if provided (MeshCollider) a4d1bd1 Add private key derivation functions to descriptors (MeshCollider) Pull request description: ~This is based on bitcoin#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. ACKs for commit 53b7de: achow101: ACK 53b7de6 Tree-SHA512: c060bc01358a1adc76d3d470fefc2bdd39c837027f452e9bc4bd2e726097e1ece4af9d5627efd942a5f8819271e15ba54f010b169b50a9435a1f0f40fd1cebf3

…iptor Summary: 53b7de629d3d9281dc6f8fa10e32c4cdec59c140 Add test for dumping the private key imported from descriptor (MeshCollider) 2857bc4a64cc8dc7914bc984ac878397ac64292d Extend importmulti descriptor tests (MeshCollider) 81a884bbd0dbee108d11776794d9627ca07504aa Import private keys from descriptor with importmulti if provided (MeshCollider) a4d1bd1a29be2dcc5e00c63b6b41916b1c466de0 Add private key derivation functions to descriptors (MeshCollider) Pull request description: ~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. --- Backport of Core [[bitcoin/bitcoin#15024 | PR15024]] Test Plan: ninja all check check-functional ~~afaict, either this functionality is unavailable for legacy addresses or Core didn't add tests aside from sh(wpkh()) descriptors~~ Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: PiRK, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7804

meshcollider added the Wallet label Dec 21, 2018

meshcollider mentioned this pull request Dec 21, 2018

Allow descriptor imports with importmulti #14491

Merged

Sjors reviewed Jan 7, 2019

View reviewed changes

DrahtBot added the Needs rebase label Feb 1, 2019

Sjors mentioned this pull request Feb 15, 2019

[wallet] allow adding pubkeys from imported private keys to keypool #15414

Closed

meshcollider removed the Needs rebase label Apr 9, 2019

jnewbery reviewed May 2, 2019

View reviewed changes

ariard reviewed May 10, 2019

View reviewed changes

ryanofsky reviewed Jun 6, 2019

View reviewed changes

meshcollider added 2 commits June 6, 2019 22:03

Add private key derivation functions to descriptors

a4d1bd1

Import private keys from descriptor with importmulti if provided

81a884b

meshcollider requested a review from achow101 June 6, 2019 13:45

Extend importmulti descriptor tests

2857bc4

achow101 approved these changes Jun 7, 2019

View reviewed changes

Add test for dumping the private key imported from descriptor

53b7de6

laanwj merged commit 53b7de6 into bitcoin:master Jun 7, 2019

meshcollider deleted the 201812_descriptor_keys branch June 7, 2019 13:52

Sjors mentioned this pull request Aug 14, 2019

descriptor: fix missed m_script_arg arg renaming in #14934 #16609

Merged

bonomat mentioned this pull request Apr 9, 2020

Bitcoin shows incorrect balances comit-network/comit-js-sdk#177

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Allow specific private keys to be derived from descriptor #15024

Allow specific private keys to be derived from descriptor #15024

Uh oh!

Conversation

meshcollider commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 1, 2019

Uh oh!

meshcollider commented Feb 5, 2019

Uh oh!

Sjors commented Feb 15, 2019

Uh oh!

gwillen commented Apr 8, 2019

Uh oh!

gwillen commented Apr 8, 2019

Uh oh!

meshcollider commented Apr 9, 2019

Uh oh!

gwillen commented Apr 9, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery May 2, 2019

Choose a reason for hiding this comment

Uh oh!

meshcollider Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 9, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard May 10, 2019

Choose a reason for hiding this comment

Uh oh!

sipa commented May 10, 2019

Uh oh!

ariard commented May 10, 2019

Uh oh!

sipa commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented May 10, 2019

Uh oh!

Sjors commented May 12, 2019

Uh oh!

promag commented May 13, 2019

Uh oh!

meshcollider commented Jun 6, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Jun 6, 2019

Uh oh!

jnewbery commented Jun 6, 2019

Uh oh!

meshcollider commented Dec 21, 2018 •

edited

Loading

DrahtBot commented Dec 24, 2018 •

edited

Loading

meshcollider Jun 6, 2019 •

edited

Loading

sipa commented May 10, 2019 •

edited

Loading

ariard commented May 10, 2019 •

edited

Loading

Sjors commented May 10, 2019 •

edited

Loading