[Wallet] Add simplest BIP32/deterministic key generation implementation #8035

jonasschnelli · 2016-05-10T09:21:39Z

Probably most simplest HD implementation.

New wallets will use hd/bip32 (enabling hd only possible during wallet creation/first start)
HD can be disabled with --usehd=0
Only hardened key derivation
Fixed keypath (m/0'/0'/k')
Only one additional database object (CHDChain) which contains the CKeyID for the master key together with the child key counter

I'll add some tests once there are some conceptual acks (and also to keep the diff at a very minimum).

slush0 · 2016-05-10T10:06:40Z

Nice minimalistic PR. One question though. Why to use m/0'/0'/i instead of something more standard? BIP32 proposes m/0'/0/i for external chains (I suppose Core in current implementation does not need internal/change chain). Or even better - use m/0/i, which is forward compatible with BIP44 (maybe Core will make it somedays). m/0/i is subset of BIP44 - an account.

paveljanik · 2016-05-10T10:15:42Z

src/wallet/wallet.cpp

No need for strprintf. See previous line.

Hmm... I have looked at walletbroadcast, there strprintf is also used. Isn't more flexible for the translation then?

I think you want to strprintf(_(string)) the whole string and not two separate stings, as other languages might have different grammar. but consider it a nit.

s/Has only affect/Only has effect/

paveljanik · 2016-05-10T10:25:15Z

Concept ACK

Nice, small, elegant 👍

paveljanik · 2016-05-10T10:52:06Z

What should RPC dumpwallet dump after this?

paveljanik · 2016-05-10T10:56:40Z

Some import path needed then.

laanwj · 2016-05-10T11:02:02Z

Concept ACK

jonasschnelli · 2016-05-10T13:18:03Z

@slush0: One reason for the current keypath is the use hardened-only key derivation.
I personally think people overestimate the switch-wallet feature. Importing a bip32 master key together with a given keypath can be tricky (lookup window).

But I agree.
If and once this PR gets merged into master, next thing I would do is writing a PR that would make the keypath flexible.

We could discuss the default keypath and the keypath flexibility in a such follow up PR.

This PR only intent to solve the "backup problem" by using deterministic key derivation. So we could even think of using m/k'.

jonasschnelli · 2016-05-10T13:56:03Z

Fixed @paveljanik nits.

slush0 · 2016-05-10T14:19:33Z

I personally think people overestimate the switch-wallet feature.

@jonasschnelli It's actually quite useful feature, and it may be even for Core. I can imagine somebody who'll have Qt wallet awfully out of sync and will need to get his funds ASAP. Then loading the backup to some other software like Multibit HD will do the job. BIP44 is actually being used by most of current wallets so heading towards the same structure makes sense.

This PR only intent to solve the "backup problem"

Fair point and I appreciate somebody is finally addressing it.

paveljanik · 2016-05-10T17:23:14Z

src/wallet/wallet.cpp

jonasschnelli · 2016-05-10T18:12:40Z

Force push fixed @paveljanik nits. Formatted some of the code with clang-format.

sipa · 2016-05-10T18:13:12Z

src/wallet/wallet.cpp

I think you should check whether the derived key already exists in the keystore, and if so, try the next chaincounter. Otheriwse you'll give out the same keys twice after a backup restore.

EDIT: nevermind, that's not going to work. we'll also need to make keypool entries be regenerated when one is encountered in the blockchain or mempool. I think we can defer that...

I added a loop to derive the next key as long as its not known by the wallet. I think this is required otherwise we might run into an exception if someone restores a backup or has imported a derived key of a higher child index.

achow101 · 2016-05-11T12:17:48Z

Concept ACK

So as I understand this, it won't affect the keys already in the wallet, right? It just generates all new keys using the HD master key?

Also there should be an RPC command to dump the master private key.

jonasschnelli · 2016-05-11T12:20:54Z

@achow101:
for now (this PR), HD will only be enabled when you create a new wallet (first start).

Also there should be an RPC command to dump the master private key.

I agree. Have a look at #6265 and #7273.
This is my third try to get HD into Cores wallet. I think the way to go is this extremely simple basic step.
We can add RPC/API changes later.

instagibbs · 2016-05-12T14:50:59Z

src/wallet/wallet.h

external only for now? (and in the other 2 places in the PR)

laanwj · 2016-06-14T06:45:36Z

src/wallet/wallet.cpp

Can be simplified a bit:

else if (mapArgs.count("-usehd")) { bool useHd = GetBoolArg("-usehd", DEFAULT_USEHD); if (!walletInstance->hdChain.masterKeyID.IsNull() && !useHD) return InitError(strprintf(_("Error loading %s: You can't disable HD on a already existing HD wallet"), walletFile)); if (walletInstance->hdChain.masterKeyID.IsNull() && useHD) return InitError(strprintf(_("Error loading %s: You can't enable HD on a already existing non-HD wallet"), walletFile)); }

(also please introduce a DEFAULT_USEHD constant)

Thanks. Much simpler.
Force push updated last commit.

laanwj · 2016-06-14T09:44:18Z

utACK afcd77e

Even though this wallet change is reasonably risky, I think this has received enough review for correctness, and it should be merged now to make sure it gets more actual testing.

… implementation afcd77e Detect -usehd mismatches when wallet.dat already exists (Jonas Schnelli) 17c0131 [Docs] Add release notes and bip update for Bip32/HD wallets (Jonas Schnelli) c022e5b [Wallet] use constant for bip32 hardened key limit (Jonas Schnelli) f190251 [Wallet] Add simplest BIP32/deterministic key generation implementation (Jonas Schnelli)

gmaxwell · 2016-06-15T09:40:19Z

Post merge utACK. Will also test.

schildbach · 2016-06-15T18:22:39Z

Just found out about this PR. Would like to clarify the keypath: It's purpose number is 0' not 0? ((m/0'/0'/k') not (m/0/0'/k'))

I'm asking because otherwise it could be incompatible to how bitcoinj derives addresses.

jonasschnelli · 2016-06-17T14:12:32Z

@schildbach: We have decided to go with a "hardened only derivation" version for the first implementation. Using m/0/0'/k' would introduce the risk of revealing extended pubkey m/0 together with a non extended child privatekey resulting in revealing the whole chain of keys.

On top of that, I personally think importing a xpriv into another wallet in order to "reconstruct everything" is not a very good concept.

schildbach · 2016-06-17T19:42:18Z

@jonasschnelli Yes that's fine. I just wanted to make sure there is no "partial overlap" with the bitcoinj impl.

rebroad · 2016-06-25T15:54:49Z

So, how does one import a BIP32 master key into an existing wallet?

diegoviola · 2016-06-25T16:10:31Z

I'm switching to Core (from Electrum) after this lands in 0.13, thanks for your efforts.

laanwj · 2016-06-27T12:27:01Z

So, how does one import a BIP32 master key into an existing wallet?

That functionality currently doesn't exist. Please read the OP: this is the smallest possible change to introduce BIP32 into Bitcoin Core. This made it realistic to get it reviewed and merged in a small timespan. More functionality can be added later.

maflcko · 2016-07-08T17:20:34Z

src/wallet/wallet.cpp

+    // key metadata is not required
+    CPubKey pubkey = key.GetPubKey();
+    if (!AddKeyPubKey(key, pubkey))
+        throw std::runtime_error("CWallet::GenerateNewKey(): AddKey failed");


Nit: Wrong function name

TheBlueMatt · 2016-07-19T01:48:44Z

Post-merge review comments, sorry :/ (continued from IRC):
There is also a comment on #8324 and other related PRs here.

The use of an ECDSA private key (stored as such) as the master seed is somewhat strange...While not strictly an issue, I'm concerned about the ability of users to dump this as a regular key, potentially causing some interesting behavior.
The way I read the current code, every time we go to GenerateNewKey() we will iterate over every key we have already generated before generating a new one (as we never call SetHDChain to store the updated counter on disk)....anyone with a test hd wallet lying around want to see what their disk-counter is?
I think IsKeyType should be changed to include hdchain, not add an explicit hdchain check in ::Recover, since we probably definitely want it in ::LoadWallet as well.
The duplication of the old CTransaction READWRITE(this->nVersion); nVersion = this->nVersion; logic seems strange to me. I always found that a kinda funny way to do things and was glad to see it go in SegWit...Its "hidden functionality" and I'd prefer if we handled that very explicitly instead of via this strange hack.

I think all but the first could easily be fixed even now, if we decide they are, indeed, issues.

jonasschnelli · 2016-07-19T05:32:11Z

Thanks for the post-merge review Matt.

The use of an ECDSA private key (stored as such) as the master seed is somewhat strange...While not strictly an issue, I'm concerned about the ability of users to dump this as a regular key, potentially causing some interesting behavior.

Yes. Agree. This was done after two of my former PR did not made it into the master (#6265 #7273) and I was looking for a simpler solutions.
There is a PR to identify the seed when dumping (#8206).

The way I read the current code, every time we go to GenerateNewKey() we will iterate over every key we have already generated before generating a new one (as we never call SetHDChain to store the updated counter on disk)....anyone with a test hd wallet lying around want to see what their disk-counter is?

This is to ensure we don't re-generate an already existing key (there are some cases where this would be possible otherwise).
The iteration should be painless. It's a std::map count() of an in memory map of all the key.

TheBlueMatt · 2016-07-19T23:51:11Z

re: GenerateNewKey(): Not sure what your response was about (seems to be communication failure there), but it was pointed out to me that I missed the save line and was incorrect.

zihad944 · 2018-08-11T21:10:58Z

I foget my sceond wallet passwor. Thats why i cant get backup. I need help

jeandudey · 2018-08-11T21:16:09Z

@zihad944 what problem do you have specifically, what do you mean by "my second wallet password"?

sorawee1970

src/wallet/walletdb.h

jonasschnelli added the Wallet label May 10, 2016

jonasschnelli added this to the 0.13.0 milestone May 10, 2016

This was referenced May 10, 2016

Increase DEFAULT_KEYPOOL_SIZE to 10000. #8025

Closed

Add HD/Bip32 support #6265

Closed

[Wallet] Simple HD/BIP32 support #7273

Closed

paveljanik reviewed May 10, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/05/simplest_hd branch from ce6332e to c3b8aa5 Compare May 10, 2016 13:55

paveljanik reviewed May 10, 2016
View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

paveljanik May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the the

jonasschnelli force-pushed the 2016/05/simplest_hd branch from c3b8aa5 to 30815d2 Compare May 10, 2016 18:11

sipa reviewed May 10, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/05/simplest_hd branch 2 times, most recently from 0141792 to cc1df8f Compare May 11, 2016 12:32

instagibbs reviewed May 12, 2016
View reviewed changes

src/wallet/wallet.h Outdated

Copy link

Member

instagibbs May 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

external only for now? (and in the other 2 places in the PR)

laanwj reviewed Jun 14, 2016
View reviewed changes

Detect -usehd mismatches when wallet.dat already exists

afcd77e

jonasschnelli force-pushed the 2016/05/simplest_hd branch from 71ff55f to afcd77e Compare June 14, 2016 06:56

laanwj merged commit afcd77e into bitcoin:master Jun 14, 2016

jonasschnelli mentioned this pull request Jun 15, 2016

Implement deterministic wallets #3250

Closed

maflcko reviewed Jul 8, 2016
View reviewed changes

laanwj mentioned this pull request Jul 20, 2016

[Wallet][Trivial] Rename usehd option to createhdwallet #8377

Closed

sipa mentioned this pull request Sep 9, 2016

Implement gap detection for deterministic wallets #8689

Closed

ghost mentioned this pull request Sep 1, 2017

Feature Request: Implement deterministic wallets gridcoin-community/Gridcoin-Research#570

Closed

str4d mentioned this pull request Oct 22, 2017

Support Hierarchical Deterministic t-addresses (BIP 32) zcash/zcash#2673

Closed

peerchemist mentioned this pull request Dec 17, 2017

Port simple BIP32 implementation from Bitcoin-core peercoin/peercoin#290

Closed

sorawee1970 reviewed May 16, 2019

View reviewed changes

bitcoin deleted a comment from zihad944 May 16, 2019

TheQuantumPhysicist mentioned this pull request Jan 20, 2020

Implement BIP44 deterministic addresses (same as Orion) NeblioTeam/neblio#115

Open

andronoob mentioned this pull request Dec 17, 2020

Dealing with mnemonic formats other than BIP39, their ambiguity, and nonstandard derivation paths trezor/trezor-firmware#1392

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[Wallet] Add simplest BIP32/deterministic key generation implementation #8035

[Wallet] Add simplest BIP32/deterministic key generation implementation #8035

Uh oh!

Conversation

jonasschnelli commented May 10, 2016

Uh oh!

slush0 commented May 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented May 10, 2016

Uh oh!

paveljanik commented May 10, 2016

Uh oh!

paveljanik commented May 10, 2016

Uh oh!

laanwj commented May 10, 2016

Uh oh!

jonasschnelli commented May 10, 2016

Uh oh!

jonasschnelli commented May 10, 2016

Uh oh!

slush0 commented May 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 10, 2016

Uh oh!

sipa May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 11, 2016

Uh oh!

jonasschnelli commented May 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 14, 2016

Uh oh!

gmaxwell commented Jun 15, 2016

Uh oh!

schildbach commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jun 17, 2016

Uh oh!

schildbach commented Jun 17, 2016

Uh oh!

rebroad commented Jun 25, 2016

Uh oh!

diegoviola commented Jun 25, 2016

Uh oh!

laanwj commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jul 19, 2016

Uh oh!

TheBlueMatt commented Jul 19, 2016

Uh oh!

zihad944 commented Aug 11, 2018

Uh oh!

sipa May 10, 2016 •

edited

Loading

laanwj Jun 14, 2016 •

edited

Loading

schildbach commented Jun 15, 2016 •

edited

Loading

TheBlueMatt commented Jul 19, 2016 •

edited

Loading