rpc: add path to gethdkey #22341

Sjors · 2021-06-25T15:12:03Z

Depends on #26728.

This PR expands the wallet RPC gethdkey introduced in #26728. It takes a BIP32 path as argument and returns the xpub, along with the master key fingerprint.

To test this PR:

create a regular descriptor wallet
call getxpub m/86h/0h/0h
call listdescriptors, compare the xpub in the tr() descriptor with the previous step

Bigger picture

This paves the way for using Bitcoin Core as one signer in a multisig setup. It simplifies the proposed flow in #22067.

The eventual flow would be:

Create a blank wallet with a seed (either with no descriptors, or where its single key descriptors are not active, so they don't get used when calling getnewaddress).
Call getxpub m/87h/0h/0h (as suggested in BIP 87)
(Manually, with Specter or with a simple Python utility - TBD): craft a multisig descriptor containing this xpub
Call importdescriptors which will allow the import if its own fingerprint is recognized (and after checking the xpub)
The usual flow with getnewaddress, send and walletprocesspsbt (and their GUI equivalents)

This PR makes step (2) possible.

Step (1) would require a followup, because blank wallets currently can't have a seed. Step 4 would also require a followup, so that importdescriptors treats an xpub as if it was an xpriv, after checking that it can derive the xpub from the seed (that matches the fingerprint)

On the GUI side a wizard could perform the above steps and guide the user, where they either:

export their own xpub and then import a descriptor from somewhere else (performing some sanity checks); or
import xpub(s) from other wallets, compose a multisig and get the descriptors for export; or
get xpub(s) from connected hardware devices and configure a multisig (with or without a key on their machine)

test/functional/wallet_getxpub.py

DrahtBot · 2021-06-25T23:39:30Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fjahr, Zero-1729, JeremyRubin, aureleoules, jonatack
Approach ACK	w0xlt
Stale ACK	lsilva01, mjdietzx, Rspigler

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29154 (tests: improve wallet multisig descriptor test and docs by mjdietzx)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#29016 (RPC: add new listmempooltransactions by niftynei)
#28976 (wallet: Fix migration of blank wallets by achow101)
#28868 (wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28610 (wallet: Migrate entire address book entries to watchonly and solvables too by achow101)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28192 (ParseHDKeypath: support h as hardened marker by Sjors)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

jonatack · 2021-06-26T17:19:31Z

Thanks for working on this!

src/wallet/wallet.cpp

Rspigler · 2021-06-30T06:01:18Z

Thanks so much for continuing to work on this! Love the use of BIP 87 :)

Call importdescriptors which will allow the import if its own fingerprint is recognized (and after checking the xpub)

Yes, important to check the xpub also, not just the fingerprint, which is easy to spoof

export their own xpub and then import a descriptor...

import xpub(s) from other wallets, compose a multisig...

get xpub(s) from connected hardware devices...

I agree that the above 3 encompasses all use cases/needs for Bitcoin Core being a coordinator and a signer in a multisig setup.

importdescriptors should treat an xpub as if it was an xpriv, after checking that it can derive the xpub from the seed (that matches the fingerprint); it needs to copy the seed from a sibling descriptor??

Can you explain this to me more?

As for the blank wallets, why do we need to be working from blank wallets, and if xprivs are derived from seeds, how can a blank wallet have a seed?

Sjors · 2021-07-01T13:39:13Z

It depends on how we want to define blank. It would be empty except for a seed, so without any descriptors in it.

If a wallet has a seed and you call importdescriptors on it with some multisig descriptor, it should check the fingerprint(s) and path(s) mentioned in that descriptor. It should then derive the xpriv & xpub for that path, from the wallet seed, and check if it matches the descriptor xpub. In that case, the wallet should replace that xpub with the xpriv, as if the user provided it upon import. This avoid the dangerous act of exporting and re-importing an xpriv. That way, if you want to sign a transaction, the wallet knows it can provide one of the signatures.

Rspigler · 2021-07-06T18:46:00Z

Thank you, makes a lot of sense!

And what about sibling descriptors?

lsilva01

Code Review ACK 1ff7a25

src/wallet/wallet.cpp

test/functional/wallet_getxpub.py

mjdietzx · 2021-07-14T21:30:59Z

Concept ACK

I'll be sure to update the tests/docs in #22067 to use this when this PR gets merged

fjahr · 2021-08-01T19:11:08Z

Concept ACK

Zero-1729 · 2021-08-02T06:29:18Z

Concept ACK

Sjors · 2021-10-21T18:24:24Z

I managed to reconstruct the master extended private key, by combining the key part of the descriptor CExtKey with the chaincode part of the CExtPubKey stored inside that descriptor. This code is absolute trash though. I'll try to reuse some of @achow101's work in https://github.com/achow101/bitcoin/commits/upgrade-to-tr-1 which tries to solve a similar problem, for adding a taproot descriptor to an existing descriptor wallet.

Best reviewed with `git show -w`

…ster The test creates a wallet on master, downgrades and encrypts the wallet. Then, it tries to open it again on master.

Sjors · 2023-12-22T10:09:53Z

I plan to re-work this on top of #29130 with a similar interface to createwalletdescriptor. That is, if you have a regular wallet with normal descriptors, it will Just Work(tm). Otherwise you need to specify which master key to use.

Initially I'll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

ryanofsky · 2023-12-23T15:01:45Z

Initially I'll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

It seems like you could just rebase this on top of #29130, since the description of the PR and new commits aren't going to change that much, they will just be based on #29130 instead of #26728. Either way seems fine, though.

Sjors · 2023-12-27T08:29:30Z

@ryanofsky building on #29130 introduces a new argument to specify which master key you intend to use.

DrahtBot · 2024-01-08T14:56:17Z

🐙 This pull request conflicts with the target branch and needs rebase.

Sjors · 2024-01-12T18:19:34Z

Given that #26728 was closed, I'll just modify this PR to build on top of #29130.

(bit busy with stratum v2 stuff, but it will happen)

Sjors · 2024-02-13T09:49:14Z

This seems more involved than a simple rebase. #29130 changes gethdkey to gethdkeys making it a less obvious choice to add a path argument to. We probably need a new RPC that's more similar to createwalletdescriptor in how it's called.

I probably won't have time for this in the near future, so I'm closing this as up for grabs.

…k to closed PR 3cd24aa doc: remove obsolete mention and link to closed PR (Marnix) Pull request description: Remove the mention and link as the PR (#22341) is closed and the description is wrong/outdated anyway. ACKs for top commit: BrandonOdiwuor: ACK 3cd24aa tdb3: ACK 3cd24aa Tree-SHA512: 5cd97029337f0cdfe81b6be9401adc4fe51ae2868f8fcadcb03828531a38380a587c32840850a924b6428f62df7d20a1e16ef7414d4078e7bb2c4e359b1fae40

Sjors commented Jun 25, 2021

View reviewed changes

test/functional/wallet_getxpub.py Outdated Show resolved Hide resolved

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 25, 2021

Sjors force-pushed the 2021/06/getxpub branch from 75d0983 to 1ff7a25 Compare June 25, 2021 19:33

Sjors mentioned this pull request Jun 25, 2021

wallet: Multi-sig flow with descriptor wallets #21278

Closed

This was referenced Jun 26, 2021

wallet: Properly support a wallet id #20205

Closed

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

Sjors mentioned this pull request Jun 26, 2021

External signer multisig support #16895

Closed

2 tasks

Sjors mentioned this pull request Jun 28, 2021

Core hot wallet with (proposed) getxpub RPC cryptoadvance/specter-desktop#1255

Open

Sjors commented Jun 28, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request Jun 29, 2021

wallet: Make a tr() descriptor by default #22364

Merged

lsilva01 reviewed Jul 10, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

test/functional/wallet_getxpub.py Outdated Show resolved Hide resolved

test/functional/wallet_getxpub.py Outdated Show resolved Hide resolved

test/functional/wallet_getxpub.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 7, 2021

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

Merged

This was referenced Aug 30, 2021

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

wallet: Avoid locking cs_wallet recursively #19833

Merged

Rspigler mentioned this pull request Sep 21, 2021

~~$1,500 Bounty for Offline Multisignature through the GUI~~ View #24861 #21071

Closed

Sjors mentioned this pull request Oct 20, 2021

MuSig2 support #23326

Closed

7 tasks

Sjors force-pushed the 2021/06/getxpub branch from 1ff7a25 to bfdf045 Compare October 21, 2021 18:22

This was referenced Oct 22, 2021

Update basic multisig test/docs to use multipath descriptor #23308

Closed

rpc: deprecate top-level fee fields in getmempool RPCs #22689

Merged

Sjors mentioned this pull request Nov 2, 2021

wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

Closed

achow101 and others added 14 commits December 11, 2023 17:01

wallet: Automatically upgrade a wallet to have global hd key

ce5495a

tests: Test for gethdkey

9ac7fe7

test: Test automatic upgrade of descriptor wallets to have hd key

5679b4a

wallet: Set global hd key for migrated wallets

1887998

test: Also do backwards compat test with encrypted wallets

af3667a

Best reviewed with `git show -w`

walletdb: Check that unencrypted and crypted keys are mutually exclusive

1bbbb73

test: Check that no unencrypted records persist after encrypting

291cb32

test: add coverage for re-opening a downgraded encrypted wallet on ma…

059d847

…ster The test creates a wallet on master, downgrades and encrypts the wallet. Then, it tries to open it again on master.

ParseHDKeypath: support h as hardened marker

c9f7127

wallet: GetExtKey()

20a522c

rpc: ParsePathBIP32 helper

24d2240

rpc: add path argument to gethdkey

259d52f

test: use getxpub in wallet_multisig_descriptor_psbt.py

a4330ba

doc: use gethdkey in basic multisig descriptor example

aebbcd4

Sjors force-pushed the 2021/06/getxpub branch from 60361ee to aebbcd4 Compare December 14, 2023 11:29

This was referenced Dec 21, 2023

test: Test that migration automatically repairs corrupted metadata with doubled derivation path #29124

Merged

wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors #29130

Merged

DrahtBot mentioned this pull request Dec 23, 2023

wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor #29136

Open

DrahtBot mentioned this pull request Dec 29, 2023

tests: improve wallet multisig descriptor test and docs #29154

Merged

DrahtBot added the Needs rebase label Jan 8, 2024

Sjors closed this Feb 13, 2024

MarnixCroes mentioned this pull request Jul 31, 2024

doc: multisig-tutorial: remove obsolete mention and link to closed PR #30556

Merged

bitcoin locked and limited conversation to collaborators Feb 12, 2025

rpc: add path to gethdkey #22341

rpc: add path to gethdkey #22341

Uh oh!

Conversation

Sjors commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bigger picture

Uh oh!

Uh oh!

DrahtBot commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

jonatack commented Jun 26, 2021

Uh oh!

Uh oh!

Rspigler commented Jun 30, 2021

Uh oh!

Sjors commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rspigler commented Jul 6, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjdietzx commented Jul 14, 2021

Uh oh!

fjahr commented Aug 1, 2021

Uh oh!

Zero-1729 commented Aug 2, 2021

Uh oh!

Sjors commented Oct 21, 2021

Uh oh!

Sjors commented Dec 22, 2023

Uh oh!

ryanofsky commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 27, 2023

Uh oh!

DrahtBot commented Jan 8, 2024

Uh oh!

Sjors commented Jan 12, 2024

Uh oh!

Sjors commented Feb 13, 2024

Uh oh!

Uh oh!

Sjors commented Jun 25, 2021 •

edited

Loading

DrahtBot commented Jun 25, 2021 •

edited

Loading

Sjors commented Jul 1, 2021 •

edited

Loading

ryanofsky commented Dec 23, 2023 •

edited

Loading