Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native descriptor wallets #15764

Open
wants to merge 27 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@achow101
Copy link
Member

commented Apr 6, 2019

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 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from e27cf04 to ef2236a Apr 6, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
  • #15778 ([wallet] Move maxtxfee from node to wallet by jnewbery)
  • #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
  • #15730 (rpc: Show scanning details in getwalletinfo by promag)
  • #15424 ([wallet] wallet-tool: command to remove key metadata by Sjors)
  • #15414 ([wallet] allow adding pubkeys from imported private keys to keypool by Sjors)
  • #15024 (Allow specific private keys to be derived from descriptor by meshcollider)
  • #15006 (Add option to create an encrypted wallet by achow101)
  • #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

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

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.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Impressive work. Concept ACK.

@Sjors

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Obvious concept ACK

@Sjors

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

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 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from ef2236a to 3e0e3c2 Apr 16, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 3e0e3c2 to da06671 Apr 16, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Apr 16, 2019

achow101 and others added some commits Apr 3, 2019

Add AddWatchOnlyWithDB and AddKeyOriginWithDB functions
AddWatchOnlyWithDB and AddKeyOriginWithDB add their respective data to
the wallet using the provided WalletBatch instead of creating a new
WalletBatch object every time. This allows for batching writes to the
database.
Batch writes for importmulti
When writing all of the imported data to the wallet, use a common
WalletBatch object so that batch writes are done and the writes
finish more quickly.

AddKeypoolPubkey is no longer needed so it is also removed
Have WalletBatch automatically flush every 1000 updates
Since it now automatically flushes, we don't need to have
UpgradeKeyMetadata count and flush separately
Move some of ProcessImport into CWallet::Import*
This maintains encapsulation of CWallet::database in the face of
batching, e.g. allows making the `WithDB` methods private.

achow101 and others added some commits Mar 26, 2019

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

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from da06671 to d9b9564 Apr 22, 2019

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from d9b9564 to 35030a2 Apr 22, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

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 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 2 times, most recently from 67ffa66 to 1ac17ca Apr 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Apr 22, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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.

@Sjors Sjors referenced this pull request Apr 24, 2019

Open

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

0 of 2 tasks complete

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 1ac17ca to 9c4a76e Apr 26, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Needs rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.