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

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/12/hd_no_priv changing 4 files +185 −18

jonasschnelli commented at 11:00 am on December 7, 2016: contributor
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.
jonasschnelli added the label Wallet on Dec 7, 2016
jonasschnelli added this to the milestone 0.14.0 on Dec 7, 2016
jonasschnelli force-pushed on Dec 7, 2016
instagibbs commented at 1:42 pm on December 7, 2016: member

Can you explain a bit of motivation for this?
jonasschnelli force-pushed on Dec 7, 2016
jonasschnelli commented at 2:09 pm on December 7, 2016: contributor

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.

in src/wallet/wallet.h: in 2b61ef8d2a outdated

82@@ -83,6 +83,7 @@ enum WalletFeature
83     FEATURE_COMPRPUBKEY = 60000, // compressed public keys
84 
85     FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet)
86+    FEATURE_WITH_HD_PUBKEY = 139900, // Hierarchical key derivation after BIP32 (HD Wallet)

paveljanik commented at 1:27 pm on December 8, 2016:

The comment here should be different compared to the FEATURE_HD.

in src/wallet/wallet.cpp: in 2b61ef8d2a outdated

2582@@ -2475,7 +2583,7 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2583 
2584                     if (!signSuccess)
2585                     {
2586-                        strFailReason = _("Signing transaction failed");
2587+                        strFailReason = _("Signing transaction failed ")+(sign ? std::string("yes"): std::string("no"));

paveljanik commented at 1:28 pm on December 8, 2016:

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

MarcoFalke commented at 2:17 pm on December 8, 2016: member

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…
jonasschnelli force-pushed on Dec 8, 2016
[Wallet] use CHDPubKey, don't store child priv keys in db, derive them on the fly 3fd850ae5e
jonasschnelli force-pushed on Dec 8, 2016
jonasschnelli commented at 4:27 pm on December 8, 2016: contributor
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.
1. We already recover the hdchain (child key counter, private key id thats used for the HD master key).
2. Don’t abort if a privat-key is corrupted, instead…
3. 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?
jonasschnelli removed this from the milestone 0.14.0 on Jan 12, 2017

in src/wallet/wallet.cpp:167 in 3fd850ae5e

162 
163+    CPubKey pubkey = secret.GetPubKey();
164+    assert(secret.VerifyPubKey(pubkey));
165+
166+    // store metadata
167+    mapKeyMetadata[pubkey.GetID()] = metadata;

promag commented at 2:05 pm on October 10, 2017:

This could stay in GenerateNewKey?

in src/wallet/wallet.cpp:168 in 3fd850ae5e

163+    CPubKey pubkey = secret.GetPubKey();
164+    assert(secret.VerifyPubKey(pubkey));
165+
166+    // store metadata
167+    mapKeyMetadata[pubkey.GetID()] = metadata;
168+    if (!nTimeFirstKey || metadata.nCreateTime < nTimeFirstKey)

promag commented at 2:06 pm on October 10, 2017:

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

in src/wallet/wallet.cpp:242 in 3fd850ae5e

237+    return CCryptoKeyStore::HaveKey(address);
238+}
239+
240+bool CWallet::LoadHDPubKey(const CHDPubKey &hdPubKey)
241+{
242+    AssertLockHeld(cs_wallet); // mapKeyMetadata

promag commented at 2:14 pm on October 10, 2017:

Wrong comment.

in src/wallet/wallet.cpp:250 in 3fd850ae5e

245+    return true;
246+}
247+
248+bool CWallet::AddHDPubKey(const CExtPubKey &extPubKey)
249+{
250+    AssertLockHeld(cs_wallet); // mapKeyMetadata

promag commented at 2:14 pm on October 10, 2017:

Wrong comment.

promag commented at 2:15 pm on October 10, 2017: member

@jonasschnelli Still interested in pushing this forward?

Needs rebase.

Should have a test.
sipa commented at 5:25 pm on March 6, 2018: member

@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?
MarcoFalke added the label Needs rebase on Jun 6, 2018
DrahtBot commented at 11:20 pm on November 8, 2018: member
DrahtBot added the label Up for grabs on Nov 8, 2018
DrahtBot closed this on Nov 8, 2018
laanwj removed the label Needs rebase on Oct 24, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors
jonasschnelli instagibbs paveljanik MarcoFalke promag sipa DrahtBot

Labels
Wallet Up for grabs