[Wallet] Simple HD/BIP32 support #7273

jonasschnelli commented at 4:04 PM on January 2, 2016: contributor

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 added the label Wallet on Jan 2, 2016

jonasschnelli added this to the milestone 0.13.0 on Jan 2, 2016

jonasschnelli commented at 4:13 PM on January 2, 2016: contributor

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

in qa/rpc-tests/hdwallet.py:None in 42d2a1a37b outdated

  64 | +        assert_equal(adr['keypath'], "m/0'/3'");
  65 | +        
  66 | +        print "encrypt wallet"
  67 | +        self.nodes[0].encryptwallet("test")
  68 | +        bitcoind_processes[0].wait()
  69 | +        del bitcoind_processes[0]

MarcoFalke commented at 6:01 PM on January 2, 2016:

Nit: Is there a compelling reason to do this?

jonasschnelli commented at 6:26 PM on January 2, 2016:

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

ali8889 commented at 4:56 PM on September 25, 2018:

Riky

ali8889 commented at 4:56 PM on September 25, 2018:

Riky

in src/init.cpp:None in 42d2a1a37b outdated

 394 | @@ -395,6 +395,8 @@ std::string HelpMessage(HelpMessageMode mode)
 395 |      strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
 396 |      strUsage += HelpMessageOpt("-mintxfee=<amt>", strprintf(_("Fees (in %s/kB) smaller than this are considered zero fee for transaction creation (default: %s)"),
 397 |              CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)));
 398 | +    strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key derivation (HD wallets) (default: true)"));

MarcoFalke commented at 6:02 PM on January 2, 2016:

We no longer translate every time the default changes.

MarcoFalke commented at 3:22 PM on February 1, 2016:

Also, true is not a valid default value.

in src/init.cpp:None in 42d2a1a37b outdated

1481 | +                CKey key;
1482 | +                key.MakeNewKey(true); //generate a seed
1483 | +                CKeyingMaterial seed = CKeyingMaterial(32);
1484 | +                seed.assign(key.begin(), key.end());
1485 | +
1486 | +                if (GetArg("-hdseed", "").size() == 64)

MarcoFalke commented at 6:04 PM on January 2, 2016:

Shouldn't this fail otherwise?

jonasschnelli commented at 6:27 PM on January 2, 2016:

Yes. Good point. Will fix.

jonasschnelli commented at 8:46 AM on March 14, 2016:

Fixed.

in src/wallet/hdkeystore.cpp:None in 42d2a1a37b outdated

 165 | +    return true;
 166 | +}
 167 | +
 168 | +bool CHDKeyStore::DeriveKey(const HDChainID chainID, const std::string keypath, CKey& keyOut) const
 169 | +{
 170 | +    //this methode required no locking

MarcoFalke commented at 6:05 PM on January 2, 2016:

Nit: typo

in src/wallet/rpcwallet.cpp:None in 42d2a1a37b outdated

 327 | @@ -317,12 +328,13 @@ UniValue getaddressesbyaccount(const UniValue& params, bool fHelp)
 328 |      if (!EnsureWalletIsAvailable(fHelp))
 329 |          return NullUniValue;
 330 |      
 331 | -    if (fHelp || params.size() != 1)
 332 | +    if (fHelp || params.size() > 2)

MarcoFalke commented at 6:07 PM on January 2, 2016:

The first arg is required

in src/wallet/rpcwallet.cpp:None in 42d2a1a37b outdated

 357 | @@ -344,7 +358,27 @@ UniValue getaddressesbyaccount(const UniValue& params, bool fHelp)
 358 |          const CBitcoinAddress& address = item.first;
 359 |          const string& strName = item.second.name;
 360 |          if (strName == strAccount)
 361 | -            ret.push_back(address.ToString());
 362 | +        {
 363 | +            if (GetBoolArg("-usehd", true) && showDetails)

MarcoFalke commented at 6:09 PM on January 2, 2016:

Guess it makes sense to use a global or DEFAULT_ here.

jgarzik commented at 6:21 PM on January 2, 2016: contributor

concept ACK

in src/wallet/walletdb.h:None in 42d2a1a37b outdated

 100 | @@ -133,6 +101,13 @@ class CWalletDB : public CDB
 101 |      static bool Recover(CDBEnv& dbenv, const std::string& filename, bool fOnlyKeys);
 102 |      static bool Recover(CDBEnv& dbenv, const std::string& filename);
 103 |  
 104 | +    /* HD functions */
 105 | +    bool WriteHDMasterSeed(const uint256& hash, const CKeyingMaterial& masterSeed);
 106 | +    bool WriteHDCryptedMasterSeed(const uint256& hash, const std::vector<unsigned char>& vchCryptedSecret);
 107 | +    bool EraseHDMasterSeed(const uint256& hash);
 108 | +    bool WriteHDChain(const CHDChain& chain);
 109 | +    bool WriteHDAchiveChain(const uint256& hash);

luke-jr commented at 6:00 PM on January 15, 2016:

Typo? Achive -> Active?

in src/wallet/hdkeystore.h:None in 42d2a1a37b outdated

 105 | +};
 106 | +
 107 | +class CHDKeyStore : public CCryptoKeyStore
 108 | +{
 109 | +protected:
 110 | +    std::map<HDChainID, CKeyingMaterial > mapHDMasterSeeds; //master seeds are stored outside of CHDChain (mind crypting)

luke-jr commented at 6:11 PM on January 15, 2016:

mind crypting???

luke-jr commented at 6:12 PM on January 15, 2016: member

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

in src/wallet/wallet.cpp:None in 42d2a1a37b outdated

1009 | +bool CWallet::EncryptHDSeeds(CKeyingMaterial& vMasterKeyIn)
1010 | +{
1011 | +    EncryptSeeds(vMasterKeyIn);
1012 | +
1013 | +    std::vector<HDChainID> chainIds;
1014 | +    GetAvailableChainIDs(chainIds);

luke-jr commented at 6:18 PM on January 15, 2016:

Return value is ignored.

jonasschnelli commented at 12:38 PM on February 22, 2016:

Fixed.

ali8889 commented at 7:33 AM on September 24, 2018:

Oky

ali8889 commented at 7:33 AM on September 24, 2018:

Oky

ali8889 commented at 7:33 AM on September 24, 2018:

Oky

ali8889 commented at 7:33 AM on September 24, 2018:

Oky

ali8889 commented at 7:34 AM on September 24, 2018:

Oky

ali8889 commented at 7:34 AM on September 24, 2018:

Oky

in src/wallet/hdkeystore.h:None in 42d2a1a37b outdated

  36 | +    template <typename Stream, typename Operation>
  37 | +    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
  38 | +        READWRITE(this->nVersion);
  39 | +        nVersion = this->nVersion;
  40 | +        READWRITE(nCreateTime);
  41 | +        if (nVersion >= 2)

luke-jr commented at 7:32 PM on January 15, 2016:

2 is VERSION_SUPPORT_FLAGS...

jonasschnelli commented at 12:16 PM on February 22, 2016:

2 is VERSION_SUPPORT_FLAGS...

Not in core... :) But yes. Let me use three.

in src/wallet/rpcwallet.cpp:None in 42d2a1a37b outdated

 134 | +    {
 135 | +        UniValue result(UniValue::VOBJ);
 136 | +        result.pushKV("address", CBitcoinAddress(keyID).ToString());
 137 | +        result.pushKV("keypath", pwalletMain->mapKeyMetadata[keyID].keypath);
 138 | +        return result;
 139 | +    }

luke-jr commented at 8:20 PM on January 15, 2016:

showDetails ought to return Object format even when not generating from a HD key IMO.

jonasschnelli commented at 12:19 PM on February 22, 2016:

showDetails ought to return Object format even when not generating from a HD key IMO.

It shouldn't (&& pwalletMain->mapKeyMetadata[keyID].keypath.size() > 0).

instagibbs commented at 4:24 PM on March 14, 2016:

I think @luke-jr meant prescriptively that the output should be same format regardless.

luke-jr commented at 5:03 AM on March 15, 2016:

Yes

pointbiz commented at 9:10 PM on January 22, 2016: none

Concept ACK

in src/init.cpp:None in 42d2a1a37b outdated

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

instagibbs commented at 3:12 PM on February 1, 2016:

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

jonasschnelli commented at 12:22 PM on February 22, 2016:

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 force-pushed on Feb 22, 2016

jonasschnelli commented at 12:38 PM on February 22, 2016: contributor

Rebased and fixed reported nits.

[Wallet] Add hdkeystore

- master seeds are kept seperated and can be encrypted

6a65143731

[Wallet] Extend walletdb with some hd related records b717b4258b

[Wallet] use HD by default for all new wallets 3494217323

in src/wallet/walletdb.cpp:None in 915b69dbde outdated

1048 | +    nWalletDBUpdated++;
1049 | +    if (!Write(std::make_pair(std::string("hdcryptedmasterseed"), hash), vchCryptedSecret))
1050 | +        return false;
1051 | +
1052 | +    Erase(std::make_pair(std::string("hdmasterseed"), hash));
1053 | +    Erase(std::make_pair(std::string("hdmasterseed"), hash));

instagibbs commented at 9:45 PM on March 11, 2016:

double deletion?

in src/wallet/hdkeystore.cpp:None in 915b69dbde outdated

 172 | +
 173 | +    keyOut = extKeyOut.key;
 174 | +    return true;
 175 | +}
 176 | +
 177 | +bool CHDKeyStore::DeriveKeyAtIndex(const HDChainID chainID, CKey& keyOut, std::string& keypathOut, unsigned int nIndex, bool internal) const

instagibbs commented at 9:48 PM on March 11, 2016:

arg sandwich: in/out/out/in/in

in src/wallet/hdkeystore.cpp:None in 915b69dbde outdated

 232 | +
 233 | +    for (unsigned int i=0;i<0x80000000;i++)
 234 | +        if (std::find(vIndices.begin(), vIndices.end(), i) == vIndices.end())
 235 | +            return i;
 236 | +
 237 | +    return 0;

instagibbs commented at 9:49 PM on March 11, 2016:

(nit?) technically this only returns when you have 0x80000000 keys already... clearly not a worry, but if reached should probably barf

in src/init.cpp:None in 915b69dbde outdated

1527 | @@ -1526,6 +1528,46 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1528 |              // Create new keyUser and set as default key
1529 |              RandAddSeedPerfmon();
1530 |  
1531 | +            if (GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET))

instagibbs commented at 9:54 PM on March 11, 2016:

I'd still prefer the user is warned that they are trying to use hd but already have a running non-hd wallet or simply be disallowed.

jonasschnelli commented at 8:51 AM on March 14, 2016:

Right. This PR currently only allows to use hd when no wallet.dat file is present (wallet creation phase). IMO this could be a save start,... first release that includes HD option should probably silently offer the feature with a more or less in-prominent startup argument. But agree, if someone passes -usehd while the current wallet is present and does not support HD, we should warn.

in qa/rpc-tests/hdwallet.py:None in 915b69dbde outdated

  70 | +
  71 | +        self.nodes[0] = start_node(0, self.options.tmpdir, extra_args=['-hdseed=f81a7a4efdc29e54dcc739df87315a756038d0b68fbc4880ffbbbef222152e6a'])
  72 | +        self.nodes[0].walletpassphrase("test", 100)
  73 | +        adr = self.nodes[0].getnewaddress("", True)
  74 | +        assert_equal(adr['address'], "n32WRiXX6P6KFkdGV37CAbsTjLcxt4VhRY");
  75 | +        assert_equal(adr['keypath'], "m/0'/5'");

instagibbs commented at 10:02 PM on March 11, 2016:

I'm missing the jump from 3 to 5 here.

jonasschnelli force-pushed on Mar 14, 2016

jonasschnelli commented at 8:57 AM on March 14, 2016: contributor

Rebased and fixed some nits.

instagibbs commented at 5:35 PM on March 14, 2016: member

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 commented at 9:26 AM on May 10, 2016: contributor

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

jonasschnelli closed this on May 10, 2016

MarcoFalke commented at 1:22 PM on May 15, 2016: member

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

jonasschnelli deleted the branch on May 16, 2016

jonasschnelli commented at 1:45 PM on May 16, 2016: contributor

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 removed this from the milestone 0.13.0 on May 16, 2016

MarcoFalke locked this on Sep 8, 2021