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

[WIP] descriptor based wallet serialization and import #15487

Open
wants to merge 26 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Sjors
Copy link
Member

commented Feb 26, 2019

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
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 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:

  • #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 fanquake added the Wallet label Feb 26, 2019

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch 3 times, most recently from 6457e69 to df79475 Feb 27, 2019

@@ -800,6 +804,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

std::map<uint256, CWalletTx> mapWallet GUARDED_BY(cs_wallet);

std::map<unsigned int, std::unique_ptr<WalletDescriptor>> m_descriptors GUARDED_BY(cs_wallet);;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 28, 2019

Member

Nit: ;; at end of line :-)

return true;
}

bool CWallet::HaveDescriptor(const unsigned int nID) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 28, 2019

Member

Could be const?

if (!WalletBatch(*database).WriteDescriptor(nID, desc.get())) {
return false;
}
bool res = LoadDescriptor(nID, std::move(desc));

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 28, 2019

Member

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

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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

ssKey >> nID;
WalletDescriptor desc;
ssValue >> desc;
// TODO: figure out how to combine ssValue >> with MakeUnique<WalletDescriptor>

This comment has been minimized.

Copy link
@Sjors

Sjors Mar 1, 2019

Author Member

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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.

Sjors added some commits Mar 13, 2019

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch 3 times, most recently from e3d0a33 to 839c4e7 Mar 16, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

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.

Show resolved Hide resolved src/wallet/walletdb.h Outdated

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch 2 times, most recently from 2adbe65 to aa03f8e Mar 20, 2019

Sjors added some commits Feb 26, 2019

[rpc] add importdescriptor
importdescriptor replaces importmulti for descriptor based wallets.

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch from aa03f8e to a29ff2c Mar 21, 2019

Sjors added some commits Mar 16, 2019

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch from e192f02 to 4e4063d Mar 21, 2019

Sjors added some commits Mar 21, 2019

@Sjors Sjors force-pushed the Sjors:2019/02/descriptor-wallet branch from 4e4063d to ad60f1e Mar 21, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 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.