Skip to content

[Wallet] Simple HD/BIP32 support #7273

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.

Sign up for GitHub

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

Jump to bottom
Closed
jonasschnelli wants to merge 3 commits into from
Closed

[Wallet] Simple HD/BIP32 support #7273

jonasschnelli wants to merge 3 commits into from

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Jan 2, 2016

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 jonasschnelli added this to the 0.13.0 milestone Jan 2, 2016
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Jan 2, 2016

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

maflcko
maflcko reviewed Jan 2, 2016
View reviewed changes
qa/rpc-tests/hdwallet.py
Copy link
Member

@maflcko maflcko Jan 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is there a compelling reason to do this?

Copy link
Contributor Author

@jonasschnelli jonasschnelli Jan 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@ali8889 ali8889 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riky

Copy link

@ali8889 ali8889 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riky

@jgarzik
Copy link
Contributor

jgarzik commented Jan 2, 2016

concept ACK

luke-jr
luke-jr reviewed Jan 15, 2016
View reviewed changes
src/wallet/walletdb.h Outdated
Copy link
Member

@luke-jr luke-jr Jan 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? Achive -> Active?

@luke-jr
Copy link
Member

luke-jr commented Jan 15, 2016

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

luke-jr
luke-jr reviewed Jan 15, 2016
View reviewed changes
src/wallet/wallet.cpp
Copy link
Member

@luke-jr luke-jr Jan 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return value is ignored.

Copy link
Contributor Author

@jonasschnelli jonasschnelli Feb 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

Copy link

@ali8889 ali8889 Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oky

@pointbiz
Copy link

pointbiz commented Jan 22, 2016

Concept ACK

instagibbs
instagibbs reviewed Feb 1, 2016
View reviewed changes
src/init.cpp Outdated
Copy link
Member

@instagibbs instagibbs Feb 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jonasschnelli jonasschnelli Feb 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jonasschnelli commented Feb 22, 2016

Rebased and fixed reported nits.

instagibbs
instagibbs reviewed Mar 11, 2016
View reviewed changes
src/wallet/walletdb.cpp Outdated
return false;

Erase(std::make_pair(std::string("hdmasterseed"), hash));
Erase(std::make_pair(std::string("hdmasterseed"), hash));
Copy link
Member

@instagibbs instagibbs Mar 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double deletion?

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Mar 14, 2016

Rebased and fixed some nits.

@instagibbs
Copy link
Member

instagibbs commented Mar 14, 2016

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 jonasschnelli mentioned this pull request May 9, 2016
Closed
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 10, 2016

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 jonasschnelli mentioned this pull request May 11, 2016
Merged
@maflcko
Copy link
Member

maflcko commented May 15, 2016

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

@jonasschnelli jonasschnelli deleted the 2016/01/hdsimple branch May 16, 2016 13:44
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 16, 2016

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 jonasschnelli removed this from the 0.13.0 milestone May 16, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ali8889 ali8889 ali8889 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jonasschnelli @jgarzik @luke-jr @pointbiz @instagibbs @maflcko @ali8889