Skip to content
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

BIP85: revert XPRV breaking changes for application 32' #1673

Closed

Conversation

scgbckbone
Copy link
Contributor

  • revert (only) breaking changes wrt XPRV (application 32')
  • no need to mention 0x00 private key prepend as it is defined in BIP-032

@nvk
Copy link

nvk commented Oct 4, 2024

ACK

@Rob1Ham
Copy link

Rob1Ham commented Oct 4, 2024

ACK - we should never break user space, if someone has an alternative implementation it can go in a different bip

@@ -438,8 +439,7 @@ BIP32, BIP39

* 1.0 (2020-07)
* 2.0.0 (2024-09-22)
* Swap chain code and private key bytes in application 32' for consistentcy with BIP-32 (major change)
* Correct derived entropy for application 128169' test vector (major change)
* Correct derived entropy for application 707764' test vector (bugfix)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akarve I assume this was bug in application number ? you mentioned 128169' in previous PR BUT wrong entropy was in 707764'. I see no breaking changes in hex app

Copy link
Contributor

Choose a reason for hiding this comment

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

The only vector that needs correction as of my simplified PR #1679 is Hex (128169').

If somehow an extant implementation got the old test vector to work this correction would be a breaking change but I agree that if the correct BIP 85 algo is used then this is non-breaking :)

@luisschwab
Copy link

ACK

@jonatack jonatack changed the title rever XPRV breaking changes for application 32' BIP85: revert XPRV breaking changes for application 32' Oct 4, 2024
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Approach ACK

Taking 64 bytes of the HMAC digest, the first 32 bytes are the chain code,
and second 32 bytes are the private key for BIP32 XPRV value.

WARNING: This is not consistent with BIP-032
Copy link
Member

Choose a reason for hiding this comment

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

a3ff1ea nit, may as well use the same convention as the rest of this BIP, not a blocker

Suggested change
WARNING: This is not consistent with BIP-032
WARNING: This is not consistent with BIP32

@@ -258,7 +259,7 @@ INPUT:

OUTPUT
* DERIVED ENTROPY=ead0b33988a616cf6a497f1c169d9e92562604e38305ccd3fc96f2252c177682
* DERIVED XPRV=xprv9s21ZrQH143K4Px85utdpu6DFvY2NpHkJajPoupAznfiacH2MC9LasyW4uvqKXNxLWcjqGTbHKAhoZoMAbmRe5g9tAPA7cUUX4UVA1vFKFm
* DERIVED XPRV=xprv9s21ZrQH143K2srSbCSg4m4kLvPMzcWydgmKEnMmoZUurYuBuYG46c6P71UGXMzmriLzCCBvKQWBUv3vPB3m1SATMhp3uEjXHJ42jFg7myX
Copy link
Member

Choose a reason for hiding this comment

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

private key<ref name="curve-order" />. Prepend an empty byte (<code>0x00</code>)
per BIP32 on master key serialization. Use the last (rightmost) 32 bytes as the chain code.
Taking 64 bytes of the HMAC digest, the first 32 bytes are the chain code,
and second 32 bytes are the private key for BIP32 XPRV value.
Copy link
Member

Choose a reason for hiding this comment

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

In a3ff1ea, is there a reason not to keep the previous footnote at https://github.com/bitcoin/bips/pull/1600/files#diff-eae7a61b6d2a0b6091c1ce04618f216cbb871d987b985ba712de89e04014654cL379?

Suggested change
and second 32 bytes are the private key for BIP32 XPRV value.
and second 32 bytes[1] are the private key for BIP32 XPRV value.

@jonatack
Copy link
Member

jonatack commented Oct 4, 2024

The reference implementations section would also need to be reverted.

@nvk
Copy link

nvk commented Oct 4, 2024

Approach ACK

Thank you 🙏

@jonatack
Copy link
Member

jonatack commented Oct 4, 2024

The reference implementations section would also need to be reverted.

I've done a full revert in #1674.

From #1600 (comment):

Although the BIP is still in Draft status, I think it should have been marked as proposed or final a long time ago as it does appear to be deployed by a few projects.

@scgbckbone would you like to update this pull to do that instead?

@scgbckbone
Copy link
Contributor Author

closing this - as full revert already merged #1674

thanks @jonatack

@scgbckbone scgbckbone closed this Oct 5, 2024
@scgbckbone
Copy link
Contributor Author

The reference implementations section would also need to be reverted.

I've done a full revert in #1674.

From #1600 (comment):

Although the BIP is still in Draft status, I think it should have been marked as proposed or final a long time ago as it does appear to be deployed by a few projects.

@scgbckbone would you like to update this pull to do that instead?

created new PR instead #1676

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants