This PR adds a test using the Wycheproof vectors as outlined in #1106. We add all 463 ECDSA test vectors. These vectors cover:
The vectors are pulled from the Wycheproof project using a python script to emit C code.
All the new ECDSA Wycheproof vectors pass.
IMO it is a mistake to have the unit test consume test vectors as C code, because we miss out on 50% of the real value add - universally portable test vectors.
This is such a fundamental library that so many DLT project rely on, many of which through wrappers in higher abstraction languages, e.g. my own Swift Wrapper K1. Thus it will be an enormous value add for all these wrappers and other secp256k1 libraries to be able to copy over a JSON file in full.
Thus I think it is a mistake to not focus on the portability benefits of putting test vectors in a file, most preferably JSON.
Second best option is putting the test vector in a file, but on another format than JSON, eg raw binary or CBOR, because we still get the portability advantage, but less easy accessible for other language.
What so you all think?
I was more thinking along the lines of having the conversion (wycheproof json -> C) script be part of the repository, together with the source json and converted C code (as a separate file that gets #included). There could be a Makefile target to regenerate the C code whenever the JSON is updated. Alternatively, it could be part of the normal build process (not having the C code checked in), but then the build stage depends on having Python.
Yes that would also work, but if so, I’d prefer if we’d use one c file per unit test vector, and if we would refer to which JSON file in the repo was used to generate the C code, preferably in the top of the file.
What so you think about that?
I don’t have any opinion on one file vs many, but regarding high-level approach, my vote is
Thank you all for the suggestions. We’ve make changes to:
.inc file that gets #includedSince we check in the converted C code, the Makefile target doesn’t get triggered on every build (which is good since we don’t want to introduce a dependency on Python in the build stage).
Check in CI (not in the normal build process!) that the checked-in .c files can be regenerated and are identical to the checked-in ones.
What do you think it’s best for this? Would a line in ci/cirrus.sh work?
What do you think it’s best for this? Would a line in ci/cirrus.sh work?
That sounds great to me. Though bear in mind that I’m not a maintainer of this library (anymore) and I’m not the one to make such calls.
What do you think it’s best for this? Would a line in
ci/cirrus.shwork?
We already have that line because we check the same with the “precomputed” C files, see: https://github.com/bitcoin-core/secp256k1/blob/9c8c4f443c0027c3ed933a3883497131071b1c7e/ci/cirrus.sh#L109-L118
We could add make targets testvectors and clean-testvectors (or with a different name) similar to the existing ones for “precomputed” (see the Makefile) that create the C files from JSON. If we then call make clean-testvectors and make testvectors in the CI script, the already existing git diff --exit-code will catch any need to regenerate.
Thank you everyone for the great feedback.
We could add make targets testvectors and clean-testvectors (or with a different name) similar to the existing ones for “precomputed” (see the Makefile) that create the C files from JSON. If we then call make clean-testvectors and make testvectors in the CI script, the already existing git diff –exit-code will catch any need to regenerate.
Good suggestion! This is implemented now: we have new Makefile targets testvectors and clean-testvectors. We call them from ci/cirrus.sh. CI will fail if the checked .inc files need to be regenerated.
0@@ -0,0 +1,69 @@
1+import json
2+import hashlib
3+import urllib.request
4+import sys
5+
6+# These vectors come from Project Wycheproof (https://github.com/google/wycheproof)
7+# commit b063b4aedae951c69df014cd25fa6d69ae9e8cb9
0# Test vector origin - Project Wycheproof
1# Link: https://github.com/google/wycheproof/blob/b063b4aedae951c69df014cd25fa6d69ae9e8cb9/testvectors_v1/ecdsa_secp256k1_sha256_bitcoin_test.json
nit: Can we name that .h instead of .inc? Technically, we don’t use it exactly like a header because it’s included within a function, but it has all characteristics of a header, and .h makes it clear that it contains C code (also for editors, etc.)
And the subdir should be named wycheproof (see below for reasoning).
257+src/vectors/ecdsa_secp256k1_sha256_bitcoin_test.inc: src/vectors/ecdsa_secp256k1_sha256_bitcoin_test.json
258+ python3 src/vectors/tests_wycheproof_generate.py $< > $@
259+
260+testvectors: $(TESTVECTORS)
261+
262+clean-testvectors:
maintainer-clean-local
53+ unsigned char msg[SECP256K1_TEST_ECDSA_WYCHEPROOF_MAX_MSG];
54+ size_t msglen;
55+ unsigned char sig[SECP256K1_TEST_ECDSA_WYCHEPROOF_MAX_SIG];
56+ size_t siglen;
57+ int expected_verify;
58+} wycheproof_testcase_t;
0} wycheproof_ecdsa_testvector;
(_t suffix is reserved by POSIX)
58+} wycheproof_testcase_t;
59+"""
60+
61+
62+print("/* Note: this file was autogenerated using tests_wycheproof_generate.py. DO NOT EDIT. */")
63+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_NUMBER_TESTS ({})".format(num_cases))
0print("#define SECP256K1_ECDSA_WYCHEPROOF_NUMBER_TESTVECTORS ({})".format(num_cases))
And you could drop the TEST_ part, also below. We have this nowhere else, I think.
59+"""
60+
61+
62+print("/* Note: this file was autogenerated using tests_wycheproof_generate.py. DO NOT EDIT. */")
63+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_NUMBER_TESTS ({})".format(num_cases))
64+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_MAX_MSG ({})".format(max_msg_size))
0print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_MSG_SIZE ({})".format(max_msg_size))
60+
61+
62+print("/* Note: this file was autogenerated using tests_wycheproof_generate.py. DO NOT EDIT. */")
63+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_NUMBER_TESTS ({})".format(num_cases))
64+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_MAX_MSG ({})".format(max_msg_size))
65+print("#define SECP256K1_TEST_ECDSA_WYCHEPROOF_MAX_SIG ({})".format(max_sig_size))
0print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_SIG_SIZE ({})".format(max_sig_size))
Awesome. I have only looked at the diff so far, and I just have a bunch of minor comments, mostly related to consistent naming.
One major issue is licensing: It’s not entirely clear if the test vectors are copyrightable, so let’s be conservative and assume they are. Then Wycheproof’s Apache license applies, and we have to adhere to it, see https://www.apache.org/licenses/LICENSE-2.0 Section 4. This means we should probably create a subdir src/wycheproof with
.json file.h fileCOPYING file that says that the .json files in this directory have been taken from Google’s project Wycheproof with commit id (you can then remove the comment in your python file), that the C files are generated from the .json files using the script. And the file should include the Apache 2.0 license text (i.e., the contents of https://github.com/google/wycheproof/blob/master/LICENSE )The script can then go to a new directory tools in the root of the repo (as suggested in other unmerged PRs too): #1135 (comment)
Thank you for the detailed review @real-or-random. I have addressed all nits. I also changed folder names as requested:
.h and .json to src/wycheprooftools/I put the license file in src/wycheproof/WYCHEPROOF_COPYING. Like this, the name does not clash with the top level COPYING file (which is a different license). This is the same approach as with examples/EXAMPLES_COPYING.
13+def to_c_array(x):
14+ if x == "": return "{0}"
15+ s = ',0x'.join(a+b for a,b in zip(x[::2], x[1::2]))
16+ return "{ 0x" + s + " }"
17+
18+num_cases, max_msg_size, max_sig_size = 0, 0, 0
0num_vectors, max_msg_size, max_sig_size = 0, 0, 0
59+print("/* Note: this file was autogenerated using tests_wycheproof_generate.py. Do not edit. */")
60+print("#define SECP256K1_ECDSA_WYCHEPROOF_NUMBER_TESTVECTORS ({})".format(num_cases))
61+print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_MSG_SIZE ({})".format(max_msg_size))
62+print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_SIG_SIZE ({})".format(max_sig_size))
63+print(struct_definition)
64+print("static const wycheproof_ecdsa_testvector testcases[SECP256K1_ECDSA_WYCHEPROOF_NUMBER_TESTVECTORS] = {")
0print("static const wycheproof_ecdsa_testvector testvectors[SECP256K1_ECDSA_WYCHEPROOF_NUMBER_TESTVECTORS] = {")
21+for i in range(num_groups):
22+ group = doc['testGroups'][i]
23+ num_tests = len(group['tests'])
24+ public_key = group['publicKey']
25+ for j in range(num_tests):
26+ test_case = group['tests'][j]
0 test_vector = group['tests'][j]
0@@ -0,0 +1,212 @@
1+* The file `ecdsa_secp256k1_sha256_bitcoin_test.json` in this directory
2+ comes from Google's project Wycheproof with git commit
3+ `b063b4aedae951c69df014cd25fa6d69ae9e8cb9`.
4+ The Wycheproof project can be found at https://github.com/google/wycheproof
In the spirit of @Sajjon ’s suggestion:
0* The file `ecdsa_secp256k1_sha256_bitcoin_test.json` in this directory
1 comes from Google's project Wycheproof with git commit
2 `b063b4aedae951c69df014cd25fa6d69ae9e8cb9`, see
3 https://github.com/google/wycheproof/blob/b063b4aedae951c69df014cd25fa6d69ae9e8cb9/testvectors_v1/ecdsa_secp256k1_sha256_bitcoin_test.json .
I think this is useful, I first looked in the wrong directory (testvectors instead of the new testvectors_v1…).
Thanks for the quick fixups!
I put the license file in
src/wycheproof/WYCHEPROOF_COPYING. Like this, the name does not clash with the top levelCOPYINGfile (which is a different license). This is the same approach as withexamples/EXAMPLES_COPYING.
Good catch!
Here are some more comments, but this is good to go from my side, with or without these addressed.
ACK 04c70ffacc04ef120d8c700a9e1e650bde0e2570
Here are some more comments, but this is good to go from my side, with or without these addressed.
Thanks for the quick review! I just pushed changes to address all the remaining comments.
Do we need any other extra approval?
Yes. There’s no fixed rule, but I’d say we need one more.
Things can be a bit slow, we don’t have many active people. But I’m sure we’ll have another review soon.
This encoding using maximum-length padded arrays for all inputs does result in about 2 MiB of redundant zeroes in the binaries (and even more in the checked-in version of the .h file).
What do you think about an encoding using separately declared array constants for all the messages/signatures, pointed to by the main wycheproof_ecdsa_testvector struct elements?
Agreed, the encoding used here is not the most compact. We did it this way to keep things simple, but can change it. @sipa would this make more sense to you?
wycheproof_ecdsa_testvector to store offsets for the previous arrays instead of the actual messages/signatures
This encoding using maximum-length padded arrays for all inputs does result in about 2 MiB of redundant zeroes in the binaries (and even more in the checked-in version of the .h file).
What do you think about an encoding using separately declared array constants for all the messages/signatures, pointed to by the main
wycheproof_ecdsa_testvectorstruct elements?
Then we lose the other important property which is portability.
Fine for the wycheproof vectors since we are not the original source, the Wycheproof github repo is.
But if we generate and vendor our own test vectors for other libraries to use, it would be great if those we portable, eg in JSON format to be used as is.
What are your thoughts on that?
I would eg love to see a standardization around serialization of recoverable signatures. Some libraries chose (using V for recid) format: V || R || S and some use R || S || V, and there exist few test vectors for recovery on interwebz. So would be great if libsecp256k1 could vendor some, and on a portable format.
tests binary size by less than 82 KiB on a dev machine (total size 2.0 MiB before and 2.1 MiB after).
77+
78+
79+print("/* Note: this file was autogenerated using tests_wycheproof_generate.py. Do not edit. */")
80+print("#define SECP256K1_ECDSA_WYCHEPROOF_NUMBER_TESTVECTORS ({})".format(num_vectors))
81+print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_MSG_SIZE ({})".format(max_msg_size))
82+print("#define SECP256K1_ECDSA_WYCHEPROOF_MAX_SIG_SIZE ({})".format(max_sig_size))
max_msg_size and friends unused. Removing them too…
45+ if test_vector['result'] == "invalid": expected_verify = 0
46+ elif test_vector['result'] == "valid": expected_verify = 1
47+ else: raise ValueError("invalid result field")
48+
49+ if num_vectors != 0 and msg_size != 0: messages += ", "
50+ if num_vectors != 0 and sig_size != 0: signatures += ", "
0 if num_vectors != 0 and msg_size != 0: messages += ",\n"
1 if num_vectors != 0 and sig_size != 0: signatures += ",\n"
This will result in much more readable output. And we should avoid very long lines. Some tools don’t like them, e.g., some editors become very slow.
@Sajjon That seems unrelated to this PR, but I see no reason why any such portable test vectors couldn’t use a similar approach.
Indeed. If we want to create portable test vectors, we could still create them in JSON and add have another conversion script.
Following this PR, I think perhaps we may want to also add Python code to e.g. convert the BIP340 CSV test vectors to C source code, rather than just hardcoding.
It’s cleaner, but now that we have them in C, I don’t think it’s worth the effort. They won’t change often. (Well, #1133 adds some but even this PR is ready already…)
Now the array wycheproof_ecdsa_messages stores unique messages only, resulting in a more compact .h file since there were so many repeated messages.
tests_wycheproof_generate.py since we’re detecting repeated messages.tests.c file is unaffected by this change since we’re storing pointers to wycheproof_ecdsa_messages in testvectors anyways.
Adds a test using the Wycheproof vectors as outlined in #1106. The
vectors are taken from the Wycheproof repo. We use a python script
to convert the JSON-formatted vectors into C code.
Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>
