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.

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

added test vector #5 for invalid extended keys #921

Merged
merged 2 commits into from
Aug 30, 2021
Merged

added test vector #5 for invalid extended keys #921

merged 2 commits into from
Aug 30, 2021

Conversation

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

junderw commented May 17, 2020

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
Copy link
Member

luke-jr commented Jun 1, 2020

@sipa

@fametrano
Copy link
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
Copy link
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
Copy link

klim0v commented Oct 19, 2020

#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
Copy link
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
Copy link
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
Copy link
Member

sipa commented Nov 16, 2020

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

fametrano commented Nov 17, 2020 •
edited
Loading

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

any chance for this to be accepted?

@maflcko
Copy link
Member

maflcko commented Aug 23, 2021

Needs rebase?

@luke-jr
Copy link
Member

luke-jr commented Aug 23, 2021

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
Copy link
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
Copy link
Member

sipa commented Aug 30, 2021

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
Copy link
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
Copy link
Member

darosior commented Sep 1, 2021

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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 2, 2021
@fanquake
Merge bitcoin#22836: Stricter BIP32 decoding and test vector 5
8fdd2cb 
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 25, 2023
@fanquake @PastaPastaPasta
Merge bitcoin#22836: Stricter BIP32 decoding and test vector 5
a8cf30f 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 25, 2023
@fanquake @PastaPastaPasta
Merge bitcoin#22836: Stricter BIP32 decoding and test vector 5
00796a3 
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
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
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
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