Add HD keypath to CKeyMetadata, report metadata in validateaddress #8323

jonasschnelli commented at 10:15 AM on July 9, 2016: contributor

I strongly recommend to merge this into 0.13 (current master or in 0.13 once we have split off).

Without this PRs CKeyMetadata extension, we will very likely run into problem to later identify which key are HD. Wallets in Core can always have non-hd keys along with hd-keys (through importwallet, importprivkey).

[Wallet] extend CKeyMetadata with HD keypath 5b95dd2c25

[Wallet] report optional HDKeypath/HDMasterKeyId in validateaddress b1c7b244e2

[Wallet] print hd masterkeyid in getwalletinfo 986c223214

[QA] extend wallet-hd test to cover HD metadata f70808596f

jonasschnelli added the label Wallet on Jul 9, 2016

jonasschnelli added the label Priority Medium on Jul 9, 2016

jonasschnelli added this to the milestone 0.13.0 on Jul 9, 2016

in qa/rpc-tests/wallet-hd.py:None in f70808596f outdated

  48 | @@ -45,6 +49,9 @@ def run_test (self):
  49 |          num_hd_adds = 300
  50 |          for _ in range(num_hd_adds):
  51 |              hd_add = self.nodes[1].getnewaddress()
  52 | +            hd_info = self.nodes[1].validateaddress(hd_add)
  53 | +            assert_equal(hd_info["hdkeypath"], "m/0'/0'/"+str(_+1)+"'")

MarcoFalke commented at 9:37 AM on July 10, 2016:

Nit: underscore is usually used for unused loop variables. Mind to change it to a single char?

MarcoFalke commented at 10:06 AM on July 11, 2016: member

utACK f708085

in src/wallet/wallet.cpp:None in f70808596f outdated

 125 | @@ -126,6 +126,8 @@ CPubKey CWallet::GenerateNewKey()
 126 |              // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
 127 |              // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
 128 |              externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
 129 | +            metadata.hdKeypath     = "m/0'/0'/"+std::to_string(hdChain.nExternalChainCounter)+"'";
 130 | +            metadata.hdMasterKeyID = hdChain.masterKeyID;

MarcoFalke commented at 10:08 AM on July 11, 2016:

I could imagine this will bloat wallet.dat a bit?

MarcoFalke commented at 7:33 PM on July 14, 2016:

5% to 10% increase in wallet.dat size, it seems.

jonasschnelli commented at 7:37 PM on July 14, 2016:

I guess its around ~30bytes per pubkey. I think this is acceptable.

laanwj commented at 10:52 AM on July 11, 2016: member

Concept ACK

in src/wallet/rpcwallet.cpp:None in f70808596f outdated

2268 | @@ -2269,6 +2269,7 @@ UniValue getwalletinfo(const UniValue& params, bool fHelp)
2269 |              "  \"keypoolsize\": xxxx,        (numeric) how many new keys are pre-generated\n"
2270 |              "  \"unlocked_until\": ttt,      (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
2271 |              "  \"paytxfee\": x.xxxx,         (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
2272 | +            "  \"masterkeyid\": \"<hash160>\", (string) the Hash160 of the hd master pubkey\n"

paveljanik commented at 7:07 PM on July 14, 2016:

hd -> HD

in src/wallet/rpcwallet.cpp:None in f70808596f outdated

2288 | @@ -2288,6 +2289,9 @@ UniValue getwalletinfo(const UniValue& params, bool fHelp)
2289 |      if (pwalletMain->IsCrypted())
2290 |          obj.push_back(Pair("unlocked_until", nWalletUnlockTime));
2291 |      obj.push_back(Pair("paytxfee",      ValueFromAmount(payTxFee.GetFeePerK())));
2292 | +    CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
2293 | +    if (!masterKeyID.IsNull())
2294 | +         obj.push_back(Pair("masterkeyid",masterKeyID.GetHex()));

paveljanik commented at 7:07 PM on July 14, 2016:

SPC after comma

paveljanik commented at 7:18 PM on July 14, 2016: contributor

Please use HD and BIPxx always in uppercase.

in src/wallet/walletdb.h:None in f70808596f outdated

 108 |  
 109 |      void SetNull()
 110 |      {
 111 |          nVersion = CKeyMetadata::CURRENT_VERSION;
 112 |          nCreateTime = 0;
 113 | +        hdKeypath.clear();

instagibbs commented at 8:36 PM on July 14, 2016:

any reason hdMasterKeyID isn't set null here

jonasschnelli commented at 8:35 AM on July 15, 2016:

Good catch. Thanks. Added a commit to address this: https://github.com/bitcoin/bitcoin/pull/8323/commits/68d7682b9f8c0f548e0a9ef03296320b8ae3960d

luke-jr commented at 12:38 AM on July 15, 2016: member

Am I correct that this does not conflict with #8132?

jonasschnelli commented at 7:35 AM on July 15, 2016: contributor

@luke-jr: I think this is incompatible with Knots implementation of "the key generation type" (similar to #8132). But, using Version 10 for the CKeyMetadata allows Knots to detect the HDness and migrate the data. I guess Core 0.13 wallets are incompatible with Knows 0.12. But Know 0.13 can be compatible with Core 0.13.

[Wallet] ensure CKeyMetadata.hdMasterKeyID will be cleared during SetNull() 68d7682b9f

[Wallet] comsetic non-code changes for the HD feature 7945088d41

jonasschnelli commented at 8:36 AM on July 15, 2016: contributor

Added two commits to address the nits.

laanwj commented at 5:58 AM on July 18, 2016: member

Looks good to me, utACK 7945088

laanwj merged this on Jul 18, 2016

laanwj closed this on Jul 18, 2016

laanwj referenced this in commit 238300b398 on Jul 18, 2016

lateminer referenced this in commit b741b11cad on Jan 2, 2018

lateminer referenced this in commit cd03a6780c on Jan 2, 2018

zkbot referenced this in commit 25c3f903c1 on Sep 19, 2018

MarcoFalke locked this on Sep 8, 2021