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 0: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

    0Root m:
    1xprv9s21ZrQH143K2PjtEr9foXN58DmBJu3nHmSnzXwvbqQiKqWZSsGPCNG1Je5qVeBF8fGGffBHAFu4jqXLkaHW4AbnByYDCfGKB4pxpyTTFRn
    2m/0H
    3xprv9v7bi4PiA35i9dhjDptXMMn46wLrFx37xorBaJhYzvjYUFC4fAFmPv1thGA9BinMyqdh6YnDAfasQdCBpYKUkW548fmeG1ZPkXLy5KLwkUY
    

    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
  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. fametrano cross-referenced this on Aug 19, 2020 from issue Derive child keys from an ExtendedKey by achow101
  11. klim0v commented at 3:57 pm on October 19, 2020: none
  12. fametrano renamed this:
    added test vectors for invalid extended keys
    added test vector #4 for invalid extended keys
    on Nov 15, 2020
  13. 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

  14. 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

  15. 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.
  16. 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
  17. fametrano force-pushed on Nov 17, 2020
  18. 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.

  19. fametrano commented at 4:07 pm on August 8, 2021: contributor
    any chance for this to be accepted?
  20. MarcoFalke commented at 11:12 am on August 23, 2021: member
    Needs rebase?
  21. 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

  22. Merge branch 'master' into patch-4 f9a81b7791
  23. fametrano renamed this:
    added test vector #4 for invalid extended keys
    added test vector #5 for invalid extended keys
    on Aug 25, 2021
  24. 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
  25. sipa cross-referenced this on Aug 30, 2021 from issue Stricter BIP32 decoding and test vector 5 by sipa
  26. 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

  27. luke-jr merged this on Aug 30, 2021
  28. luke-jr closed this on Aug 30, 2021

  29. benthecarman cross-referenced this on Aug 30, 2021 from issue Add new invalid BIP 32 test vectors by benthecarman
  30. 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

  31. fametrano deleted the branch on Aug 31, 2021
  32. darosior cross-referenced this on Sep 1, 2021 from issue More deserialization sanity checks, BIP32 test vector 5 by darosior
  33. 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

  34. fanquake referenced this in commit 01fa1481f9 on Sep 2, 2021
  35. sidhujag referenced this in commit 8fdd2cb327 on Sep 2, 2021
  36. kristapsk cross-referenced this on Nov 3, 2021 from issue BIP32 test vector 5 should be added to tests and checked by kristapsk
  37. kristapsk cross-referenced this on Feb 17, 2022 from issue Stricter BIP32 decoding and test vector 5 by kristapsk
  38. kristapsk referenced this in commit 83918a971d on Mar 2, 2022

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: 2024-10-30 03:10 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me