[WIP] descriptor based wallet serialization and import #15487

Sjors · 2019-02-26T21:33:52Z

Introduces a non-backwards compatible wallet type which contains a set of output descriptors.

A new RPC method importdescriptor works like importmulti but for one descriptor at a time and with fewer options.

Each wallet descriptor has a purpose: current receive, current change, archive (receive & change)

The current receive and change purposes can have one descriptor with address type bech32 and one with address type base58. It builds on top of #15590 for that.

getnewaddress finds correct receive descriptor based on address type, which must be either bech32 or legacy (base58).

Usage:

bitcoin-cli createwallet true true true
bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#...."
bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#..." '{"internal": true}, "timestamp": "now"}' 
bitcoin-cli getnewaddress "SegWit"
bitcoin-cli importdescriptor "sh(wpkh([00000000/49h/1h/0h]tpub.../1/*))#...."
bitcoin-cli getnewaddress "Pre SegWit" legacy
bitcoin-cli dumpwallet "dump" # only way to see the descriptors at the moment

This initial version will have several limitations (ignoring the TODO list below). It paves the way towards usage with hardware wallets in #14912.

only available through createwallet followed by importdescriptor
no upgrade path yet
no private keys allowed
doesn't serialise descriptor cache

TODO:

fix methods that deal with labels
fix signmessage
fix getrawchangeaddress
improve getwalletinfo
decide on descriptor serialization (currently stores a string)

For followup PR:

(re)scan wallet transactions matching descriptor
transaction creation (e.g. get change address from change descriptor)
private key support
multisig support

DrahtBot · 2019-02-26T21:36:48Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15463 (rpc: Speedup getaddressesbylabel by promag)
#15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)
#15129 (rpc: Added ability to remove watch only addresses by benthecarman)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
#15006 (Add option to create an encrypted wallet by achow101)
#13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
#12419 (Force distinct destinations in CWallet::CreateTransaction by promag)

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.

practicalswift · 2019-02-28T20:10:27Z

src/wallet/wallet.h

Nit: ;; at end of line :-)

practicalswift · 2019-02-28T20:12:36Z

src/wallet/wallet.cpp

Could be const?

practicalswift · 2019-02-28T20:13:18Z

src/wallet/wallet.cpp

Nit: Just return LoadDescriptor(nID, std::move(desc));?

sipa · 2019-02-28T20:14:48Z

@practicalswift There is absolutely no point in giving coding nits on a PR that is just at a proof of concept stage like this.

practicalswift · 2019-02-28T21:10:30Z

@sipa Oh, personally I'm very happy as a PR author when reviewers take time to point out areas of improvement (large or small) or small mistakes in my code also in the WIP stage. In the event that I find a particular nit irrelevant or premature I would simply hit "Resolve conversation" and be done with it.

What do you think @Sjors?

practicalswift · 2019-02-28T21:50:15Z

Perhaps "Draft pull requests" could be used to opt out of any code feedback:

When you create a pull request, you can choose to create a pull request that is ready for review or a draft pull request. […] When you're ready to get feedback on your draft pull request, you can mark your pull request as ready for review.

Either way it would be nice to have something something similar to the developer notes for review work with clearly stated rationales for the guidelines.

Sjors · 2019-03-01T12:05:55Z

I don't mind and usually just fix it in the next update, but whether it's useful depends on the type of nit.

Semi columns, whitespace, consts and return value stuff feedback is not useful imo, because those things are likely to be moved around. I suggest holding on off on those until I remove the WIP label. My own standard for WIP is anything that compiles and works in the happy-case scenario.

What I find more useful is help like "don't use unique_ptr in this way", or suggestions anywhere I put "TODO: this is terrible".

Sjors · 2019-03-01T12:07:30Z

src/wallet/walletdb.cpp

@practicalswift e.g. this is the kind of thing I find feedback more useful on in the WIP stage

ryanofsky · 2019-03-01T15:43:44Z

I took a look at this, and it seems well structured. I'm curious what the first use-case will be. Or I guess what's the minimum thing that needs that needs to be implemented here for this to be useful for hardware wallets and #14912? Current state as of df794753284cc1bd991a5cdf0bd747fffb1db3b9 seems to be that a descriptor wallet is a blank wallet that you're allowed to import public key descriptors to, without them being used for anything yet.

It does seem to me that the flag stuff might be overkill. I don't think we need a new flag or changes to createwallet now that we already have blank wallets. It looks like WALLET_FLAG_DESCRIPTOR_WALLET just creates a lot of new branches all over the code that don't need to exist or could be replaced with !m_descriptors.empty().

Sjors · 2019-03-01T16:11:40Z

The reason for the flag is to prevent "legacy" keys from getting mixed with descriptors. Maybe later on I discover that such mixing is fine, then I can drop the flag.

Also note that the blank flag goes away when you add a key. Which reminds me that it should also go away when you add a descriptor; that's missing in this version.

src/wallet/walletdb.h

importdescriptor replaces importmulti for descriptor based wallets.

DrahtBot · 2019-04-17T19:24:06Z

Needs rebase

Sjors · 2019-06-06T09:30:08Z

Closing in favor of @achow101's #15764 which has better momentum.

fanquake added the Wallet label Feb 26, 2019

Sjors force-pushed the 2019/02/descriptor-wallet branch 3 times, most recently from 6457e69 to df79475 Compare February 28, 2019 13:11

practicalswift reviewed Feb 28, 2019

View reviewed changes

src/wallet/wallet.h Outdated

Copy link

Contributor

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ;; at end of line :-)

practicalswift reviewed Feb 28, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be const?

practicalswift reviewed Feb 28, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Just return LoadDescriptor(nID, std::move(desc));?

Sjors commented Mar 1, 2019

View reviewed changes

src/wallet/walletdb.cpp Outdated

Copy link

Member Author

Sjors Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@practicalswift e.g. this is the kind of thing I find feedback more useful on in the WIP stage

DrahtBot added the Needs rebase label Mar 4, 2019

Sjors force-pushed the 2019/02/descriptor-wallet branch from df79475 to 7cbf31c Compare March 5, 2019 11:59

DrahtBot removed the Needs rebase label Mar 5, 2019

This was referenced Mar 9, 2019

Make OutputType consistent with Descriptor and return it #15567

Closed

[WIP] External signer support (e.g. hardware wallet) #14912

Closed

Descriptor: add GetAddressType() and IsSegWit() #15590

Closed

Sjors added 9 commits March 13, 2019 13:25

[build] add IO support for Boost::Optional

b5d67b2

Add AddressType (base58, bech32)

74a893f

Descriptor: add GetAddressType()

4371f3a

[wallet] add mandatory flag for descriptor based wallet

2489c72

[rpc] createwallet: add descriptor argument

c61a451

[wallet] descriptor wallet must be created blank

0eacc98

[test] createwallet: descriptor wallet must have private keys disabled

3ef7a78

[wallet] cannot upgrade to descriptor wallet

6ea2d2b

[wallet] descriptor based wallets do not have a keypool

a750012

practicalswift reviewed Mar 20, 2019

View reviewed changes

src/wallet/walletdb.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/02/descriptor-wallet branch 2 times, most recently from 2adbe65 to aa03f8e Compare March 20, 2019 19:39

Sjors added 4 commits March 21, 2019 11:30

[wallet] add descriptor (de)serialization

7e01e28

[rpc] add importdescriptor

d3fb62c

importdescriptor replaces importmulti for descriptor based wallets.

[rpc] dumpwallet: dump descriptors

c3d226b

[rpc] importwallet: no descriptor import

fff1afd

Sjors force-pushed the 2019/02/descriptor-wallet branch from aa03f8e to a29ff2c Compare March 21, 2019 10:31

Sjors added 3 commits March 21, 2019 19:07

[wallet] descriptor address serialization + derivation

b8ff7b1

[rpc] getnewaddress: support descriptor wallet

8debda3

[rpc] dumpwallet: add descriptor addresses

ee86df4

Sjors force-pushed the 2019/02/descriptor-wallet branch from e192f02 to 4e4063d Compare March 21, 2019 18:18

Sjors added 10 commits March 21, 2019 19:39

[wallet] add FindDescriptorAddress

02ae76e

[rpc] getaddressinfo: support descriptor wallets

1f823ba

[test] add descriptor wallet multisig tests

2e5e977

[rpc] addmultisigaddress: no descriptor support

542b1a7

[rpc] dumpprivkey and importprivkey: no descriptor support

a1ae065

[rpc] importaddress and importpubkey: no descriptor support

06f23b3

[rpc] keypoolrefill: no descriptor support

88968b6

[wallet] label helpers

3fc3fe2

[rpc] getaddressesbylabel descriptor wallet support

1238f69

[rpc] listaddressgroupings: descriptor wallet support

ad60f1e

Sjors force-pushed the 2019/02/descriptor-wallet branch from 4e4063d to ad60f1e Compare March 21, 2019 18:41

Sjors mentioned this pull request Apr 12, 2019

Native descriptor wallets #15764

Closed

DrahtBot added the Needs rebase label Apr 17, 2019

Sjors closed this Jun 6, 2019

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[WIP] descriptor based wallet serialization and import #15487

[WIP] descriptor based wallet serialization and import #15487

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 28, 2019

Uh oh!

practicalswift commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Sjors Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 1, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Apr 17, 2019

Uh oh!

Sjors commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Sjors commented Feb 26, 2019 •

edited

Loading

DrahtBot commented Feb 26, 2019 •

edited

Loading

practicalswift commented Feb 28, 2019 •

edited

Loading

practicalswift commented Feb 28, 2019 •

edited

Loading

Sjors commented Mar 1, 2019 •

edited

Loading

Sjors commented Mar 1, 2019 •

edited

Loading