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

BIP32: Modified test vectors for hardened derivation with leading zeros #1030

Merged
merged 1 commit into from
Jun 12, 2021

Conversation

silencer-Tsai
Copy link
Contributor

As mentioned in bitpay/bitcore-lib#47, iancoleman/bip39#58 and btcsuite/btcutil#172, the private key with leading zeros(i.e. less than 32 bytes) should be padded to 32 bytes when we derive a hardened child private key from a private key.

Unfortunately, the original test vector 3 is not suitable for this case since both keys are 32 bytes.

In the new test vectors, the key m/0H is 31 bytes.

btcsuite/btcutil just fix the bug. See btcsuite/btcutil#182.

@schildbach
Copy link
Contributor

I think old test vectors should not be deleted. They are needed as regression tests.

FYI bitcoinj passes the new test vectors without changes.

dgpv added a commit to Simplexum/python-bitcointx that referenced this pull request Nov 4, 2020
The code in bitcointx correctly handles this, but just to be in-sync
with the text of the BIP, use the new test vectors submitted in
bitcoin/bips#1030 (when it is merged)
@silencer-Tsai
Copy link
Contributor Author

@schildbach
Agree. I have brought it back already.

dgpv added a commit to Simplexum/python-bitcointx that referenced this pull request Nov 4, 2020
The code in bitcointx correctly handles this, but just to be in-sync
with the text of the BIP, use the new test vectors submitted in
bitcoin/bips#1030 (when it is merged)
@afk11
Copy link
Contributor

afk11 commented Nov 4, 2020

Thanks for sharing the test fixture!

@junderw
Copy link
Contributor

junderw commented Nov 4, 2020

So existing Test 3 is for testing if the initial seed -> master generation step works with a 0x00 leading private key part.
Newly added Test 4 tests for the existing extended key -> child hardened key step for working with 0x00 leading private key part.

This makes sense since most implementations perform the HMACs for seed -> master and node -> child in separate places in the code, so it makes sense to test both.

LGTM

dgpv added a commit to Simplexum/python-bitcointx that referenced this pull request Dec 4, 2020
The code in bitcointx correctly handles this, but just to be in-sync
with the text of the BIP, use the new test vectors submitted in
bitcoin/bips#1030 (when it is merged)
@luke-jr luke-jr requested a review from sipa February 3, 2021 22:03
SomberNight added a commit to spesmilo/electrum that referenced this pull request Feb 18, 2021
@Giszmo
Copy link
Contributor

Giszmo commented May 20, 2021

What exactly is the hold-up here? LGTM!

kristapsk added a commit to JoinMarket-Org/joinmarket-clientserver that referenced this pull request May 25, 2021
… leading zeros

516e46a Additional BIP32 test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

Top commit has no ACKs.

Tree-SHA512: 7c0d6f988391297ba83fbebb88f890e88d7458a963e58497714f7385a9982a0c5898d8bbfc3852f51ba3b433d419449bb1e1ca95c2832780628123f66a12e1ad
@sipa
Copy link
Member

sipa commented May 28, 2021

ACK, verified that it actually exercises derivation from a private key with leading 0 byte.

@murchandamus
Copy link
Contributor

ACK b0521f0

maflcko pushed a commit to bitcoin/bitcoin that referenced this pull request May 31, 2021
…ion with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
@maflcko
Copy link
Member

maflcko commented May 31, 2021

@luke-jr

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 1, 2021
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
Copy link

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK b0521f0

t-bast added a commit to ACINQ/bitcoin-lib that referenced this pull request Jun 9, 2021
A new test vectors was introduced in bitcoin/bips#1030
because some wallets didn't handle leading zeros correctly in hardened
derivation.

We already handled that case correctly, but didn't have a test vector for it.
t-bast added a commit to ACINQ/bitcoin-kmp that referenced this pull request Jun 9, 2021
A new test vector was introduced in bitcoin/bips#1030
because some wallets didn't handle leading zeros correctly in
hardened derivation.

We already handled that case correctly, but didn't have a test vector for it.
darosior added a commit to darosior/python-bip32 that referenced this pull request Jun 9, 2021
We were doing The Right Thing! See bitcoin/bips#1030

Signed-off-by: Antoine Poinsot <[email protected]>
@maflcko
Copy link
Member

maflcko commented Jun 11, 2021

@kallewoof

@darosior
Copy link
Member

ACK b0521f0 - tested in darosior/python-bip32#16

@kallewoof kallewoof merged commit d2766d0 into bitcoin:master Jun 12, 2021
t-bast added a commit to ACINQ/bitcoin-lib that referenced this pull request Jun 14, 2021
A new test vectors was introduced in bitcoin/bips#1030
because some wallets didn't handle leading zeros correctly in hardened
derivation.

We already handled that case correctly, but didn't have a test vector for it.
t-bast added a commit to ACINQ/bitcoin-kmp that referenced this pull request Jun 14, 2021
A new test vector was introduced in bitcoin/bips#1030
because some wallets didn't handle leading zeros correctly in
hardened derivation.

We already handled that case correctly, but didn't have a test vector for it.
dgpv added a commit to Simplexum/python-bitcointx that referenced this pull request Jul 15, 2021
The code in bitcointx correctly handles this, but just to be in-sync
with the text of the BIP, use the new test vectors submitted in
bitcoin/bips#1030 (when it is merged)
knst pushed a commit to knst/dash that referenced this pull request Nov 28, 2023
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
knst pushed a commit to knst/dash that referenced this pull request Nov 30, 2023
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
UdjinM6 pushed a commit to knst/dash that referenced this pull request Nov 30, 2023
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
PastaPastaPasta pushed a commit to knst/dash that referenced this pull request Dec 4, 2023
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.