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] Add simplest BIP32/deterministic key generation implementation #8035

Merged
merged 4 commits into from Jun 14, 2016

Conversation

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented May 10, 2016

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
Copy link

@slush0 slush0 commented May 10, 2016

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
paveljanik reviewed May 10, 2016
View changes
src/wallet/wallet.cpp Outdated
@@ -3036,6 +3104,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), DEFAULT_SEND_FREE_TRANSACTIONS));
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key generation (HD) after bip32. Has only affect during wallet creation/first start") + " " + strprintf(_("(default: %u)"), DEFAULT_USE_HD_WALLET));

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

No need for strprintf. See previous line.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 10, 2016
Author Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke May 10, 2016
Member

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.

This comment has been minimized.

@instagibbs

instagibbs May 12, 2016
Member

s/Has only affect/Only has effect/

@paveljanik
paveljanik reviewed May 10, 2016
View changes
src/wallet/walletdb.cpp Outdated
ssValue >> chain;
if (!pwallet->SetHDChain(chain, true))
{
strErr = "Error reading wallet database: AddChain failed";

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

s/AddChain/SetHDChain/

@@ -599,6 +599,16 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
return false;
}
}
else if (strType == "hdchain")

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

Repeating "hdchain" several times reminds me there was some patch redefining these as constants...

This comment has been minimized.

@laanwj

laanwj May 10, 2016
Member

Which one? We did the redefine-magic-strings-as-constants it in other places, but also for the wallet?

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

Yes, because some of them are repeated several times in the file. But maybe I'm remembering some other patch.

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

Ah, it probably was #5707.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 10, 2016
Author Member

Yes. I did refactor out these stings into constants serval times. But this should be done in another PR and hopefully after this has been merged.

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

Definitely not for this PR, of course.

@paveljanik
Copy link
Contributor

@paveljanik paveljanik commented May 10, 2016

Concept ACK

Nice, small, elegant 👍

@paveljanik
Copy link
Contributor

@paveljanik paveljanik commented May 10, 2016

What should RPC dumpwallet dump after this?

@paveljanik
Copy link
Contributor

@paveljanik paveljanik commented May 10, 2016

Some import path needed then.

@laanwj
Copy link
Member

@laanwj laanwj commented May 10, 2016

Concept ACK

@jonasschnelli
Copy link
Member Author

@jonasschnelli jonasschnelli commented May 10, 2016

@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 jonasschnelli force-pushed the jonasschnelli:2016/05/simplest_hd branch May 10, 2016
@jonasschnelli
Copy link
Member Author

@jonasschnelli jonasschnelli commented May 10, 2016

Fixed @paveljanik nits.

@slush0
Copy link

@slush0 slush0 commented May 10, 2016

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
paveljanik reviewed May 10, 2016
View changes
src/wallet/wallet.cpp Outdated
LOCK(cs_wallet);

// store the key as normal "key"/"ckey" object
// in the the database

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

the the

@paveljanik
paveljanik reviewed May 10, 2016
View changes
src/wallet/wallet.h Outdated
@@ -57,6 +57,9 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2;
static const unsigned int MAX_FREE_TRANSACTION_CREATE_SIZE = 1000;
static const bool DEFAULT_WALLETBROADCAST = true;

//! if set, all keys will be deriven by using BIP32

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

derived

@paveljanik
paveljanik reviewed May 10, 2016
View changes
src/wallet/walletdb.h Outdated
{
public:
uint32_t nExternalChainCounter;
CKeyID masterKeyID; /* master key hash160 */

This comment has been minimized.

@paveljanik

paveljanik May 10, 2016
Contributor

two spaces

This comment has been minimized.

@MarcoFalke

MarcoFalke via email May 10, 2016
Member

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/05/simplest_hd branch May 10, 2016
@jonasschnelli
Copy link
Member Author

@jonasschnelli jonasschnelli commented May 10, 2016

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

@sipa
sipa reviewed May 10, 2016
View changes
src/wallet/wallet.cpp Outdated
accountKey.Derive(externalChainChildKey, 0 | 0x80000000);

// derive child key at next index
externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | 0x80000000);

This comment has been minimized.

@sipa

sipa May 10, 2016
Member

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...

This comment has been minimized.

@jonasschnelli

jonasschnelli May 11, 2016
Author Member

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
Copy link
Member

@achow101 achow101 commented May 11, 2016

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
Copy link
Member Author

@jonasschnelli jonasschnelli commented May 11, 2016

@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.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/05/simplest_hd branch 2 times, most recently May 11, 2016
@instagibbs
instagibbs reviewed May 12, 2016
View changes
src/wallet/wallet.h Outdated
@@ -574,6 +577,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface

void SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator>);

/* the hd chain data model (internal and external counters) */

This comment has been minimized.

@instagibbs

instagibbs May 12, 2016
Member

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

@MarcoFalke
MarcoFalke reviewed May 12, 2016
View changes
src/wallet/walletdb.h Outdated
{
public:
uint32_t nExternalChainCounter;
CKeyID masterKeyID; /* master key hash160 */

This comment has been minimized.

@MarcoFalke

MarcoFalke May 12, 2016
Member

Nit: I think you want to use //!< when placing a trailing comment.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#doxygen-comments

// in the database
// key metadata is not required
CPubKey pubkey = key.GetPubKey();
if (!AddKeyPubKey(key, pubkey))

This comment has been minimized.

@instagibbs

instagibbs May 12, 2016
Member

nit: simply calling AddKey should work here nevermind, wrong function

This comment has been minimized.

@sipa

sipa Jun 1, 2016
Member

I'd like to know why this comment does not apply.

EDIT: because you need pubkey below anyway, ok!

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 13, 2016

utACK 71ff55fcc2245e03c786d7e0312755e812d9de8e

@laanwj
laanwj reviewed Jun 14, 2016
View changes
src/wallet/wallet.cpp Outdated
@@ -3236,6 +3236,14 @@ bool CWallet::InitLoadWallet()

walletInstance->SetBestChain(chainActive.GetLocator());
}
else

This comment has been minimized.

@laanwj

laanwj Jun 14, 2016
Member

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)

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 14, 2016
Author Member

Thanks. Much simpler.
Force push updated last commit.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/05/simplest_hd branch to afcd77e Jun 14, 2016
@laanwj
Copy link
Member

@laanwj laanwj commented Jun 14, 2016

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.

@laanwj laanwj merged commit afcd77e into bitcoin:master Jun 14, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 14, 2016
… 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
Copy link
Contributor

@gmaxwell gmaxwell commented Jun 15, 2016

Post merge utACK. Will also test.

@schildbach
Copy link
Contributor

@schildbach schildbach commented Jun 15, 2016

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
Copy link
Member Author

@jonasschnelli jonasschnelli commented Jun 17, 2016

@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
Copy link
Contributor

@schildbach schildbach commented Jun 17, 2016

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

@rebroad
Copy link
Contributor

@rebroad rebroad commented Jun 25, 2016

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

@diegoviola
Copy link
Contributor

@diegoviola diegoviola commented Jun 25, 2016

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

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 27, 2016

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.

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 8, 2016
Member

Nit: Wrong function name

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Jul 19, 2016

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
Copy link
Member Author

@jonasschnelli jonasschnelli commented Jul 19, 2016

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
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Jul 19, 2016

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
Copy link

@zihad944 zihad944 commented Aug 11, 2018

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

@jeandudey
Copy link

@jeandudey jeandudey commented Aug 11, 2018

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

@MarkLTZ MarkLTZ mentioned this pull request Apr 30, 2019
53 of 77 tasks complete
Copy link

@sorawee1970 sorawee1970 left a comment

src/wallet/walletdb.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.