Add HD/Bip32 support #6265

jonasschnelli commented at 11:06 AM on June 10, 2015: contributor

This will add basic bip32 support for the current wallet. I testes the PR in serval environments, but, still needs more testing. Feedback of any type is highly welcome.

Concept

At the moment, all HD operations are independent/separated from the existing normal single key operations (allows better testing and a smother transition to HD wallets).
The wallet is still capable of using and producing single keys.
The user can define his desired chainpath like m/44'/0'/0'/c or m/0'/c
- "m" stands for master, "c" for switch between internal (1) and external (0) chain
HD private keys are not stored in the database. If they are required, they get derived via the stored master seed and the available metadata (child index, chain path).
When creating a new hd chain of keys (hdaddchain add <chainpath> <masterseed>), the rpc command will add the generated master seed as hex to the response (user can store it and use it as backup).
Bip39 is not supported (and I don't plan to support it). Instead of bip39 i'd like to use @maaku base-32 error correction encoding (https://gist.github.com/maaku/8996338). Users could write down the used master seed (or print if we would add a such option to the GUI). (not sure about that but i think users should create a print of the seeds 32byte hex representation. Brainwallets tend to have weak security and are social-conceptual a bad idea).
There is one new wallet state variable (activeHDChain) which is somehow unavoidable (change addresses, etc.).

Persistance

HD pub keys are stored within a new object CHDPubKey. Additional to the raw pub key there is also information about the chain, depth, etc.
HD master seeds are stored as a 32byte raw data blob (encrypted with the given masterkey if wallet is/gets encrypted)

RPC

This adds 5 new rpc commands

hdaddchain allows to add a new hd chain of keys
hdgetaddress get next available external key
hdsetchain set the active chain of keys (enables basic chain rotation)
hdsendtoaddress copy of sendtoaddress but uses a hd pub key for the change address
hdgetinfo list the available chains of keys

GUI

The UI still uses normal key operation. Switch to HD would be trivial, but, i'd like to extend the "first start wizard" to allow selecting a chain and encrypt before writing unencrypted data to the database.

What's next

The rpc test is a little bit mininalistic and I have plans to extend the test
Support for pub key only wallets (basically this is trivial, it only needs to allow master pub key or extended int./ext. chain pub keys as input for hdaddchain RPC. Signing would fail in a such case.
Support for HDM (allow one or two additional master pub keys per chain of keys to allow creating of multisigaddresses with standard hdgetaddress call [as well as change output keys]).

in src/wallet/rpcwallet.cpp:None in c85a0b6245 outdated

2592 | +    result.push_back(Pair("address", CBitcoinAddress(keyID).ToString()));
2593 | +    result.push_back(Pair("chainpath", keyChainPath));
2594 | +    return result;
2595 | +}
2596 | +
2597 | +UniValue hdsendtoaddress(const UniValue& params, bool fHelp)

luke-jr commented at 11:23 AM on June 10, 2015:

This should be based on sendmany (not sendtoaddress) and take the chain to use for change as a parameter. The existing RPCs and GUI should use the hdsetchain-configured default - no need for random keys.

jonasschnelli commented at 7:14 PM on July 23, 2015:

I agree with @luke-jr. The hd'ness should be used by default in future. But maybe to give a save transition, a first version would keep the hd functions separated. Users could decide where they want to use hd and where not. Maybe not everyone likes to use hd change keys for current non-hd inputs.

But right, this PR would slightly mix hd and non-hdness anyway. Unspents and balance would be shared.

in src/wallet/rpcwallet.cpp:None in c85a0b6245 outdated

2438 | +
2439 | +    UniValue result(UniValue::VOBJ);
2440 | +
2441 | +    assert(pwalletMain != NULL);
2442 | +    EnsureWalletIsUnlocked();
2443 | +

luke-jr commented at 11:25 AM on June 10, 2015:

Needs a check that the wallet format has been upgraded to support HD keys. The new format can be the default, but shouldn't upgrade unless -upgradewallet is used.

jonasschnelli commented at 11:33 AM on June 10, 2015:

I think (didn't test) that the hd wallet format is backward compatible. No existing database key/values records got changed. Only new ones added.

luke-jr commented at 11:10 PM on December 1, 2015:

If private keys are not stored, then old software won't be able to use the wallet...

jonasschnelli renamed this:
~~add HD/Bip32 support~~
Add HD/Bip32 support
on Jun 10, 2015

laanwj added the label Wallet on Jun 12, 2015

fanquake commented at 10:55 PM on June 16, 2015: member

One warning while compiling

  CXX      qt/qt_libbitcoinqt_a-walletmodeltransaction.o
qt/walletmodeltransaction.cpp:20:5: warning: delete called on 'CReserveKey' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor]
    delete keyChange;
    ^
1 warning generated.

hdgetaddress doesn't seem to work?

tinyformat: Not enough conversion specifiers in format string (code -1)

in src/wallet/hdkeystore.h:None in 3555883a50 outdated

  16 | +class CHDPubKey
  17 | +{
  18 | +public:
  19 | +    static const int CURRENT_VERSION=1;
  20 | +    int nVersion;
  21 | +    CPubKey pubkey; //the acctual pubkey

fanquake commented at 10:55 PM on June 16, 2015:

s/acctual/actual

in src/wallet/hdkeystore.h:None in 3555883a50 outdated

 145 | +    bool AddChain(const CHDChain& chain);
 146 | +
 147 | +    //!writes a chain defined by given chainId to chainOut, returns false if not found
 148 | +    bool GetChain(const HDChainID chainId, CHDChain& chainOut) const;
 149 | +
 150 | +    //!Derives a hdpubkey object in a given chain defined by chainId from the existing external oder internal chain root pub key

fanquake commented at 10:56 PM on June 16, 2015:

s/oder

jonasschnelli commented at 6:26 AM on June 17, 2015: contributor

@fanquake: thanks for the tests. Will have a look at it. Before calling hdgetaddress did you call hdaddchain?

rebroad commented at 1:56 PM on June 18, 2015: contributor

can this be used to import master seeds? If not, would this require much additional work?

in src/wallet/rpcwallet.cpp:None in 3555883a50 outdated

2429 | +                            "{\n"
2430 | +                            "  \"seed_hex\" : \"<hexstr>\",  (string) seed used during master key generation (only if no masterseed hex was provided\n"
2431 | +                            "}\n"
2432 | +
2433 | +                            "\nExamples\n"
2434 | +                            + HelpExampleCli("hdaddchain", "set")

unknown commented at 6:48 PM on June 18, 2015:

Should this not read HelpExampleCli("hdaddchain", "") ? Since by providing "set" as an argument, you must also provide a seed ... I believe the intention was to provide a example run without providing a seed, which should just run as hdaddchain without arguments

jonasschnelli commented at 11:37 AM on June 19, 2015:

Right. Will fix this.

jonasschnelli commented at 11:41 AM on June 19, 2015: contributor

@rebroad: right. This PR would enabled HD features in the main bitcoin-core wallet. It won't affect the current ways of generating keys and sending coins. It's totally independent form the rest of the wallet features. So it is relatively save to use/test this.

You can import hd seeds, but only in raw hex. There is no support for Bip39 word lists. I'm pretty sure you'll find a tool to convert bip39 wordlists into 32byte master seed represented in hex.

afk11 commented at 2:32 AM on June 25, 2015: contributor

I haven't gotten to testing this yet, but why did you not decide to accept a base58 encoded key?

The first reason against accepting raw hex is there's no way to validate the key without visually comparing. And extended keys can often look similar at the start. xpub/xprv's include a checksum, so the user can be sure they entered the right thing.

The other reason is extensibility, since adding watch-only chains implies there won't be any master hex around to use.

jonasschnelli commented at 6:41 AM on June 25, 2015: contributor

@afk11: The lack of base58check encoded master keys is only because it's not implemented yet (didn't find time to do it). But it's very trivial to add. I decided to export the master seed hex to potentially allow to create a bip39 mnemonic (there could be a tool hex-seed->bip39mnemonic). IIRC, you can't create a bip39 mnemonic from the master private key (not sure). Somehow i feel to store a hd chains of keys in the very root, the seed and not in the first derivation, the master private key. Indeed, you can't visually verify a hex master seed. At the moment i'm not sure what's best practice here. I'd like to avoid Bip39 for security reasons and so im stuck now with hex encoding for the 256bit entropy.

But right. Especially for watch-only and HDM wallets, base58check encoded public key importing and usage as external chain root is extremely helpful. I also wrote that in the PR description (see "whats next").

jonasschnelli force-pushed on Jul 23, 2015

jonasschnelli commented at 10:44 AM on July 23, 2015: contributor

Rebased and added the base58check encoded master pub/priv key to the hdaddchain RPC output.

jonasschnelli force-pushed on Jul 23, 2015

jonasschnelli commented at 7:58 PM on July 23, 2015: contributor

Added some commits on top to address nits and add a feature for creating a hd chain of keys with a existing base58check encoded extended master private key (xpriv...). Supporting watch only wallets with a given base58check encoded pub key of the external key chain will follow.

jonasschnelli commented at 8:04 PM on July 23, 2015: contributor

Most of the bip32 work is also included in CoreWallet (https://github.com/jonasschnelli/bitcoin/tree/2015/05/corewallet), which aims to be a replacement/twin for the current wallet with the option of completely split it of, of the bitcoin-core repository.

The PR is an option for quicker adding bip32 supporting to bitcoin-core (for the ones who can compile bitcoin-core by them self) or for the case where CoreWallet could fail to reach a stable level.

jgarzik commented at 8:14 PM on July 23, 2015: contributor

concept ACK

jonasschnelli force-pushed on Jul 23, 2015

afk11 commented at 5:36 PM on July 27, 2015: contributor

@jonasschnelli oops, missed the notification on this.. You're right you can't go from hex to bip39 mnemonic, as mnemonics have pbkdf2 applied to them. +1 on base58 also.

I noticed you have a hard limit of 32 bytes of hex for the provided seed. This isn't correct as BIP32 doesn't specify a size, but rather states the binary seed may be short, or as long as 512 bits (128-256 recommended), since it's hashed with HMAC.

Small PR against your branch for this: https://github.com/jonasschnelli/bitcoin/pull/8

Also, chalk it up to not reading the docs carefully, but I didn't expect hdaddchain to create a random key when I started testing. Maybe explicitly mention this will happen if no hex seed / master private key is provided?

jonasschnelli commented at 5:46 PM on July 27, 2015: contributor

@afk11: Meh. Right. I see your point. So you are saying once you have created a bip32 chain with a entropy over GetRandBytes() you can't build a bip39 wordlist (sorry, not familiar with bip39)?

This would definitively mean using base58c for the seed output, or even better, just the master private key in base58c. Or do you see any reason to keep the seed?

Maybe it should also support importing a bip39 wordlist to create a bip32 chain.

jonasschnelli force-pushed on Jul 27, 2015

afk11 commented at 6:34 PM on July 27, 2015: contributor

Yes, that is correct - the entropy used to produce the bip39 mnemonic is different to the seed that is produced.

I would suggest only displaying the base58 master key. It would keep the API consistent, since in certain situations the initial seed used to yield the master key won't be available, and in all other dumphd... cases it'll probably just be the xprv/xpub.

There isn't much reason to keep the hex seed - the same can be said for BIP39 mnemonics (and their passphrases) I guess. You could have separate RPC methods to create a new master via both methods.

I'm not sure if anything is lost in doing this. If someone dumps their master xprv, they can import it using hdaddchain just the same as if they had the mnemonic / hex.

rnicoll commented at 11:01 AM on August 6, 2015: contributor

Could you point me at some background on the concerns about BIP 39 and security please?

Seed interoperability with other wallets would seem like one of the major advantages to HD wallet support, although being about to import is IMHO the most important part of that use case, so reference client can be used to recover where another wallet fails.

jonasschnelli commented at 12:27 PM on August 6, 2015: contributor

@rnicoll: Mostly because Bip39 can harm your security:

The 2048 kdf rounds might be okay for embedded USB hardware wallet. But IMO nothing that i'd like to have on my full-node wallet.

But i'm pretty sure you can export/convert a bip39 memonic into a master xpriv key (maybe on bip32.org)? Also feel free to patch the RPC hdaddchain with import support for bip39.

rnicoll commented at 1:27 PM on August 6, 2015: contributor

@jonasschnelli Thanks for the links, interesting reading. Will definitely take a look at patching hdaddchain for imports

jonasschnelli commented at 1:35 PM on August 6, 2015: contributor

@rnicoll: this PR is out of sync with the corewallet branch (which is more stable and has more features). If you add a patch, i think it makes much more sense there: https://github.com/jonasschnelli/bitcoin/

I will try to update this PR after stabilizing the corewallet hd features. The reason for the parallelism: #6265 (comment)

rnicoll commented at 1:59 PM on August 6, 2015: contributor

@jonasschnelli Noted, thanks for letting me know. Mind if I pull this into an altcoin reference client? Would mean it goes out to general users Oct/Nov this year, and might be a good way of getting further testing? If it's not helpful will pick the change up after merge into Bitcoin Core instead.

jonasschnelli commented at 2:11 PM on August 6, 2015: contributor

@rnicoll: Feel free to play, share, copy this PR. But mind, it's pretty untested. I opened this PR for people who likes to use HD features now together with the current bitcoin-core wallet. Long term i think the corewallet branch (https://github.com/jonasschnelli/bitcoin/) makes more sense. People can start testing the new features in parallel to the existing wallet... if it reaches a stable level, it might get merged back into the bitcoin-core master.

jonasschnelli commented at 12:29 PM on August 7, 2015: contributor

Today there was a discussion about the Bip32 support on IRC (http://bitcoinstats.com/irc/bitcoin-dev/logs/2015/08/07#l1438905927.0). This PR uses public key derivation which has significant security weakness, but allows xpubkey only wallets. But by default the wallet should use privatekey derivation together with keypools.

The multiple bip32 chains per wallet can also be dangerous because one needs to be very careful about master key backups and funds might end up in multiple chains which would require multiple master keys to recover.

I try to simplify the PR by removing serval features (like multichain support) and try to add the minimal by bip32 derivation during keypool refill. More feature rich version aims for the corewallet branch.

rnicoll commented at 12:49 PM on August 7, 2015: contributor

It sounds like my use-case of importing BIP 39 seeds as a recovery might be better implemented as a sweep (i.e. it scans for any transactions to keys from the seed, and then puts the relevant non-master public/private keys pairs into the wallet, rather than keeping the master keys). Alternative would be to sweep the funds to wallet addresses, but that introduces privacy concerns (clearly identifies the funds as related). Thoughts?

jonasschnelli commented at 7:08 PM on August 7, 2015: contributor

Importing a master seed (over bip39 phrase or over master extended private key) should certainly remind the user (some warnings and confirmations) that it is possible that the seed is already compromised (because of the seed transfer or because of leaks in the other wallet software).

[...] i.e. it scans for any transactions to keys from the seed [...]

This is very hard to achieve in a stable way IMO. If you don't have a clear lookup window this process could be very time consuming. I also think people might not know what chainpath they used before.

Sweep to a fresh seeded chain would make most sense IMO, but as @rnicoll points out: it could be a privacy leak.

Maybe the user should decide between privacy import keys/seed or security with a complete sweep.

slush0 commented at 12:23 AM on September 25, 2015: none

Slightly offtopic, but: Uh, I don't understand that neverending discussions about BIP39 "security". The fact that PBKDF iterations may not be "slow enough" does not mean the concept is insecure. Such hashing is just additional security protection against bruteforcing, but BIP39 produced seeds are completely safe, they use at least 256 bits of entropy (24 words) and they're NOT "brain wallets" despite the fact that encoding is human readable!!

BIP44+BIP39 is currently the most used solution across various wallets and I don't see real reasons to use anything else in Core. Attitude "let's use some other encoding and recommend users to use external tools like bip32.org to convert seed" has huge practical impacts to real security.

dcousens commented at 6:08 AM on September 25, 2015: contributor

Recommend users to use external tools like bip32.org to convert seed" has huge practical impacts to real security.

IMHO, all other points aside, this would be my biggest concern.

sipa commented at 11:40 PM on September 25, 2015: member

@slush0 I know that BIP39 has been around for a while and is supported by multiple applications. However, what exactly do you think should be implemented? As I read it, BIP39 can be interpreted in two ways:

Either on import you decode the mnemonic and verify the checksum. This means BIP39 requires a normative dictionary (or set thereof), and results in a simple scheme that discourages brainwallets (but does not have the plausible deniability it aims to have).
Or you interpret the wordlist to be purely generator side, in which case the checksum is a pointless complication, and it becomes possible to use it as a weak brainwallet system (with plausible deniability which disappears as soon as an attacker has seen your wallet).

I think Bitcoin Core has a responsability to adopt standards that are in general secure by default, so I don't think we can use the second interpretation. Then the question arises with which other application's dictionaries we should be compatible? New ones seem to get added over time, and that changes the standard. The BIP even encourages applications to choose their own.

In addition to that, 2048 iterations is very low these days, and will continue to deteriorate over time, as BIP39 has no variable strengthening or version number to improve in the future.

dcousens commented at 6:27 AM on September 26, 2015: contributor

but does not have the plausible deniability it aims to have

My understanding was that that was only a claim if you used a passphrase? In any case, the primary benefit of BIP39 is human-readable cross-compatible BIP32 seeds, I don't think its potential use as a brain wallet is why it has seen widespread adoption.

In addition to that, 2048 iterations is very low these days, and will continue to deteriorate over time, as BIP39 has no variable strengthening or version number to improve in the future.

That step is basically useless. It should be ignored for the sake of argument and is simply just an annoying implementation detail.

btcdrak commented at 4:04 PM on October 9, 2015: contributor

needs rebase

afk11 commented at 4:23 PM on October 22, 2015: contributor

I don't think BIP39 should be packaged into this feature.

I suspect for a reasonable integration, it's would not be ideal having different passwords protecting different parts of the wallet. Similarly, conditionally storing mnemonics and perhaps their passwords adds a lot of complexity to work around.

A stateless mnemonic to xprv RPC command could be offered to help people using BIP39, but that opens the door to supporting every single KDF function out there.. Why not just accept extended pub/priv keys, and leave human readable entropy for humans?

rubensayshi commented at 10:08 AM on October 23, 2015: contributor

something to keep in mind with BIP39 is that because seed = hash(mnemonic, pass) it doesn't allow for changing passwords :/

dcousens commented at 10:11 AM on October 23, 2015: contributor

@rubensayshi, at least, not without transferring all the BTC.

dcousens commented at 2:04 AM on December 2, 2015: contributor

Needs rebase

add bip32 pubkey serialization

CExtPubKey should be serializable like CPubKey

b2ea642d8c

add constant for bip32 extkey size 812a395174

CKeyMetadata: support for bip32 9ddff30a26

walletdb: basic bip32 support b6e30bb97c

basic bip32 support f814de07cf

add bip32/hd keystore with support for crypted masterseeds 856fc928a3

allow multiple hd chains (keyrotation basics) 6aeda34e35

hdwallet: refactor hdkeystore.cpp, support encryption

- master seed can now be stored encrypted
- refactor hdkeystore.cpp/wallet.cpp
- add CHDPubKey which represents a deriven child key with some metdata for adding to a persistant store

5d0f0814a2

hdwallet: add 'hdsetchain' and 'hdgetinfo' b5bf17c9b4

hdwallet: remove std::string back() and replace it with *rbegin()

avoid c++11

ca19680907

RPC hdaddchain: add ext master pub/priv key to JSON output c0c98ed9c8

add hdwallet rpc test 4ada7d6add

fix comment and help typos f7dc4bdbf3

hdaddchain: support for creating a chain of key with an existing master private key d0eebc011c

fix index error in hdaddchain chain path parameter, fix hex size of >256bit entropy 0a2b71d8e0

extend hdwallet.py PRC to cover flexible bip32 chain path and initial creation with base58c encoded ext priv key 3327d299d4

jonasschnelli force-pushed on Dec 23, 2015

Use CKey instead of pure `GetRandBytes` for master seed 476761d8d2

Add support for private key derivation, use m/c' as default keypath 4ed902c14d

jonasschnelli commented at 3:10 PM on December 23, 2015: contributor

Rebased and updated.

Not the default keypath is m/c' = (m/0'/0' for first external key and m/1'/0' for first internal/change key).
Support for private child key derivation (default).

This PR is relatively complex and are meant for user who are seeking full control over their hd structure. I have plans to also open a smaller, simpler HD patch.

jonasschnelli commented at 9:26 AM on May 10, 2016: contributor

Closing in favor of #8035. Some parts of this PR could be used to extend HD functionality if and once #8035 has been merged.

jonasschnelli closed this on May 10, 2016

MarcoFalke locked this on Sep 8, 2021