[Wallet] Simple HD/BIP32 support #7273

jonasschnelli · 2016-01-02T16:04:13Z

Most simplest HD feature.

Use BIP32/hd key derivation by default for new wallets (m/0'/0')
Only private key derivation is supported (no pubkey-only-wallets for now)
No on the fly generation of private keys (keypools get still refilled, all derived private keys are touching the database, but deterministic)
Internal support for multiple hd keychains (potential allows simple key rotation feature)
Supports encrypted wallets
RPC- and unit-tests included
Currently now way to create a hd keychain for existing wallets (upcoming feature if/once this got merged)

ment as a starting point, have concrete plans to extend this, but don't want to overload this PR

getnewaddress "" true (last parameter true = "show details") returns an object if the new address was hd derived:

{
  "address": "n1EU7TC4YqVYGQYy5eav1APHhS3z3Jrgf4",
  "keypath": "m/0'/230'"
}

(same for getaddressesbyaccount)

jonasschnelli · 2016-01-02T16:13:01Z

The main benefits of this PR:

A wallet.dat backup, regardless of it's age, can be used to recover all private keys.
Basic groundwork for detaching the private-keys from the wallet (allow signing over hardware wallets, etc.)

maflcko · 2016-01-02T18:01:35Z

qa/rpc-tests/hdwallet.py

+        print "encrypt wallet"
+        self.nodes[0].encryptwallet("test")
+        bitcoind_processes[0].wait()
+        del bitcoind_processes[0]


Nit: Is there a compelling reason to do this?

Encrypting the wallet does stop the process,... i think this is required.

jgarzik · 2016-01-02T18:21:43Z

concept ACK

luke-jr · 2016-01-15T18:00:20Z

src/wallet/walletdb.h

+    bool WriteHDCryptedMasterSeed(const uint256& hash, const std::vector<unsigned char>& vchCryptedSecret);
+    bool EraseHDMasterSeed(const uint256& hash);
+    bool WriteHDChain(const CHDChain& chain);
+    bool WriteHDAchiveChain(const uint256& hash);


Typo? Achive -> Active?

luke-jr · 2016-01-15T18:12:32Z

Note to test: If encrypting wallet fails for any reason at any late stage, the wallet should retain all unencrypted data.

luke-jr · 2016-01-15T18:18:18Z

src/wallet/wallet.cpp

+    EncryptSeeds(vMasterKeyIn);
+
+    std::vector<HDChainID> chainIds;
+    GetAvailableChainIDs(chainIds);


Return value is ignored.

pointbiz · 2016-01-22T21:10:19Z

Concept ACK

instagibbs · 2016-02-01T15:12:55Z

src/init.cpp

@@ -395,6 +395,8 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
    strUsage += HelpMessageOpt("-mintxfee=<amt>", strprintf(_("Fees (in %s/kB) smaller than this are considered zero fee for transaction creation (default: %s)"),
            CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)));
+    strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key derivation (HD wallets) (default: true)"));
+    strUsage += HelpMessageOpt("-hdseed", _("Use the given 256bit (64 char hex) as HD master seed (default: <generate random seed>)"));


Probably should make this clear it only happens upon first run of the wallet.

Probably should make this clear it only happens upon first run of the wallet.

I agree, -hdseed as startup parameter is somehow not ideal. Not seeding the wallet at first start, would require an additional rpc command (something generatehdwallet, seedwallet, etc.).

Or an additional "bitcoin-wallet" tool that could allow generating and manipulating wallet.dat files.

But because I fear lack of reviewer, I don't want to make this PR more complex for now.

jonasschnelli · 2016-02-22T12:38:59Z

Rebased and fixed reported nits.

instagibbs · 2016-03-11T21:45:20Z

src/wallet/walletdb.cpp

+        return false;
+
+    Erase(std::make_pair(std::string("hdmasterseed"), hash));
+    Erase(std::make_pair(std::string("hdmasterseed"), hash));


double deletion?

- master seeds are kept seperated and can be encrypted

jonasschnelli · 2016-03-14T08:57:30Z

Rebased and fixed some nits.

instagibbs · 2016-03-14T17:35:32Z

Concept ACK.

I think on-ramping users to this safely/transparently will be the most difficult part.

Warning users that the flag they've set can't be properly used until a new wallet is created(or some migration happens) is a must imo. There needs to be a tool/process to help with that migration, most likely with a backup/cloning step.

jonasschnelli · 2016-05-10T09:26:39Z

Closing in favor of #8035.
Some parts of this PR could be used to extend HD functionality if and once #8035 has been merged.

maflcko · 2016-05-15T13:22:16Z

@jonasschnelli Is it ok to delete the 2016/01/hdsimple branch from https://github.com/bitcoin/bitcoin/branches now?

jonasschnelli · 2016-05-16T13:45:58Z

I deleted the branch. There is still some useful stuff in this branch (thinks like child key derivation by keypath-string) that could be re-used later.

jonasschnelli added the Wallet label Jan 2, 2016

jonasschnelli added this to the 0.13.0 milestone Jan 2, 2016

maflcko reviewed Jan 2, 2016
View reviewed changes

luke-jr reviewed Jan 15, 2016
View reviewed changes

instagibbs reviewed Feb 1, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/01/hdsimple branch from 42d2a1a to 915b69d Compare February 22, 2016 12:37

instagibbs reviewed Mar 11, 2016
View reviewed changes

jonasschnelli added 3 commits March 14, 2016 09:42

[Wallet] Add hdkeystore

6a65143

- master seeds are kept seperated and can be encrypted

[Wallet] Extend walletdb with some hd related records

b717b42

[Wallet] use HD by default for all new wallets

3494217

jonasschnelli force-pushed the 2016/01/hdsimple branch from 915b69d to 3494217 Compare March 14, 2016 08:56

jonasschnelli mentioned this pull request May 9, 2016

Increase DEFAULT_KEYPOOL_SIZE to 10000. #8025

Closed

jonasschnelli closed this May 10, 2016

jonasschnelli mentioned this pull request May 11, 2016

[Wallet] Add simplest BIP32/deterministic key generation implementation #8035

Merged

jonasschnelli deleted the 2016/01/hdsimple branch May 16, 2016 13:44

jonasschnelli removed this from the 0.13.0 milestone May 16, 2016

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[Wallet] Simple HD/BIP32 support #7273

[Wallet] Simple HD/BIP32 support #7273

Uh oh!

Conversation

jonasschnelli commented Jan 2, 2016

Uh oh!

jonasschnelli commented Jan 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgarzik commented Jan 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pointbiz commented Jan 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Mar 14, 2016

Uh oh!

instagibbs commented Mar 14, 2016

Uh oh!

jonasschnelli commented May 10, 2016

Uh oh!

maflcko commented May 15, 2016

Uh oh!

jonasschnelli commented May 16, 2016

Uh oh!

Uh oh!