[WIP] descriptor based wallet serialization and import #15487

pull Sjors wants to merge 26 commits into bitcoin:master from Sjors:2019/02/descriptor-wallet changing 22 files +1001 −141
  1. 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:

    bitcoin-cli createwallet true true true
bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#...."
bitcoin-cli importdescriptor "wpkh([00000000/84h/1h/0h]tpub.../1/*)#..." '{"internal": true}, "timestamp": "now"}' 
bitcoin-cli getnewaddress "SegWit"
bitcoin-cli importdescriptor "sh(wpkh([00000000/49h/1h/0h]tpub.../1/*))#...."
bitcoin-cli getnewaddress "Pre SegWit" legacy
bitcoin-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
  2. DrahtBot commented at 9:36 PM on February 26, 2019: contributor

    <!--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:

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

  3. fanquake added the label Wallet on Feb 26, 2019
  4. Sjors force-pushed on Feb 27, 2019
  5. Sjors force-pushed on Feb 27, 2019
  6. Sjors force-pushed on Feb 28, 2019
  7. 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 :-)

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

  9. 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));?

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

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

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

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

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

  15. ryanofsky commented at 3:43 PM on March 1, 2019: contributor

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

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

  17. DrahtBot added the label Needs rebase on Mar 4, 2019
  18. Sjors force-pushed on Mar 5, 2019
  19. DrahtBot removed the label Needs rebase on Mar 5, 2019
  20. Sjors cross-referenced this on Mar 9, 2019 from issue Make OutputType consistent with Descriptor and return it by Sjors
  21. Sjors cross-referenced this on Mar 13, 2019 from issue [WIP] External signer support (e.g. hardware wallet) by Sjors
  22. Sjors cross-referenced this on Mar 13, 2019 from issue Descriptor: add GetAddressType() and IsSegWit() by Sjors
  23. [build] add IO support for Boost::Optional b5d67b2d9d
  24. Add AddressType (base58, bech32) 74a893f2b6
  25. Descriptor: add GetAddressType() 4371f3aca1
  26. [wallet] add mandatory flag for descriptor based wallet 2489c72a4e
  27. [rpc] createwallet: add descriptor argument c61a451563
  28. [wallet] descriptor wallet must be created blank 0eacc98ec7
  29. [test] createwallet: descriptor wallet must have private keys disabled 3ef7a78c18
  30. [wallet] cannot upgrade to descriptor wallet 6ea2d2b05a
  31. [wallet] descriptor based wallets do not have a keypool a750012291
  32. Sjors force-pushed on Mar 15, 2019
  33. Sjors renamed this:
    [WIP] descriptor based wallet
    [WIP] descriptor based wallet serialization and import
    on Mar 15, 2019
  34. Sjors force-pushed on Mar 16, 2019
  35. Sjors force-pushed on Mar 16, 2019
  36. Sjors force-pushed on Mar 16, 2019
  37. 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.

  38. 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" :-)

  39. Sjors force-pushed on Mar 20, 2019
  40. Sjors force-pushed on Mar 20, 2019
  41. [wallet] add descriptor (de)serialization 7e01e28f7c
  42. [rpc] add importdescriptor
    importdescriptor replaces importmulti for descriptor based wallets.
    d3fb62c828
  43. [rpc] dumpwallet: dump descriptors c3d226b0cc
  44. [rpc] importwallet: no descriptor import fff1afd538
  45. Sjors force-pushed on Mar 21, 2019
  46. [wallet] descriptor address serialization + derivation b8ff7b1412
  47. [rpc] getnewaddress: support descriptor wallet 8debda3276
  48. [rpc] dumpwallet: add descriptor addresses ee86df4f3c
  49. Sjors force-pushed on Mar 21, 2019
  50. [wallet] add FindDescriptorAddress 02ae76e7cc
  51. [rpc] getaddressinfo: support descriptor wallets 1f823ba8a4
  52. [test] add descriptor wallet multisig tests 2e5e977dc4
  53. [rpc] addmultisigaddress: no descriptor support 542b1a7842
  54. [rpc] dumpprivkey and importprivkey: no descriptor support a1ae0651d2
  55. [rpc] importaddress and importpubkey: no descriptor support 06f23b3ece
  56. [rpc] keypoolrefill: no descriptor support 88968b631d
  57. [wallet] label helpers 3fc3fe244f
  58. [rpc] getaddressesbylabel descriptor wallet support 1238f69e4b
  59. [rpc] listaddressgroupings: descriptor wallet support ad60f1e40a
  60. Sjors force-pushed on Mar 21, 2019
  61. Sjors cross-referenced this on Apr 12, 2019 from issue Native descriptor wallets by achow101
  62. DrahtBot added the label Needs rebase on Apr 17, 2019
  63. DrahtBot commented at 7:24 PM on April 17, 2019: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  64. Sjors commented at 9:30 AM on June 6, 2019: member

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

  65. Sjors closed this on Jun 6, 2019
  66. laanwj removed the label Needs rebase on Oct 24, 2019
  67. bitcoin locked this on Dec 16, 2021
Contributors
SjorsDrahtBotpracticalswiftsiparyanofsky
Labels
Wallet
Linked (view graph)
#14912 [WIP] External signer support (e.g. hardware wallet)#15567 Make OutputType consistent with Descriptor and return it#15590 Descriptor: add GetAddressType() and IsSegWit()#15764 Native descriptor wallets
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC