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

pull jonasschnelli wants to merge 6 commits into bitcoin:master from jonasschnelli:2016/07/hd_013 changing 6 files +43 −7

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
jonasschnelli cross-referenced this on Jul 9, 2016 from issue [Wallet] add HD keypath to CKeyMetadata, report over validateaddress by jonasschnelli

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
jonasschnelli cross-referenced this on Jul 11, 2016 from issue [Wallet] keep HD seed during salvagewallet by jonasschnelli
MarcoFalke cross-referenced this on Jul 14, 2016 from issue [qa] wallet*.py: Check for salvagewallet regressions by MarcoFalke

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
str4d cross-referenced this on Sep 12, 2018 from issue Extend RPC to be able to import and export Sapling keys by str4d
Eirik0 cross-referenced this on Sep 12, 2018 from issue Modify validateaddress and z_validateaddress for zip32 by Eirik0
Eirik0 cross-referenced this on Sep 12, 2018 from issue Add Sapling support to z_importwallet and z_exportwallet by Eirik0
zkbot referenced this in commit 25c3f903c1 on Sep 19, 2018
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.13.0

Linked (view graph)

#8132 wallet: Add key generation type #8205 [Wallet] add HD keypath to CKeyMetadata, report over validateaddress #8319 [qa] wallet*.py: Check for salvagewallet regressions #8324 [Wallet] keep HD seed during salvagewallet