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.

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
  • Descriptor: add GetAddressType() and IsSegWit() #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).

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:

• 7453e3e
• 60a93ae
• 95fe833
• 8e61da1
• 4fe18b9
• 795608e

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.