New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Jul 18, 2016

Conversation

Projects
None yet
6 participants
@jonasschnelli
Member

jonasschnelli commented Jul 9, 2016

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

jonasschnelli added some commits Jul 9, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Jul 11, 2016

utACK f708085

@@ -126,6 +126,8 @@ CPubKey CWallet::GenerateNewKey()
// childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
// 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;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 11, 2016

Member

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

@MarcoFalke

MarcoFalke Jul 11, 2016

Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 14, 2016

Member

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

@MarcoFalke

MarcoFalke Jul 14, 2016

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 14, 2016

Member

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

@jonasschnelli

jonasschnelli Jul 14, 2016

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 11, 2016

Member

Concept ACK

Member

laanwj commented Jul 11, 2016

Concept ACK

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 14, 2016

Contributor

Please use HD and BIPxx always in uppercase.

Contributor

paveljanik commented Jul 14, 2016

Please use HD and BIPxx always in uppercase.

}
void SetNull()
{
nVersion = CKeyMetadata::CURRENT_VERSION;
nCreateTime = 0;
hdKeypath.clear();

This comment has been minimized.

@instagibbs

instagibbs Jul 14, 2016

Member

any reason hdMasterKeyID isn't set null here

@instagibbs

instagibbs Jul 14, 2016

Member

any reason hdMasterKeyID isn't set null here

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2016

Member

Good catch. Thanks.
Added a commit to address this: 68d7682

@jonasschnelli

jonasschnelli Jul 15, 2016

Member

Good catch. Thanks.
Added a commit to address this: 68d7682

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 15, 2016

Member

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

Member

luke-jr commented Jul 15, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 15, 2016

Member

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

Member

jonasschnelli commented Jul 15, 2016

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

jonasschnelli added some commits Jul 15, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 15, 2016

Member

Added two commits to address the nits.

Member

jonasschnelli commented Jul 15, 2016

Added two commits to address the nits.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 18, 2016

Member

Looks good to me,
utACK 7945088

Member

laanwj commented Jul 18, 2016

Looks good to me,
utACK 7945088

@laanwj laanwj merged commit 7945088 into bitcoin:master Jul 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 18, 2016

Merge #8323: Add HD keypath to CKeyMetadata, report metadata in valid…
…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)

lateminer added a commit to lateminer/bitcoin that referenced this pull request Jan 2, 2018

lateminer added a commit to lateminer/bitcoin that referenced this pull request Jan 2, 2018

@str4d str4d referenced this pull request Sep 12, 2018

Closed

Extend RPC to be able to import and export Sapling keys #3218

4 of 4 tasks complete

zkbot added a commit to zcash/zcash that referenced this pull request Sep 19, 2018

Auto merge of #3491 - Eirik0:3218-sapling-import-export-wallet, r=str4d
Add Sapling support to z_importwallet and z_exportwallet

Includes code adapted from upstream PR bitcoin/bitcoin#8323

Closes #3218.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment