added test vector #5 for invalid extended keys #921

pull fametrano wants to merge 2 commits into bitcoin:master from fametrano:patch-4 changing 1 files +20 −0
  1. fametrano commented at 12:58 AM on May 16, 2020: contributor

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

  2. fametrano renamed this:
    added invalid extended keys vectors
    added test vector for invalid extended keys
    on May 16, 2020
  3. fametrano renamed this:
    added test vector for invalid extended keys
    added test vectors for invalid extended keys
    on May 16, 2020
  4. junderw commented at 9:57 AM on May 17, 2020: 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: https://github.com/btcsuite/btcutil/pull/161

    Root m:
xprv9s21ZrQH143K2PjtEr9foXN58DmBJu3nHmSnzXwvbqQiKqWZSsGPCNG1Je5qVeBF8fGGffBHAFu4jqXLkaHW4AbnByYDCfGKB4pxpyTTFRn
m/0H
xprv9v7bi4PiA35i9dhjDptXMMn46wLrFx37xorBaJhYzvjYUFC4fAFmPv1thGA9BinMyqdh6YnDAfasQdCBpYKUkW548fmeG1ZPkXLy5KLwkUY

    Would be one such example.

  5. luke-jr added the label Proposed BIP modification on Jun 1, 2020
  6. luke-jr assigned sipa on Jun 1, 2020
  7. luke-jr commented at 7:27 PM on June 1, 2020: member

    @sipa

  8. fametrano commented at 11:00 AM on June 2, 2020: contributor

    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

  9. fametrano commented at 2:34 PM on August 19, 2020: contributor

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

  10. klim0v commented at 3:57 PM on October 19, 2020: none

    https://github.com/bitcoin/bips/pull/905

  11. fametrano renamed this:
    added test vectors for invalid extended keys
    added test vector #4 for invalid extended keys
    on Nov 15, 2020
  12. fametrano commented at 9:41 AM on November 15, 2020: contributor

    #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

  13. fametrano commented at 9:59 AM on November 15, 2020: contributor

    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

  14. sipa commented at 7:57 PM on November 16, 2020: 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.

  15. added invalid extended keys vectors
    The BIP32 specification lacks test vectors for invalid extended keys that should not be parsed as valid. Such test vectors are proposed here.
    ee2e059820
  16. fametrano force-pushed on Nov 17, 2020
  17. fametrano commented at 6:19 AM on November 17, 2020: contributor

    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.

  18. fametrano commented at 4:07 PM on August 8, 2021: contributor

    any chance for this to be accepted?

  19. maflcko commented at 11:12 AM on August 23, 2021: member

    Needs rebase?

  20. luke-jr commented at 4:08 PM on August 23, 2021: member

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

    Just needs @sipa's approval

  21. Merge branch 'master' into patch-4 f9a81b7791
  22. fametrano renamed this:
    added test vector #4 for invalid extended keys
    added test vector #5 for invalid extended keys
    on Aug 25, 2021
  23. fametrano commented at 8:56 PM on August 25, 2021: contributor

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

  24. sipa commented at 4:36 PM on August 30, 2021: member

    ACK.

    See implementation for Bitcoin Core here: https://github.com/bitcoin/bitcoin/pull/22836

  25. luke-jr merged this on Aug 30, 2021
  26. luke-jr closed this on Aug 30, 2021
  27. benthecarman commented at 7:58 PM on August 30, 2021: contributor

    ACK

    this found a bug in bitcoin-s

    https://github.com/bitcoin-s/bitcoin-s/pull/3634

  28. fametrano deleted the branch on Aug 31, 2021
  29. darosior commented at 9:08 AM on September 1, 2021: member

    ACK

    Implemented in python-bip32, which was too lax on parsing. https://github.com/darosior/python-bip32/pull/17

  30. fanquake referenced this in commit 01fa1481f9 on Sep 2, 2021
  31. sidhujag referenced this in commit 8fdd2cb327 on Sep 2, 2021
  32. kristapsk referenced this in commit 83918a971d on Mar 2, 2022
  33. PastaPastaPasta referenced this in commit a8cf30fb98 on Dec 25, 2023
  34. PastaPastaPasta referenced this in commit 00796a3bfc on Dec 25, 2023
  35. PastaPastaPasta referenced this in commit e0d893a11a on Dec 27, 2023
Contributors
fametranojunderwluke-jrklim0vsipamaflckobenthecarmandarosior
Labels
Proposed BIP modification
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:50 UTC