test: add unit test coverage for Python ECDSA implementation #27653

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202305-test-add_ecdsa_signing_unittest changing 1 files +21 −12
  1. theStack commented at 2:15 PM on May 14, 2023: contributor

    This PR adds missing unit test coverage for the Python ECDSA implementation, which should be useful for detecting potential problems early whenever changes in the test framework's Python implementation of secp256k1 are made (e.g. #26222). Note that right now we don't call ECPubKey.verify_ecdsa anywhere in our tests, so we wouldn't notice if it is broken at some point.

    To keep it simple, the already existing unit test for Schnorr signatures is extended to also check ECDSA signatures. For that purpose, the dictionary storing private-key/public-key entries use their legacy types ECKey/ECPubKey instead of bare byte-arrays, and for Schnorr signing/verification the necessary conversions (ECKey -> bare private key, ECPubKey -> x-only pubkey) is done later when needed. To avoid code duplication, a helper function random_bitflip for damaging signatures is introduced.

    The unit test can be run by either calling it for this single module: $ python3 -m unittest ./test/functional/test_framework/key.py or simply running $ ./test/functional/test_runner.py which calls all test framework module's unit tests at the start (see TEST_FRAMEWORK_MODULES list).

  2. DrahtBot commented at 2:15 PM on May 14, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK sipa, stratospher, achow101
    Concept ACK pinheadmz

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on May 14, 2023
  4. in test/functional/test_framework/key.py:306 in 490874fec5 outdated
     530 | -                keys[privkey] = pubkey
     531 | +        for privkey_bytes in byte_arrays:  # build array of key/pubkey pairs
     532 | +            privkey = ECKey()
     533 | +            privkey.set(privkey_bytes, compressed=True)
     534 | +            if privkey.is_valid:
     535 | +                keys[privkey] = privkey.get_pubkey()
    


    pinheadmz commented at 7:38 PM on May 15, 2023:

    IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that generate_privkey() is always valid but certain edge cases are not (0, SECP256K1_ORDER, and 2**256 - 1)

  5. pinheadmz commented at 7:53 PM on May 15, 2023: member

    concept ACK

    I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the key_tests.cpp unit test, assuring that both implementations handle Schnorr (at least those cases) identically.

  6. DrahtBot added the label Needs rebase on Jun 28, 2023
  7. achow101 requested review from sipa on Sep 20, 2023
  8. test: add unit test coverage for Python ECDSA implementation 96b3f2dbe4
  9. theStack force-pushed on Sep 20, 2023
  10. theStack commented at 4:25 PM on September 20, 2023: contributor

    Rebased on master.

  11. DrahtBot removed the label Needs rebase on Sep 20, 2023
  12. sipa commented at 2:53 PM on September 29, 2023: member

    utACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983

  13. stratospher commented at 4:04 PM on September 29, 2023: contributor

    tested ACK 96b3f2d.

  14. achow101 commented at 6:41 PM on September 29, 2023: member

    ACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983

  15. achow101 merged this on Sep 29, 2023
  16. achow101 closed this on Sep 29, 2023

  17. theStack deleted the branch on Sep 29, 2023
  18. Frank-GER referenced this in commit 31cdbf032c on Oct 5, 2023
  19. bitcoin locked this on Sep 28, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 21:13 UTC

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