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

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

Closed

Conversation

Projects
None yet
7 participants
@jonasschnelli
Copy link
Member

commented Dec 7, 2016

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 jonasschnelli added the Wallet label Dec 7, 2016

@jonasschnelli jonasschnelli added this to the 0.14.0 milestone Dec 7, 2016

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_no_priv branch Dec 7, 2016

@instagibbs

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

Can you explain a bit of motivation for this?

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_no_priv branch Dec 7, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2016

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.

src/wallet/wallet.h Outdated
@@ -83,6 +83,7 @@ enum WalletFeature
FEATURE_COMPRPUBKEY = 60000, // compressed public keys

FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet)
FEATURE_WITH_HD_PUBKEY = 139900, // Hierarchical key derivation after BIP32 (HD Wallet)

This comment has been minimized.

Copy link
@paveljanik

paveljanik Dec 8, 2016

Contributor

The comment here should be different compared to the FEATURE_HD.

src/wallet/wallet.cpp Outdated
@@ -2475,7 +2583,7 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt

if (!signSuccess)
{
strFailReason = _("Signing transaction failed");
strFailReason = _("Signing transaction failed ")+(sign ? std::string("yes"): std::string("no"));

This comment has been minimized.

Copy link
@paveljanik

paveljanik Dec 8, 2016

Contributor

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_no_priv branch Dec 8, 2016

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_no_priv branch to 3fd850a Dec 8, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2016

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?

@promag
Copy link
Member

left a comment

@jonasschnelli Still interested in pushing this forward?

Needs rebase.

Should have a test.

assert(secret.VerifyPubKey(pubkey));

// store metadata
mapKeyMetadata[pubkey.GetID()] = metadata;

This comment has been minimized.

Copy link
@promag

promag Oct 10, 2017

Member

This could stay in GenerateNewKey?


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

This comment has been minimized.

Copy link
@promag

promag Oct 10, 2017

Member

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


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

This comment has been minimized.

Copy link
@promag

promag Oct 10, 2017

Member

Wrong comment.


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

This comment has been minimized.

Copy link
@promag

promag Oct 10, 2017

Member

Wrong comment.

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

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.

@DrahtBot DrahtBot closed this Nov 8, 2018

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.