Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specific private keys to be derived from descriptor #15024

Merged
merged 4 commits into from Jun 7, 2019

Conversation

Projects
None yet
@meshcollider
Copy link
Member

commented Dec 21, 2018

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.

@meshcollider meshcollider added the Wallet label Dec 21, 2018

@meshcollider meshcollider force-pushed the meshcollider:201812_descriptor_keys branch to a84df38 Dec 24, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2018

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
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

Needs rebase
@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

I'll rebase this after #14491 is merged

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Needs rebase

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

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

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

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

@meshcollider meshcollider force-pushed the meshcollider:201812_descriptor_keys branch from a84df38 to e00c3dd Apr 9, 2019

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Rebased :)

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

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

@laanwj laanwj added this to Blockers in High-priority for review Apr 18, 2019

@jnewbery
Copy link
Member

left a comment

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

@@ -584,12 +584,12 @@ def run_test(self):
self.test_importmulti({"desc": descsum_create(desc),
"timestamp": "now",
"range": 1},
success=True,
warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
success=True)

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

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

@@ -584,12 +584,12 @@ def run_test(self):
self.test_importmulti({"desc": descsum_create(desc),
"timestamp": "now",
"range": 1},
success=True,
warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
success=True)
for address in addresses:

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

This isn't testing anything (the address loop variable in not being used, and the address tested is left over from the test above)

This comment has been minimized.

Copy link
@meshcollider

meshcollider Jun 6, 2019

Author Member

@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

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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

@ariard
Copy link
Contributor

left a comment

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

@@ -1154,12 +1154,12 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID

const UniValue& priv_keys = data.exists("keys") ? data["keys"].get_array() : UniValue();

// Expand all descriptors to get public keys and scripts.
// TODO: get private keys from descriptors too
// Expand all descriptors to get public keys and scripts, and private keys if available.

This comment has been minimized.

Copy link
@ariard

ariard May 10, 2019

Contributor

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

This comment has been minimized.

Copy link
Member

commented May 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

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

This comment has been minimized.

Copy link
Member

commented May 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

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

This comment has been minimized.

Copy link
Member

commented May 10, 2019

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

This comment has been minimized.

Copy link
Member

commented May 10, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

commented May 12, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 13, 2019

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

@meshcollider meshcollider force-pushed the meshcollider:201812_descriptor_keys branch 2 times, most recently from 3abcb13 to 30ba01a Jun 6, 2019

@meshcollider meshcollider force-pushed the meshcollider:201812_descriptor_keys branch from 30ba01a to b6dc630 Jun 6, 2019

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Fixed the test and amended the comment pointed out by John

@ryanofsky
Copy link
Contributor

left a comment

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

@@ -312,6 +323,18 @@ class BIP32PubkeyProvider final : public PubkeyProvider
}
return true;
}
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
CExtKey extkey;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 6, 2019

Contributor

In commit "Add private key derivation functions to descriptors" (ca039db)

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 meshcollider force-pushed the meshcollider:201812_descriptor_keys branch from 4105545 to 81a884b Jun 6, 2019

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@ryanofsky done, thanks!

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

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 meshcollider requested a review from achow101 Jun 6, 2019

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

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

@meshcollider meshcollider force-pushed the meshcollider:201812_descriptor_keys branch from 10f7f53 to 2857bc4 Jun 7, 2019

@achow101
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@achow101 done

@achow101

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

ACK 53b7de6

Squash yo commits

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

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jun 7, 2019

Merge #15024: Allow specific private keys to be derived from descriptor
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

@meshcollider meshcollider deleted the meshcollider:201812_descriptor_keys branch Jun 7, 2019

@meshcollider meshcollider removed this from Blockers in High-priority for review Jun 7, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 7, 2019

Merge bitcoin#15024: Allow specific private keys to be derived from d…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.