Native descriptor wallets

achow101 commented at 8:38 pm on April 6, 2019: member

Introducing the wallet of the glorious future: native descriptor wallets. With native descriptor wallet, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor.

Descriptor wallets will now store only keys, keymetadata, and descriptors. Keys are derived from descriptors but those keys are also stored in order to make signing work faster and be less complicated. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 keys and scriptPubKeys pregenerated. This has a side effect of making wallets very large since 6000 keys are generated by default, instead of the current 2000. This can probably be improved in the future as we probably don’t need so many addresses for each address type.

Descriptors can also be imported with a new importdescriptor RPC.

Native descriptor wallets also redefines how ismine and watchonly work. Ismine is changing to the simpler model of “does this scriptPubKey exist in this wallet”. To facilitate this, all of the scriptPubKeys for all of the descriptors are computed on wallet loading. A scriptPubKey is considered ISMINE_SPENDABLE if it appears in the set of scriptPubKeys for the wallet. Because of this ismine change, watchonly is also redefined. A wallet can no longer contains watchonly things and non-watchonly things. Instead wallets are either watchonly (by having private keys disabled) or not. There is no mixing of watchonly and non-watchonly in a wallet. Some tests that relied on watchonly behavior had to be removed (i.e. part of feature_segwit.py)

Additionally several RPCs related to importing and dumping data from a wallet are incompatible with descriptor wallets. These RPCs (addmultisigaddress, importaddress, importpubkey, importmulti, importprivkey, and dumpprivkey) are disabled for normal use.

This PR is built on #15741 for batched writing to the wallet so TopUpKeyPool works faster, and on #15761 for upgrading wallets with a RPC.

achow101 force-pushed on Apr 6, 2019

DrahtBot added the label GUI on Apr 6, 2019

DrahtBot added the label RPC/REST/ZMQ on Apr 6, 2019

DrahtBot added the label Tests on Apr 6, 2019

DrahtBot added the label Utils/log/libs on Apr 6, 2019

DrahtBot added the label Wallet on Apr 6, 2019

DrahtBot commented at 11:07 pm on April 6, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15986 (Add unmodified-descriptor-with-checksum to getdescriptorinfo by sipa)
#15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
#15450 ([GUI] Create wallet menu option by achow101)
#15024 (Allow specific private keys to be derived from descriptor by meshcollider)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)

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.

jnewbery commented at 3:40 pm on April 8, 2019: member

Concept ACK

Unfortunately importmulti is pretty much incompatible with importdescriptors, so tests that use importmulti are either removed (e.g. wallet_importmulti.py) or changed to use importdescriptors.

We’ll need to support non-descriptor based wallets for some time to come. I don’t think these tests should be removed.

achow101 commented at 4:56 pm on April 8, 2019: member

We’ll need to support non-descriptor based wallets for some time to come. I don’t think these tests should be removed.

There’s currently no way to create a wallet with older wallet versions. Since descriptor wallets are used by default, the tests won’t work with importmulti. However if we do add test functionality for testing old wallet files, the commit that removes wallet_importmulti.py can be reverted.

jnewbery commented at 5:01 pm on April 8, 2019: member

if we do add test functionality for testing old wallet files, the commit that removes wallet_importmulti.py can be reverted.

I think that’s a hard requirement. Native descriptor wallet is such a large change that we need to maintain full test coverage of the old wallet version.

DrahtBot added the label Needs rebase on Apr 9, 2019

ryanofsky commented at 1:59 pm on April 9, 2019: member

I had a look over the PR and it’s surprisingly clean and simple. But while I understand the impulse to want to replace old code with new code and drop support for things like creating wallets in the current format, I think the PR could be actually simpler, and users would be better off if this took a more conservative approach and just added descriptor functionality as a new optional feature alongside existing functionality, rather than trying to:

add new descriptor support
add an upgrade function
replace old code and tests

all in a single PR. It seems to me if this PR just stuck to (1) and saved (2) and (3) for later followup this would be easier to review, and we would have more opportunity to iterate and hammer out any problems with the new descriptor code and design while leaving existing functionality intact.

Andrew could correct me if I’m wrong, but I think in practice doing what I’m suggesting wouldn’t involve big changes to the PR. Mostly just restoring removed code, and maybe copying and updating existing python tests instead of updating them in place. Or adding –descriptor options to the python tests and running both variants.

achow101 commented at 2:33 pm on April 9, 2019: member

Really the only reason 2 and 3 were needed is because descriptor wallets bumped the wallet version number. I could instead make it a wallet flag and make descriptors something that users can choose to enable or disable in createwallet. However I felt it would be more sensible to make this a new wallet version since it does fundamentally change the definitions of ismine and watchonly.

jnewbery commented at 2:48 pm on April 9, 2019: member

@practicalswift - it’s too early to be adding nit code style comments when the concept and approach is still being discussed. Please hold back until concept discussion is over.

practicalswift commented at 3:07 pm on April 9, 2019: contributor

@jnewbery OK! @achow101 Let me know if/when you want me to review this PR :-)

laanwj commented at 8:56 am on April 10, 2019: member

Impressive work. Concept ACK.

Sjors commented at 7:37 pm on April 12, 2019: member

Obvious concept ACK. See also my (partial) attempt at #15487. We discussed some differences in todays wallet meeting on IRC. Will study this PR in more detail later.

jnewbery commented at 8:17 pm on April 12, 2019: member

Lots of discussion on this in today’s IRC meeting: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-04-12-19.00.log.html#l-46

Conclusion is that @achow101 will rework this PR to use feature flags, restore the tests and remove the upgrade code.

jonasschnelli commented at 7:50 am on April 16, 2019: contributor

Obvious concept ACK

Sjors commented at 1:31 pm on April 16, 2019: member

Concept ACK on:

adding a descriptor id (sha256 of the string); alternatively we could also the ID wallet based, in which case a simple auto-increment int could suffice
the WalletDescriptor class, but:
- I would drop range_start and range_end unless there’s a good reason for them
- serialization needs versioning (CURRENT_VERSION=VERSION_BASIC)
a new importdescriptors RPC method (as opposed to overloading importmulti)
dumpwallet dumps descriptors
- individual (labeled) addresses should refer to their descriptor id (or just printed directly after their descriptor)
std::map<OutputType, DescriptorID> m_..._descriptors to track descriptors by output type
- #15590 (or some variation thereof) would let you check the type upon import

We need to track if a descriptor is change / receive, and whether it’s active (like the keypool) or archived. This PR handles that with SetPrimaryDescriptor, etc. My approach was to add a purpose int to each descriptor DESCRIPTOR_PURPOSE_[RECEIVE,CHANGE]_[CURRENT,ARCHIVE]. No strong preference. I would probably rename Primary to Receive though.

One thing I’m not a fan of:

LoadDescriptor expands a descriptor and then throws keys and script into the global wallet (e.g. AddScriptPubKey); I prefer if descriptor objects handle their own IsMine stuff.

To keep this PR small I suggest, in additon to what @jnewbery summarizes:

don’t add private key support (I do think we should have a WIP PR that adds private key support, to get a rough feel for what needs to happen)

achow101 force-pushed on Apr 16, 2019

achow101 commented at 2:48 pm on April 16, 2019: member

I’ve made the changes that were discussed last Friday. I’ve also split up some of the commits so that they are smaller and easier to review.

I’ll be working on adding tests for all of this.

achow101 force-pushed on Apr 16, 2019

DrahtBot removed the label Needs rebase on Apr 16, 2019

DrahtBot added the label Needs rebase on Apr 17, 2019

achow101 force-pushed on Apr 22, 2019

achow101 commented at 3:32 am on April 22, 2019: member

Rebased, fixed a couple of bugs, and added some tests. I’ll be adding more tests soon, particularly for importdescriptors.

don’t add private key support (I do think we should have a WIP PR that adds private key support, to get a rough feel for what needs to happen)

No. That wouldn’t make this any smaller, and I think that having private key support is an important part of descriptor wallets.

LoadDescriptor expands a descriptor and then throws keys and script into the global wallet (e.g. AddScriptPubKey); I prefer if descriptor objects handle their own IsMine stuff.

Expanding the descriptors and adding scripts and keys to the global wallet allows us to have CKeystore not have to know about descriptors (and I don’t think that it should know what a descriptor is).

achow101 force-pushed on Apr 22, 2019

DrahtBot removed the label Needs rebase on Apr 22, 2019

Sjors commented at 10:35 am on April 24, 2019: member

If each descriptor has its own CKeystore then CKeystore also doesn’t have to know about descriptors. Descriptor wallets give us an opportunity to rethink IsMine and watch-only behavior. I suspect that using a global wallet keys and scripts makes changing that more difficult because it requires more backwards compatibly. But I could be wrong.

achow101 force-pushed on Apr 26, 2019

DrahtBot commented at 1:35 pm on April 27, 2019: member

DrahtBot added the label Needs rebase on Apr 27, 2019

Output a descriptor in createmultisig e0c505d28a

Introduce m_set_scriptPubKey and check it in IsMine()

Adds a set to CKeystore which contains scriptPubKeys being tracked.
Return ISMINE_SPENDABLE for anything in that set.

7453e3e5f3

Introduce DescriptorID e067ef9e4a

Introduce WalletDescriptor and writing it to the wallet e7f7af6b4e

Add functions to read and write descriptors from the wallet file

Adds functionality to read and write descriptors and related metadata
to and from the wallet file. Descriptos will be loaded into memory
on wallet loading along with fields indicated which descriptors to
use in normal wallet use.

f1ec0d7028

Add wallet flag for descriptor wallets ab1e0db0ea

Add private key derivation functions to descriptors f63b1c9398

achow101 commented at 10:34 am on June 5, 2019: member

rebased

achow101 force-pushed on Jun 5, 2019

fanquake removed the label Needs rebase on Jun 5, 2019

Add importdescriptors RPC bbe8bf1be2

Add descriptors to dumpwallet and importwallet ac5905353f

Disable RPCs that are incompatible with descriptor wallets 054498dd92

Redefine IsMine() for descriptor wallets

Descriptor wallets are the only wallets that have a set of scriptPubKeys.
For such wallets, IsMine() is redefined to be effectively a boolean that
indicates whether a scriptPubKey is in m_set_scriptPubKeys

60a93ae619

Store additional information with a scriptPubKey in the wallet

Store the id of the descriptor and the position in that descriptor that
the scriptPubKey was derived from.

c4d18618f4

Update descriptors when scriptPubKeys are found to be used c1c45c1258

Function to generate a new descriptor for use for address generation ef5a2a193a

Function to generate addresses from descriptors 22c9f21bbc

Get new addresses from descriptors for descriptor wallets 795608e516

Generate descriptors for descriptor wallets 4fe18b9b7c

Have setting the HD seed of a descriptor wallet generate the descriptors 95fe8337b2

Compute keypoolsize correctly for descriptor wallets 8e61da1116

achow101 force-pushed on Jun 5, 2019

Allow createwallet to create descriptor wallets b3b22dc900

Add whether a wallet is a descriptor wallet to getwalletinfo 57e4b35a9e

Functional tests for descriptor wallets 8634a6bfe7

achow101 force-pushed on Jun 5, 2019

achow101 commented at 2:56 pm on June 5, 2019: member

A lot of this will be restructured and changed in the near future. However a lot of the new descriptor specific functions will probably stay the same. So for now, ignore the following commits:

7453e3e5f312b1b32d677451ae8064745a848b41
60a93ae61913e5ca9f04a0f16e75dca6b1f96975
95fe8337b270e2457b13b8a9b72e08b30417e2d5
8e61da11161751f30abc757e6a14af714ebc6a29
4fe18b9b7c6502fbafc83289aac7a55d897bbd94
795608e51666769596493d1ea527e286b23db468

DrahtBot commented at 2:39 pm on June 7, 2019: member

DrahtBot added the label Needs rebase on Jun 7, 2019

achow101 commented at 9:41 pm on June 18, 2019: member

Closing this for now while the wallet restructure takes place.

achow101 closed this on Jun 18, 2019

jnewbery added this to the "PRs" column in a project

jnewbery moved this from the "PRs" to the "Design" column in a project

laanwj removed the label Needs rebase on Oct 24, 2019

ysangkok commented at 4:13 pm on February 21, 2020: none

I think this was superseded by #16528 in case anybody is wondering why it wasn’t reopened.

DrahtBot locked this on Feb 15, 2022

Native descriptor wallets #15764

Conflicts