Probably most simplest HD implementation.
--usehd=0CHDChain) which contains the CKeyID for the master key together with the child key counterI'll add some tests once there are some conceptual acks (and also to keep the diff at a very minimum).
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.
3103 | @@ -3036,6 +3104,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug) 3104 | strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), DEFAULT_SEND_FREE_TRANSACTIONS)); 3105 | strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE)); 3106 | 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)); 3107 | + 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));
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/
603 | + { 604 | + CHDChain chain; 605 | + ssValue >> chain; 606 | + if (!pwallet->SetHDChain(chain, true)) 607 | + { 608 | + strErr = "Error reading wallet database: AddChain failed";
s/AddChain/SetHDChain/
598 | @@ -599,6 +599,16 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, 599 | return false; 600 | } 601 | } 602 | + else if (strType == "hdchain")
Repeating "hdchain" several times reminds me there was some patch redefining these as constants...
Which one? We did the redefine-magic-strings-as-constants it in other places, but also for the wallet?
Yes, because some of them are repeated several times in the file. But maybe I'm remembering some other patch.
Ah, it probably was #5707.
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.
Definitely not for this PR, of course.
Concept ACK
Nice, small, elegant 👍
What should RPC dumpwallet dump after this?
Some import path needed then.
Concept ACK
@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'.
Fixed @paveljanik nits.
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.
1086 | @@ -1051,6 +1087,37 @@ CAmount CWallet::GetChange(const CTransaction& tx) const 1087 | return nChange; 1088 | } 1089 | 1090 | +bool CWallet::SetHDMasterKey(const CKey& key) 1091 | +{ 1092 | + LOCK(cs_wallet); 1093 | + 1094 | + // store the key as normal "key"/"ckey" object 1095 | + // in the the database
the the
56 | @@ -57,6 +57,9 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2; 57 | static const unsigned int MAX_FREE_TRANSACTION_CREATE_SIZE = 1000; 58 | static const bool DEFAULT_WALLETBROADCAST = true; 59 | 60 | +//! if set, all keys will be deriven by using BIP32
derived
39 | @@ -40,6 +40,34 @@ enum DBErrors 40 | DB_NEED_REWRITE 41 | }; 42 | 43 | +/* simple hd chain data model */ 44 | +class CHDChain 45 | +{ 46 | +public: 47 | + uint32_t nExternalChainCounter; 48 | + CKeyID masterKeyID; /* master key hash160 */
two spaces
Maybe just clang-format the diff?
Force push fixed @paveljanik nits. Formatted some of the code with clang-format.
119 | + 120 | + // derive m/0'/0' 121 | + accountKey.Derive(externalChainChildKey, 0 | 0x80000000); 122 | + 123 | + // derive child key at next index 124 | + externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | 0x80000000);
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.
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.
@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.
576 | @@ -574,6 +577,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface 577 | 578 | void SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator>); 579 | 580 | + /* the hd chain data model (internal and external counters) */
external only for now? (and in the other 2 places in the PR)
39 | @@ -40,6 +40,35 @@ enum DBErrors 40 | DB_NEED_REWRITE 41 | }; 42 | 43 | +/* simple hd chain data model */ 44 | +class CHDChain 45 | +{ 46 | +public: 47 | + uint32_t nExternalChainCounter; 48 | + CKeyID masterKeyID; /* master key hash160 */
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
1095 | + 1096 | + // store the key as normal "key"/"ckey" object 1097 | + // in the database 1098 | + // key metadata is not required 1099 | + CPubKey pubkey = key.GetPubKey(); 1100 | + if (!AddKeyPubKey(key, pubkey))
nit: simply calling AddKey should work here nevermind, wrong function
I'd like to know why this comment does not apply.
EDIT: because you need pubkey below anyway, ok!
utACK https://github.com/bitcoin/bitcoin/pull/8035/commits/cc1df8fa6f4849b65187f60a3cb4c83da3d16788
Glad to see a really simple PR. Baby steps better than no steps.
Added a squash me commit with some non-code nit fixes.
108 | + 109 | + // try to get the master key 110 | + if (!GetKey(hdChain.masterKeyID, key)) 111 | + throw std::runtime_error("CWallet::GenerateNewKey(): Master key not found"); 112 | + 113 | + assert(key.size() == 32);
Why is this an assertion but missing the master key is an exception?
I agree. This assertion should be removed.
112 | + 113 | + assert(key.size() == 32); 114 | + masterKey.SetMaster(key.begin(), key.size()); 115 | + 116 | + // derive m/0' 117 | + masterKey.Derive(accountKey, 0 | 0x80000000);
Needs comment about hardened derivation.
130 | + 131 | + // update the chain model in the database 132 | + if (!CWalletDB(strWalletFile).WriteHDChain(hdChain)) 133 | + throw std::runtime_error("CWallet::GenerateNewKey(): Writing HD chain model failed"); 134 | + } else 135 | + secret.MakeNewKey(fCompressed);
Please add braces here for the else block.
our clang format script accepted that. What's wrong with it? :-)
I think that is what clang-tidy is supposed to do ;)
Added another squash-me commit with minor nit fixes.
@jonasschnelli clang-format does not reject anything, it just reformats.
@sipa: right! Sorry. I meant it didn't reformat the else part.
@jonasschnelli It can't reformat it, because all it does is change whitespace. What we want here is more than just changing whitespace, but there is no way to make clang do that (and also not make it yell about it). Just because a whitespace formatting tool accepts the code, doesn't mean it satisfies all our style guidelines. For example, it also does not check consistency of variable names.
Amend force pushed last squash-me commit with the else + bracket change.
112 | + 113 | + masterKey.SetMaster(key.begin(), key.size()); 114 | + 115 | + // derive m/0' 116 | + // use hardened derivation (child keys > 0x80000000 are hardened after bip32) 117 | + masterKey.Derive(accountKey, 0 | 0x80000000);
Sorry, I don't understand this | 0x80000000 from the comments...
Hardened keys are >= 0x80000000, not > according to bip32
It's specified here: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
You can derive 2^32 key, child keys <= 2147483648 (<=0x80000000) are non-hardened key. Derived child keys > 2147483648 are hardened keys.
This line:
masterKey.Derive(accountKey, 0 | 0x80000000);
.. is equal to ..
masterKey.Derive(accountKey, 0 + 2147483648)
I think the comment has a typo?
Oh, I forgot about this, thanks. IIRC, pycoin (what I've used for bip32 so far) used an asterisk in its interface for hardened keys. I wouldn't complain if the link was added to the comment, but maybe an appropriately named constant for 0x80000000 would be enough.
Agree about adding a named constant.
I still have limited knowledge of the wallet, but thanks to this being so simple I was able to review it and everything looks good. something-between-concept-and-ut ACK
Can one of the admins verify this patch?
Rebased. Squashed squash-me commits.
Doesn't build here:
wallet/wallet.cpp:3219:9: error: use of undeclared identifier 'RandAddSeedPerfmon'
See #7891, it should be enough to remove it.
I don't think this should be added to doc/bips.md, as this pull does not add any interface at all. Though, the release notes should be updated.
111 | + throw std::runtime_error("CWallet::GenerateNewKey(): Master key not found"); 112 | + 113 | + masterKey.SetMaster(key.begin(), key.size()); 114 | + 115 | + // derive m/0' 116 | + // use hardened derivation (child keys > 0x80000000 are hardened after bip32)
comment-nit: It is >= 0x80000000
Fixed and force pushed the rebase issue RandAddSeedPerfmon (#7891).
utACK, will test. There are a few nits left to address.
Added a commit that...
0 | 0x80000000 in 0x80000000 (and is now a const)
This is a high-profile feature. Definitely needs mention in release notes.
Okay. I'll write something for the release notes.
Added a part in the release-notes.md (thanks for lang review).
Updated the bips.md.
116 | +----------------------------------------- 117 | +Newly created wallets will use hierarchical deterministic key generation 118 | +according to BIP32 (keypath m/0'/0'/k'). 119 | +Existing wallets will still use traditional key generation. 120 | + 121 | +Backups, regardless of when they have been created, can therefore be
Nit: Let's call it "Backups of HD wallets"?
117 | +Newly created wallets will use hierarchical deterministic key generation 118 | +according to BIP32 (keypath m/0'/0'/k'). 119 | +Existing wallets will still use traditional key generation. 120 | + 121 | +Backups, regardless of when they have been created, can therefore be 122 | +used to re-generate all possible private keys, even the ones who hasn't
"even the ones which haven't already"?
3131 | @@ -3058,6 +3132,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug) 3132 | strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), DEFAULT_SEND_FREE_TRANSACTIONS)); 3133 | strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE)); 3134 | 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)); 3135 | + strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key generation (HD) after bip32. Only has effect during wallet creation/first start") + " " + strprintf(_("(default: %u)"), DEFAULT_USE_HD_WALLET));
Nit: Might as well call this -createhdwallet or similar. Using a lengthy and more verbose option does not hurt if it is expected to be changed from its default only very rarely.
Agree that -createhdwallet would make more sense.
I prefer -usehd, but adding a check that if it is given in case there already exists a wallet, we verify that it matches that wallet. That way you won't ever get any surprises, just set -usehd, and you're sure you'll always use HD derivation, set -usehd=0 and you're sure you'll never.
That's similar to how -txindex works: if given, it must match the existing tree.
@sipa If you add the check that your wallet must match with -usehd you can't set the default to true, I assume. Otherwise the a user switching from 0.12.1 to 0.13 would see a InitError or InitWarning pop up on every start.
If -usehd is not explicitly given, default to whatever is the current wallet (or true for new wallets). if it is explicitly given, it must match the existing wallet.
Ok makes sense. (But that is different from txindex, as I remember you can't set -txindex=1 once and then remove it, to make it default to true. I may be mistaken but I should better not hijack this pull's discussion.)
@MarcoFalke it's slightly different, yes, but txindex behavior wouldn't make sense since you can't "reindex" your HD wallet to legacy.
Added a commit.
Now the PR detects if a user likes to disabled hd (with -usehd=0) when using a hd wallet.dat file.
The PR also detects if a user likes to enable hd (with -usehd=1) when using a non hd wallet.dat file.
The node will shutdown/stop init in a such mismatch situation.
utACK 71ff55fcc2245e03c786d7e0312755e812d9de8e
3235 | @@ -3236,6 +3236,14 @@ bool CWallet::InitLoadWallet() 3236 | 3237 | walletInstance->SetBestChain(chainActive.GetLocator()); 3238 | } 3239 | + else
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.
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.
Post merge utACK. Will also test.
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.
@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.
@jonasschnelli Yes that's fine. I just wanted to make sure there is no "partial overlap" with the bitcoinj impl.
So, how does one import a BIP32 master key into an existing wallet?
I'm switching to Core (from Electrum) after this lands in 0.13, thanks for your efforts.
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.
1099 | + // store the key as normal "key"/"ckey" object 1100 | + // in the database 1101 | + // key metadata is not required 1102 | + CPubKey pubkey = key.GetPubKey(); 1103 | + if (!AddKeyPubKey(key, pubkey)) 1104 | + throw std::runtime_error("CWallet::GenerateNewKey(): AddKey failed");
Nit: Wrong function name
Post-merge review comments, sorry :/ (continued from IRC): There is also a comment on #8324 and other related PRs here.
I think all but the first could easily be fixed even now, if we decide they are, indeed, issues.
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 (https://github.com/bitcoin/bitcoin/pull/6265 #7273) and I was looking for a simpler solutions. There is a PR to identify the seed when dumping (https://github.com/bitcoin/bitcoin/pull/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.
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.
I foget my sceond wallet passwor. Thats why i cant get backup. I need help
src/wallet/walletdb.h