[Wallet] use CHDPubKey, don't store child priv keys in db, derive on the fly #9298

jonasschnelli · 2016-12-07T11:00:43Z

Adds a new database record ("hdpubkey") reflected by class CHDPubKey.

Results in no longer storing derived child private keys in the database
Only the extended child public key will be stored
If the private key gets requested, it will be derived on the fly

Dump functions are unchanged, will result in deriving the keys on the fly.
Not backward compatible. Ideally combined with HD chain split #9294.

instagibbs · 2016-12-07T13:42:26Z

Can you explain a bit of motivation for this?

jonasschnelli · 2016-12-07T14:09:16Z

Can you explain a bit of motivation for this?

Sure. Sorry for leaving that out in the main description.

1.) Reduce amount of stored data in wallet.dat
2.) Don't expose each child private key in wallet.dat
3.) Lays groundwork for public key derivation in conjunction with a signing device.

paveljanik · 2016-12-08T13:27:43Z

src/wallet/wallet.h

The comment here should be different compared to the FEATURE_HD.

paveljanik · 2016-12-08T13:28:58Z

src/wallet/wallet.cpp

SPC added to the already translated string? Yes/No here?

maflcko · 2016-12-08T14:17:20Z

2.) Don't expose each child private key in wallet.dat

I think this means salvagewallet no longer works for hd wallets as it previously did. This could potentially lead to loss of coins for inexperienced users, because it is currently not easily possible to recover the HD wallet after salvagewallet, nor is is possible to recover the child keys, if they are not saved...

…m on the fly

jonasschnelli · 2016-12-08T16:27:21Z

I think this means salvagewallet no longer works for hd wallets as it
previously did.

Right. This is a good point.
However, I think we should do the following also for "normal" wallets.

We already recover the hdchain (child key counter, private key id thats used for the HD master key).
Don't abort if a privat-key is corrupted, instead...
Recreate all keys up to the child-key-counter (CHDChain object)

Before implementing this, I think some Concept ACKs/NACKs would be required.
@sipa, @gmaxwell: thoughts?

promag

@jonasschnelli Still interested in pushing this forward?

Needs rebase.

Should have a test.

promag · 2017-10-10T14:05:41Z

src/wallet/wallet.cpp

+    assert(secret.VerifyPubKey(pubkey));
+
+    // store metadata
+    mapKeyMetadata[pubkey.GetID()] = metadata;


This could stay in GenerateNewKey?

promag · 2017-10-10T14:06:11Z

src/wallet/wallet.cpp

+
+    // store metadata
+    mapKeyMetadata[pubkey.GetID()] = metadata;
+    if (!nTimeFirstKey || metadata.nCreateTime < nTimeFirstKey)


After rebase use UpdateTimeFirstKey(). Also could stay in GenerateNewKey?

promag · 2017-10-10T14:14:09Z

src/wallet/wallet.cpp

+
+bool CWallet::LoadHDPubKey(const CHDPubKey &hdPubKey)
+{
+    AssertLockHeld(cs_wallet); // mapKeyMetadata


Wrong comment.

promag · 2017-10-10T14:14:18Z

src/wallet/wallet.cpp

+
+bool CWallet::AddHDPubKey(const CExtPubKey &extPubKey)
+{
+    AssertLockHeld(cs_wallet); // mapKeyMetadata


Wrong comment.

sipa · 2018-03-06T17:25:17Z

@jonasschnelli I think this is generally the direction to go in, and it's also part of what my idea for refactoring accomplishes in a different way (https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2). Do you think this is worth pursuing on its own?

DrahtBot · 2018-11-08T23:20:49Z

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

jonasschnelli added the Wallet label Dec 7, 2016

jonasschnelli added this to the 0.14.0 milestone Dec 7, 2016

jonasschnelli force-pushed the 2016/12/hd_no_priv branch from e75d2cb to c1c86a3 Compare December 7, 2016 13:08

jonasschnelli force-pushed the 2016/12/hd_no_priv branch from c1c86a3 to 2b61ef8 Compare December 7, 2016 14:07

paveljanik reviewed Dec 8, 2016

View reviewed changes

src/wallet/wallet.h Outdated

Copy link

Contributor

paveljanik Dec 8, 2016

Choose a reason for hiding this comment

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

The comment here should be different compared to the FEATURE_HD.

paveljanik reviewed Dec 8, 2016

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

paveljanik Dec 8, 2016

Choose a reason for hiding this comment

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

SPC added to the already translated string? Yes/No here?

jonasschnelli force-pushed the 2016/12/hd_no_priv branch from 2b61ef8 to 0841ac8 Compare December 8, 2016 16:23

[Wallet] use CHDPubKey, don't store child priv keys in db, derive the…

3fd850a

…m on the fly

jonasschnelli force-pushed the 2016/12/hd_no_priv branch from 0841ac8 to 3fd850a Compare December 8, 2016 16:24

jonasschnelli removed this from the 0.14.0 milestone Jan 12, 2017

jonasschnelli mentioned this pull request Feb 9, 2017

Can create External HD wallet with -externalhd #9728

Closed

promag reviewed Oct 10, 2017

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

DrahtBot added the Up for grabs label Nov 8, 2018

DrahtBot closed this Nov 8, 2018

laanwj removed the Needs rebase label Oct 24, 2019

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

[Wallet] use CHDPubKey, don't store child priv keys in db, derive on the fly #9298

[Wallet] use CHDPubKey, don't store child priv keys in db, derive on the fly #9298

Uh oh!

Conversation

jonasschnelli commented Dec 7, 2016

Uh oh!

instagibbs commented Dec 7, 2016

Uh oh!

jonasschnelli commented Dec 7, 2016

Uh oh!

paveljanik Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

paveljanik Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 8, 2016 via email

Uh oh!

jonasschnelli commented Dec 8, 2016

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

promag Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

promag Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

promag Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

sipa commented Mar 6, 2018

Uh oh!

DrahtBot commented Nov 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants