Skip to content

added test vector #5 for invalid extended keys - #921

Merged
luke-jr merged 2 commits into
bitcoin:masterfrom
fametrano:patch-4
Aug 30, 2021
Merged

added test vector #5 for invalid extended keys#921
luke-jr merged 2 commits into
bitcoin:masterfrom
fametrano:patch-4

Conversation

@fametrano

@fametrano fametrano commented May 16, 2020

Copy link
Copy Markdown
Contributor

The BIP32 specification lacks test vectors for invalid extended keys that should not be parsed as valid. Such test vectors are proposed here.

@fametrano fametrano changed the title added invalid extended keys vectors added test vector for invalid extended keys May 16, 2020
@fametrano fametrano changed the title added test vector for invalid extended keys added test vectors for invalid extended keys May 16, 2020
@junderw

junderw commented May 17, 2020

Copy link
Copy Markdown
Contributor

good point.

I might also add some derivation tests that test specifically for leading zeroes in the private key for hardened derivation.

So many bigInt libraries in many languages convert to byte arrays with varying lengths and people forget that the ser_key needs to be 32 bytes.

For example: btcsuite btcutil ExtendedKey will not derive the following correctly as per this Pull Request: btcsuite/btcutil#161

Root m:
xprv9s21ZrQH143K2PjtEr9foXN58DmBJu3nHmSnzXwvbqQiKqWZSsGPCNG1Je5qVeBF8fGGffBHAFu4jqXLkaHW4AbnByYDCfGKB4pxpyTTFRn
m/0H
xprv9v7bi4PiA35i9dhjDptXMMn46wLrFx37xorBaJhYzvjYUFC4fAFmPv1thGA9BinMyqdh6YnDAfasQdCBpYKUkW548fmeG1ZPkXLy5KLwkUY

Would be one such example.

@luke-jr luke-jr added the Proposed BIP modification PR by non-owner to update BIP content label Jun 1, 2020
@luke-jr

luke-jr commented Jun 1, 2020

Copy link
Copy Markdown
Member

@sipa

@fametrano

fametrano commented Jun 2, 2020

Copy link
Copy Markdown
Contributor Author

I might also add some derivation tests that test specifically for leading zeroes in the private key for hardened derivation.

that is already covered by the test vector 3 added in April 2017

@fametrano

fametrano commented Aug 19, 2020

Copy link
Copy Markdown
Contributor Author

If it helps, a test vector is available here, as part of the btclib test suite, namely tested here

@fametrano fametrano mentioned this pull request Aug 19, 2020
Merged
@klim0v

klim0v commented Oct 19, 2020

Copy link
Copy Markdown

#905

@fametrano fametrano changed the title added test vectors for invalid extended keys added test vector #4 for invalid extended keys Nov 15, 2020
@fametrano

fametrano commented Nov 15, 2020

Copy link
Copy Markdown
Contributor Author

#905

Unless I am missing something, a library not failing at test vector #3 would not fail at this new proposed case:
e.g. see btclib-org/btclib@0cab434

@fametrano

fametrano commented Nov 15, 2020

Copy link
Copy Markdown
Contributor Author

is there anything I could do to help this PR being accepted?

For a json file of the invalid keys see in the btclib library https://github.com/btclib-org/btclib/blob/master/btclib/tests/test_data/bip32_invalid_keys.json

@sipa

sipa commented Nov 16, 2020

Copy link
Copy Markdown
Member

The tests for "zero parent fingerprint with non-zero depth" are wrong. There is no reason why fingerprint 0 implies it's a root.

@fametrano
added invalid extended keys vectors
ee2e059 
The BIP32 specification lacks test vectors for invalid extended keys that should not be parsed as valid. Such test vectors are proposed here.
@fametrano

fametrano commented Nov 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The tests for "zero parent fingerprint with non-zero depth" are wrong. There is no reason why fingerprint 0 implies it's a root.

My bad!
I've amended the code and squashed all commits so far.

@fametrano

fametrano commented Aug 8, 2021

Copy link
Copy Markdown
Contributor Author

any chance for this to be accepted?

@maflcko

maflcko commented Aug 23, 2021

Copy link
Copy Markdown
Member

Needs rebase?

@luke-jr

luke-jr commented Aug 23, 2021

Copy link
Copy Markdown
Member

Rebasing isn't needed assuming the vector # can just be bumped to 5.

Just needs @sipa's approval

@fametrano fametrano changed the title added test vector #4 for invalid extended keys added test vector #5 for invalid extended keys Aug 25, 2021
@fametrano

fametrano commented Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

Rebased, solving the vector # conflict: if/when approved, this PR can be just squashed&merged

@sipa sipa mentioned this pull request Aug 30, 2021
Merged
@sipa

sipa commented Aug 30, 2021

Copy link
Copy Markdown
Member

ACK.

See implementation for Bitcoin Core here: bitcoin/bitcoin#22836

@luke-jr
luke-jr merged commit 3e68fe4 into bitcoin:master Aug 30, 2021
@benthecarman benthecarman mentioned this pull request Aug 30, 2021
Merged
@benthecarman

benthecarman commented Aug 30, 2021

Copy link
Copy Markdown
Contributor

ACK

this found a bug in bitcoin-s

bitcoin-s/bitcoin-s#3634

@fametrano
fametrano deleted the patch-4 branch August 31, 2021 23:09
@darosior darosior mentioned this pull request Sep 1, 2021
Merged
@darosior

darosior commented Sep 1, 2021

Copy link
Copy Markdown
Member

ACK

Implemented in python-bip32, which was too lax on parsing. darosior/python-bip32#17

fanquake added a commit to bitcoin-core/gui that referenced this pull request Sep 2, 2021
@fanquake
Merge bitcoin/bitcoin#22836: Stricter BIP32 decoding and test vector 5
01fa148 
56a42f1 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)

Pull request description:

  This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed bitcoin/bips#921 BIP32 test vector 5.

ACKs for top commit:
  darosior:
    utACK 56a42f1 -- Had to implement essentially the same fix in python-bip32.
  kristapsk:
    ACK 56a42f1. Checked that test vectors are the same as in BIP32 and that tests pass.

Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
@kristapsk kristapsk mentioned this pull request Nov 3, 2021
Closed
@kristapsk kristapsk mentioned this pull request Feb 17, 2022
Merged
kristapsk added a commit to JoinMarket-Org/joinmarket-clientserver that referenced this pull request Mar 2, 2022
@kristapsk
Merge #1186: Stricter BIP32 decoding and test vector 5
83918a9 
9ab2315 Stricter BIP32 decoding and test vector 5 (Kristaps Kaupe)

Pull request description:

  See bitcoin/bips#921 and bitcoin/bitcoin#22836.

Top commit has no ACKs.

Tree-SHA512: 1e0a44ca0b78f00e9b7779dbdbdbfd7ce46965e04758e2ce4d742ff6dabb6437287b48b470333db0586d3d97ab1907539a395721a3fb419a7c3711f7bf9feae0
@junderw junderw mentioned this pull request Nov 15, 2023
Open
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 27, 2023
@fanquake @PastaPastaPasta
Merge bitcoin#22836: Stricter BIP32 decoding and test vector 5
e0d893a 
56a42f1 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)

Pull request description:

  This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed bitcoin/bips#921 BIP32 test vector 5.

ACKs for top commit:
  darosior:
    utACK 56a42f1 -- Had to implement essentially the same fix in python-bip32.
  kristapsk:
    ACK 56a42f1. Checked that test vectors are the same as in BIP32 and that tests pass.

Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
Kino1994 pushed a commit to Kino1994/bitcoin-full-history that referenced this pull request Jun 28, 2026
@fanquake
Merge bitcoin/bitcoin#22836: Stricter BIP32 decoding and test vector 5
89a39f2 
fced140 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)

Pull request description:

  This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed bitcoin/bips#921 BIP32 test vector 5.

ACKs for top commit:
  darosior:
    utACK fced140 -- Had to implement essentially the same fix in python-bip32.
  kristapsk:
    ACK fced140. Checked that test vectors are the same as in BIP32 and that tests pass.

Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
BigcoinBGC pushed a commit to BigcoinBGC/bigcoin that referenced this pull request Jun 30, 2026
@fanquake
Merge bitcoin/bitcoin#22836: Stricter BIP32 decoding and test vector 5
a67ef32 
ff3e734 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)

Pull request description:

  This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed bitcoin/bips#921 BIP32 test vector 5.

ACKs for top commit:
  darosior:
    utACK ff3e734 -- Had to implement essentially the same fix in python-bip32.
  kristapsk:
    ACK ff3e734. Checked that test vectors are the same as in BIP32 and that tests pass.

Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
ab4cus pushed a commit to ab4cus/e4Coin-core that referenced this pull request Jul 8, 2026
@fanquake @PastaPastaPasta
Merge bitcoin/bitcoin#22836: Stricter BIP32 decoding and test vector 5
c85db70 
56a42f10f452f0ac0e3e333646a8effcbebf6b30 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)

Pull request description:

  This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed bitcoin/bips#921 BIP32 test vector 5.

ACKs for top commit:
  darosior:
    utACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30 -- Had to implement essentially the same fix in python-bip32.
  kristapsk:
    ACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30. Checked that test vectors are the same as in BIP32 and that tests pass.

Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sipa sipa

Labels

Proposed BIP modification PR by non-owner to update BIP content

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@fametrano @junderw @luke-jr @klim0v @sipa @maflcko @benthecarman @darosior