Native descriptor wallets #15764

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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.

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.

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.

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:

1. add new descriptor support
2. add an upgrade function
3. 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.

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.

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

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

Impressive work. Concept ACK.

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.

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.

Obvious concept ACK

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)

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.

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

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.

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

rebased

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

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

Closing this for now while the wallet restructure takes place.

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