[WIP] descriptor based wallet serialization and import

Sjors commented at 9:33 pm on February 26, 2019: member

Introduces a non-backwards compatible wallet type which contains a set of output descriptors.

A new RPC method importdescriptor works like importmulti but for one descriptor at a time and with fewer options.

Each wallet descriptor has a purpose: current receive, current change, archive (receive & change)

The current receive and change purposes can have one descriptor with address type bech32 and one with address type base58. It builds on top of #15590 for that.

getnewaddress finds correct receive descriptor based on address type, which must be either bech32 or legacy (base58).

Usage:

0bitcoin-cli createwallet true true true
1bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#...."
2bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#..." '{"internal": true}, "timestamp": "now"}' 
3bitcoin-cli getnewaddress "SegWit"
4bitcoin-cli importdescriptor "sh(wpkh([00000000/49h/1h/0h]tpub.../1/*))#...."
5bitcoin-cli getnewaddress "Pre SegWit" legacy
6bitcoin-cli dumpwallet "dump" # only way to see the descriptors at the moment

This initial version will have several limitations (ignoring the TODO list below). It paves the way towards usage with hardware wallets in #14912.

only available through createwallet followed by importdescriptor
no upgrade path yet
no private keys allowed
doesn’t serialise descriptor cache

TODO:

fix methods that deal with labels
fix signmessage
fix getrawchangeaddress
improve getwalletinfo
decide on descriptor serialization (currently stores a string)

For followup PR:

(re)scan wallet transactions matching descriptor
transaction creation (e.g. get change address from change descriptor)
private key support
multisig support

DrahtBot commented at 9:36 pm on February 26, 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:

#15463 (rpc: Speedup getaddressesbylabel by promag)
#15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)
#15129 (rpc: Added ability to remove watch only addresses by benthecarman)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
#15006 (Add option to create an encrypted wallet by achow101)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#12419 (Force distinct destinations in CWallet::CreateTransaction by promag)

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.

fanquake added the label Wallet on Feb 26, 2019

Sjors force-pushed on Feb 27, 2019

Sjors force-pushed on Feb 28, 2019

in src/wallet/wallet.h:805 in df79475328 outdated

803@@ -800,6 +804,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
804 
805     std::map<uint256, CWalletTx> mapWallet GUARDED_BY(cs_wallet);
806 
807+    std::map<unsigned int, std::unique_ptr<WalletDescriptor>> m_descriptors GUARDED_BY(cs_wallet);;

practicalswift commented at 8:10 pm on February 28, 2019:

Nit: ;; at end of line :-)

in src/wallet/wallet.cpp:499 in df79475328 outdated

489@@ -491,6 +490,24 @@ bool CWallet::LoadWatchOnly(const CScript &dest)
490     return CCryptoKeyStore::AddWatchOnly(dest);
491 }
492 
493+bool CWallet::LoadDescriptor(const unsigned int nID, std::unique_ptr<WalletDescriptor>descriptor) {
494+    m_descriptors[nID] = std::move(descriptor);
495+    return true;
496+}
497+
498+bool CWallet::HaveDescriptor(const unsigned int nID) {

practicalswift commented at 8:12 pm on February 28, 2019:

Could be const?

in src/wallet/wallet.cpp:508 in df79475328 outdated

502+bool CWallet::AddDescriptor(const unsigned int nID, std::string descriptor, int64_t nCreateTime) {
503+    auto desc = MakeUnique<WalletDescriptor>(descriptor, nCreateTime);
504+    if (!WalletBatch(*database).WriteDescriptor(nID, desc.get())) {
505+        return false;
506+    }
507+    bool res = LoadDescriptor(nID, std::move(desc));

practicalswift commented at 8:13 pm on February 28, 2019:

Nit: Just return LoadDescriptor(nID, std::move(desc));?

sipa commented at 8:14 pm on February 28, 2019: member

@practicalswift There is absolutely no point in giving coding nits on a PR that is just at a proof of concept stage like this.

practicalswift commented at 9:10 pm on February 28, 2019: contributor

@sipa Oh, personally I’m very happy as a PR author when reviewers take time to point out areas of improvement (large or small) or small mistakes in my code also in the WIP stage. In the event that I find a particular nit irrelevant or premature I would simply hit “Resolve conversation” and be done with it.

What do you think @Sjors?

practicalswift commented at 9:50 pm on February 28, 2019: contributor

Perhaps “Draft pull requests” could be used to opt out of any code feedback:

When you create a pull request, you can choose to create a pull request that is ready for review or a draft pull request. […] When you’re ready to get feedback on your draft pull request, you can mark your pull request as ready for review.

Either way it would be nice to have something something similar to the developer notes for review work with clearly stated rationales for the guidelines.

Sjors commented at 12:05 pm on March 1, 2019: member

I don’t mind and usually just fix it in the next update, but whether it’s useful depends on the type of nit.

Semi columns, whitespace, consts and return value stuff feedback is not useful imo, because those things are likely to be moved around. I suggest holding on off on those until I remove the WIP label. My own standard for WIP is anything that compiles and works in the happy-case scenario.

What I find more useful is help like “don’t use unique_ptr in this way”, or suggestions anywhere I put “TODO: this is terrible”.

in src/wallet/walletdb.cpp:432 in df79475328 outdated

423@@ -418,6 +424,18 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
424                 strErr = "Error reading wallet database: Unknown non-tolerable wallet flags found";
425                 return false;
426             }
427+        } else if (strType == "descriptor") {
428+           unsigned int nID;
429+           ssKey >> nID;
430+           WalletDescriptor desc;
431+           ssValue >> desc;
432+           // TODO: figure out how to combine ssValue >> with MakeUnique<WalletDescriptor>

Sjors commented at 12:07 pm on March 1, 2019:

@practicalswift e.g. this is the kind of thing I find feedback more useful on in the WIP stage

ryanofsky commented at 3:43 pm on March 1, 2019: member

I took a look at this, and it seems well structured. I’m curious what the first use-case will be. Or I guess what’s the minimum thing that needs that needs to be implemented here for this to be useful for hardware wallets and #14912? Current state as of df794753284cc1bd991a5cdf0bd747fffb1db3b9 seems to be that a descriptor wallet is a blank wallet that you’re allowed to import public key descriptors to, without them being used for anything yet.

It does seem to me that the flag stuff might be overkill. I don’t think we need a new flag or changes to createwallet now that we already have blank wallets. It looks like WALLET_FLAG_DESCRIPTOR_WALLET just creates a lot of new branches all over the code that don’t need to exist or could be replaced with !m_descriptors.empty().

Sjors commented at 4:11 pm on March 1, 2019: member

The reason for the flag is to prevent “legacy” keys from getting mixed with descriptors. Maybe later on I discover that such mixing is fine, then I can drop the flag.

Also note that the blank flag goes away when you add a key. Which reminds me that it should also go away when you add a descriptor; that’s missing in this version.

DrahtBot added the label Needs rebase on Mar 4, 2019

Sjors force-pushed on Mar 5, 2019

DrahtBot removed the label Needs rebase on Mar 5, 2019

[build] add IO support for Boost::Optional b5d67b2d9d

Add AddressType (base58, bech32) 74a893f2b6

Descriptor: add GetAddressType() 4371f3aca1

[wallet] add mandatory flag for descriptor based wallet 2489c72a4e

[rpc] createwallet: add descriptor argument c61a451563

[wallet] descriptor wallet must be created blank 0eacc98ec7

[test] createwallet: descriptor wallet must have private keys disabled 3ef7a78c18

[wallet] cannot upgrade to descriptor wallet 6ea2d2b05a

[wallet] descriptor based wallets do not have a keypool a750012291

Sjors force-pushed on Mar 15, 2019

Sjors renamed this:
~~[WIP] descriptor based wallet~~
[WIP] descriptor based wallet serialization and import
on Mar 15, 2019

Sjors force-pushed on Mar 16, 2019

Sjors commented at 9:52 pm on March 16, 2019: member

It now distinguishes “address source” descriptors (similar to keypool) from “archive” descriptors. There can only be one base58 and one bech32 receive / change address source descriptor.

I implemented getnewaddress and dumpwallet includes address labels.

I could use feedback on the details of what we’re serializing, as well as on how I implemented the serialization / wallet loading code. That’s mainly in commits [wallet] add descriptor (de)serialization and [wallet] descriptor address serialization + derivation.

I got a little dizzy from tossing around unique pointers and I couldn’t figure out how to deserialize a descriptor in one go. The next step is probably to add serializer code to the Descriptor class, but I’m not sure how to do that.

in src/wallet/walletdb.h:211 in 839c4e7cef outdated

215+
216+    explicit WalletDescriptor(std::string descriptor_string_, int64_t nCreateTime_, uint64_t id_, int purpose_) :
217+        m_descriptor_string(descriptor_string_), nCreateTime(nCreateTime_), m_id(id_), m_purpose(purpose_)
218+    {
219+        nVersion = WalletDescriptor::CURRENT_VERSION;
220+        // ReadKeyValue first initalizes an empty WalletDescriptor

practicalswift commented at 10:23 am on March 20, 2019:

Should be “initializes” :-)

Sjors force-pushed on Mar 20, 2019

[wallet] add descriptor (de)serialization 7e01e28f7c

[rpc] add importdescriptor

importdescriptor replaces importmulti for descriptor based wallets.

d3fb62c828

[rpc] dumpwallet: dump descriptors c3d226b0cc

[rpc] importwallet: no descriptor import fff1afd538

Sjors force-pushed on Mar 21, 2019

[wallet] descriptor address serialization + derivation b8ff7b1412

[rpc] getnewaddress: support descriptor wallet 8debda3276

[rpc] dumpwallet: add descriptor addresses ee86df4f3c

Sjors force-pushed on Mar 21, 2019

[wallet] add FindDescriptorAddress 02ae76e7cc

[rpc] getaddressinfo: support descriptor wallets 1f823ba8a4

[test] add descriptor wallet multisig tests 2e5e977dc4

[rpc] addmultisigaddress: no descriptor support 542b1a7842

[rpc] dumpprivkey and importprivkey: no descriptor support a1ae0651d2

[rpc] importaddress and importpubkey: no descriptor support 06f23b3ece

[rpc] keypoolrefill: no descriptor support 88968b631d

[wallet] label helpers 3fc3fe244f

[rpc] getaddressesbylabel descriptor wallet support 1238f69e4b

[rpc] listaddressgroupings: descriptor wallet support ad60f1e40a

Sjors force-pushed on Mar 21, 2019

DrahtBot added the label Needs rebase on Apr 17, 2019

DrahtBot commented at 7:24 pm on April 17, 2019: member

Sjors commented at 9:30 am on June 6, 2019: member

Closing in favor of @achow101’s #15764 which has better momentum.

Sjors closed this on Jun 6, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

[WIP] descriptor based wallet serialization and import #15487

Conflicts