test: add unit test coverage for Python ECDSA implementation #27653
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
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.
| if privkey.is_valid: | ||
| keys[privkey] = privkey.get_pubkey() |
There was a problem hiding this comment.
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)
490874f to
96b3f2d
Compare
|
Rebased on master. |
|
ACK 96b3f2d |
…plementation 96b3f2d test: add unit test coverage for Python ECDSA implementation (Sebastian Falbesoner) Pull request description: 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. bitcoin#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). ACKs for top commit: achow101: ACK 96b3f2d sipa: utACK 96b3f2d stratospher: tested ACK 96b3f2d. Tree-SHA512: b993f25b843fa047376addda4ce4b0f15750ffba926528b5cca4c5f99b9af456206f4e8af885d25a017dddddf382ddebf38765819b3d16a3f28810d03b010808
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_ecdsaanywhere 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/ECPubKeyinstead 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 functionrandom_bitflipfor 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.pyor simply running
$ ./test/functional/test_runner.pywhich calls all test framework module's unit tests at the start (see TEST_FRAMEWORK_MODULES list).