Add HD keypath to CKeyMetadata, report metadata in validateaddress#8323
Add HD keypath to CKeyMetadata, report metadata in validateaddress#8323laanwj merged 6 commits intobitcoin:masterfrom
Conversation
qa/rpc-tests/wallet-hd.py
Outdated
| for _ in range(num_hd_adds): | ||
| hd_add = self.nodes[1].getnewaddress() | ||
| hd_info = self.nodes[1].validateaddress(hd_add) | ||
| assert_equal(hd_info["hdkeypath"], "m/0'/0'/"+str(_+1)+"'") |
There was a problem hiding this comment.
Nit: underscore is usually used for unused loop variables. Mind to change it to a single char?
|
utACK f708085 |
| // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649 | ||
| externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT); | ||
| metadata.hdKeypath = "m/0'/0'/"+std::to_string(hdChain.nExternalChainCounter)+"'"; | ||
| metadata.hdMasterKeyID = hdChain.masterKeyID; |
There was a problem hiding this comment.
I could imagine this will bloat wallet.dat a bit?
There was a problem hiding this comment.
5% to 10% increase in wallet.dat size, it seems.
There was a problem hiding this comment.
I guess its around ~30bytes per pubkey. I think this is acceptable.
|
Concept ACK |
src/wallet/rpcwallet.cpp
Outdated
| " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n" | ||
| " \"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" | ||
| " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" | ||
| " \"masterkeyid\": \"<hash160>\", (string) the Hash160 of the hd master pubkey\n" |
|
Please use HD and BIPxx always in uppercase. |
| { | ||
| nVersion = CKeyMetadata::CURRENT_VERSION; | ||
| nCreateTime = 0; | ||
| hdKeypath.clear(); |
There was a problem hiding this comment.
any reason hdMasterKeyID isn't set null here
There was a problem hiding this comment.
Good catch. Thanks.
Added a commit to address this: 68d7682
|
Am I correct that this does not conflict with #8132? |
|
@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. |
|
Added two commits to address the nits. |
|
Looks good to me, |
…ateaddress 7945088 [Wallet] comsetic non-code changes for the HD feature (Jonas Schnelli) 68d7682 [Wallet] ensure CKeyMetadata.hdMasterKeyID will be cleared during SetNull() (Jonas Schnelli) f708085 [QA] extend wallet-hd test to cover HD metadata (Jonas Schnelli) 986c223 [Wallet] print hd masterkeyid in getwalletinfo (Jonas Schnelli) b1c7b24 [Wallet] report optional HDKeypath/HDMasterKeyId in validateaddress (Jonas Schnelli) 5b95dd2 [Wallet] extend CKeyMetadata with HD keypath (Jonas Schnelli)
6 participants
I strongly recommend to merge this into 0.13 (current master or in 0.13 once we have split off).
Without this PRs
CKeyMetadataextension, 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).