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

Already on GitHub? Sign in to your account

Fix Images For Normal HD Key Derivation; Mention Ancestor Key Risk #408

Merged
merged 1 commit into from May 19, 2014

Conversation

Projects
None yet
4 participants
Contributor

harding commented May 17, 2014

While reading an excellent article by @vbuterin about HD wallet risks, I realized I used an incorrect key derivation formula that failed to include parent public keys in the hash. This patch for #393 fixes that error in both text and images.

I also added a short paragraph describing the risk explained in @vbuterin's article and updated the corresponding image to illustrate that risk. I would've preferred to add this new content separately after #393 gets merged, but I saw no point in updating the same image twice---once to correct my error and once to illustrate the additional risk.

Unrelated change: I removed an includes line from devref referring to a previously-deleted file; an upgraded version of Jekyll was dying because of this line.

Fix Formula For Normal HD Key Derivation; Mention Ancestor Key Risk
_includes/guide_wallets.md:

* Fix formula given for normal child key derivation to state that public
  keys must also be provided to the HMAC hash function. This required
  updating both text and images.

* Add one-paragraph warning about ancestor key compromise when the
  ancestor extended public key is compromised along with a descended
  private key.  Update img/dev/en-hd-private-parent-to-private-child.*
  to help illustrate this warning.

en/developer-reference.md:

* Remove %include% of previously-removed file which caused new versions
  of Jekyll to die.
Contributor

saivann commented May 19, 2014

@harding Thanks for your work on this! I couldn't find anything to add. I think this is better than what we have now, so:

In the absence of critical feedback, this pull request will be merged on May 19th.

@harding harding referenced this pull request May 19, 2014

Merged

Add Developer Guide To Bitcoin.org #393

6 of 6 tasks complete

saivann added a commit that referenced this pull request May 19, 2014

Merge pull request #408 from harding/hd-ancestor-compromise
Fix Images For Normal HD Key Derivation; Mention Ancestor Key Risk

@saivann saivann merged commit 5c102c5 into bitcoin-dot-org:devel-docs May 19, 2014

Contributor

gmaxwell commented May 19, 2014

@harding This is why the hardened derivation exists, as it closes that issue. Perhaps that could be made more clear in the text?

Closes it at the expense of having a way to create a complete watch-only
wallet from the master public key. It's important to point out the
tradeoffs.

On 14-05-19 07:11 PM, Gregory Maxwell wrote:

@harding https://github.com/harding This is why the hardened
derivation exists, as it closes that issue. Perhaps that could be made
more clear in the text?


Reply to this email directly or view it on GitHub
bitcoin#408 (comment).

Contributor

gmaxwell commented May 20, 2014

@harding Also, can you suggest improvements to the BIP32 text that would have made it more obvious to you there?

Contributor

harding commented May 20, 2014

@gmaxwell I like making things clearer. :-) I think we could make it
pretty darn explicit by giving the Hardened Keys subsection a nice
strong first sentence, such as:

Hardened extended keys fix a potential problem with regular
extended keys.

That'll require some rewording of the rest of the opening paragraph,
so I'll work on it tomorrow and be sure to @ mention you in the
resulting pull request.

Re: BIP32, I know the exact list item from which I drew an incorrect
inference. I'll re-read it tomorrow (to make sure I didn't read it
wrong before) and submit a pull request you and @sipa can review.

Thanks!

David A. Harding

Contributor

harding commented May 20, 2014

On Mon, May 19, 2014 at 05:04:09PM -0700, vbuterin wrote:

Closes it at the expense of having a way to create a complete watch-only
wallet from the master public key. It's important to point out the
tradeoffs.

@vbuterin Hi! Thanks for your article! More importantly, thanks for your
nice code in pybitcointools. It was a great aid to me in understanding
this risk.

When I make the revision @gmaxwell suggested, I'll be sure to add a
quick note that the master extended private key is hardened by default
in a BIP32-compliant wallet.

Thanks!

David A. Harding

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