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 using DescriptorScriptPubKeyMan #16528

Open
wants to merge 52 commits into
base: master
from

Conversation

@achow101
Copy link
Member

achow101 commented Aug 2, 2019

Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using createwallet.

Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. 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 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded.

Descriptors can also be imported with a new importdescriptors RPC.

Native descriptor wallets use the ScriptPubKeyMan interface introduced in #16341 to add a DescriptorScriptPubKeyMan. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, DescriptorScriptPubKeyMan does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with disable_private_keys needs to be used for watchonly things.

A --descriptor option was added to some tests (wallet_basic.py, wallet_encryption.py, wallet_keypool.py, wallet_keypool_topup.py, and wallet_labels.py) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (importprivkey, importpubkey, importaddress, importmulti, addmultisigaddress, dumpprivkey, dumpwallet, importwallet, and sethdseed).


Depends on #18115 (which depends on #17577) and #18034.

@fanquake fanquake added the Wallet label Aug 2, 2019
@fanquake fanquake added this to PRs in Descriptor wallets Aug 2, 2019
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from acbd19c to 6f8afc0 Aug 2, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 2, 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:

  • #18163 (descriptors: Use xpub at last hardened step if possible by achow101)
  • #18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
  • #18064 (gui: Drop WalletModel dependency to RecentRequestsTableModel by promag)
  • #18034 (Get the OutputType for a descriptor by achow101)
  • #18027 ("PSBT Operations" dialog by gwillen)
  • #17953 (refactor: Abstract boost::variant out by elichai)
  • #17938 (Disallow automatic conversion between disparate hash types by Empact)
  • #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
  • #17809 (rpc: Auto-format RPCResult by MarcoFalke)
  • #17681 (wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101)
  • #17577 (refactor: deduplicate the message sign/verify code by vasild)
  • #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
  • #17264 (rpc: set default bip32derivs to true for psbt methods by Sjors)
  • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
  • #16910 (wallet: reduce loading time by using unordered maps by achow101)
  • #16710 (build: Enable -Wsuggest-override if available by hebasto)
  • #16440 (BIP-322: Generic signed message format by kallewoof)
  • #16432 (qt: Add privacy to the Overview page by hebasto)
  • #16224 (gui: Bilingual GUI error messages by hebasto)
  • #12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option 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.

@meshcollider

This comment has been minimized.

Copy link
Member

meshcollider commented Aug 2, 2019

Strong Concept ACK, this should be much nicer now it is preceded by the rework.

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 2 times, most recently from ef59fb7 to 90a631f Aug 2, 2019
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 2 times, most recently from ef63001 to 755c4d1 Aug 2, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Aug 3, 2019

Approach ACK. It looks pretty straight forward and thanks to The Box feels cleaner than the previous attempt.

I suggest that, after a bit more progress on #16341, we review this in parallel. That ensures that we don't mess up the division of labour between ScriptPubKeyMan and its subclasses.

Also concept ACK on using BIP44/49/84 for new descriptor wallets. That may need some discussion, because it means individual addresses are no longer hardened.

Some issues I found perusing the commits:

  • when you restart bitcoind and load a (watch-only) wallet the keypool is empty and getnewaddress no longer works
  • importdescriptors should no longer require a range argument (also I would prefer a singular RPC nvm that makes rescan slow)
  • when calling getnewaddress with an address type for which the wallet misses a descriptor, it returns a blank error message:
  • can you give each ScriptPubKeyManager it's own file?
  • deleting GetMetadata and Upgrade inside Introduce WalletDescriptor; should have its own commit?
  • ScriptPubKeyMap could be introduced in Implement IsMine instead of the earlier commit. Would it make sense to move this into the Descriptor class? How are infinite range descriptors handled, expanded and cached keypoolsize items at a time?
  • Am I reading this wrong or is SetupGeneration creating a fresh seed for each descriptor?
  • I'm not a fan of determining the output type in an indirect manner by expanding the descriptor Optional<OutputType> out_script_type = DetermineOutputType(scripts_temp[0], out_keys);; that information is already encoded in the descriptor. Shameless plug for #15590.
@hugohn

This comment has been minimized.

Copy link

hugohn commented Aug 12, 2019

Concept ACK.

@achow101 I just noticed that both the existing deriveaddressses & importmulti commands treat descriptor [range_start, range_end] as inclusive, which is consistent with the common understanding of the notation []. Should we update the new importdescriptors command to do the same (make range_end inclusive)?

https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L215
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcdump.cpp#L1122

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 755c4d1 to d777554 Aug 12, 2019
@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 12, 2019

Should we update the new importdescriptors command to do the same (make range_end inclusive)?

I guess so. Latest pushed should do that.

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 693c8cc to 84fb7d7 Aug 14, 2019
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 3 times, most recently from 655ea88 to a208faa Sep 4, 2019
@Sjors Sjors mentioned this pull request Sep 4, 2019
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from a208faa to f581c8c Sep 4, 2019
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 2 times, most recently from f8edc40 to 69ad25c Sep 17, 2019
@Sjors Sjors mentioned this pull request Sep 26, 2019
1 of 2 tasks complete
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 69ad25c to 98b7838 Oct 10, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 10, 2019

Due to your latest change upstream, this now complains: scriptpubkeyman.h:516:10: warning: 'CheckDecryptionKey' overrides a member function but is not marked 'override'

@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch 2 times, most recently from e184668 to 91d4efc Feb 14, 2020
@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Feb 14, 2020

I've removed the requirement for range in importdescriptors. It will give a warning and use the default keypool range.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Feb 18, 2020
…ddmultisigaddress

19a354b Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)

Pull request description:

  Give a descriptor from `createmultisig` and `addmultisigaddress`.

  Extracted from bitcoin#16528 with `addmultisgaddress` and tests added.

ACKs for top commit:
  Sjors:
    tACK 19a354b
  MarcoFalke:
    ACK 19a354b
  promag:
    Code review ACK 19a354b.
  meshcollider:
    utACK 19a354b

Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771

// Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) {
std::unique_ptr<FlatSigningProvider> keys = GetSigningProvider(psbtx.tx->vout.at(i).scriptPubKey, true);

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 19, 2020

Member

Should this be GetSolvingProvider for watch-only wallets? I'm having difficulty getting bip32 keys added to the PSBT while trying to rebase #16546 , see Sjors@8660421
(maybe I should try less of a hack and actually make that subclass...)

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 19, 2020

Member

Once #18180 is implemented that reduce at least some of my confusion :-)

achow101 and others added 11 commits Jul 11, 2019
Adds the option to use a descriptor wallet to:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py
* wallet_keypool_topup.py
* wallet_labels.py

Also runs these tests with --descriptors in test_runner
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
@achow101 achow101 force-pushed the achow101:wallet-of-the-glorious-future branch from 91d4efc to 6c1929a Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.