added test vector #5 for invalid extended keys #921

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

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

    https://github.com/bitcoin/bips/pull/905
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

Contributors
fametrano junderw luke-jr klim0v sipa MarcoFalke benthecarman darosior

Labels
Proposed BIP modification