While looking up past review history of the previous silentpayments PR (take 3), I’ve found that adding the input JSON files for generating the musig test vectors was suggested once there: #1698 (comment)
Can be tested by first comparing that the .json files match the ones from the bips repository’s bip-0327/vectors folder and then running e.g.
0$ rm src/modules/musig/vectors.h
1$ make src/modules/musig/vectors.h
2$ git diff
3<should not show any diff>
Concept ACK
I was also able to run these commands and get the same results
0$ rm src/modules/musig/vectors.h
1$ make src/modules/musig/vectors.h
2$ git diff
3<should not show any diff>
0$ rm src/modules/musig/vectors.h 1$ make src/modules/musig/vectors.h 2$ git diff 3<should not show any diff>
Same for CMake?
0$ rm src/modules/musig/vectors.h 1$ make src/modules/musig/vectors.h 2$ git diff 3<should not show any diff>
It would be helpful to align this workflow with existing patterns.
For src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h, we currently run:
0$ make clean-testvectors
1$ make testvectors
We should probably use the same approach for the musig module.
While looking up past review history of the previous silentpayments PR (take 3), I’ve found that adding the input JSON files for generating the musig test vectors was suggested once there: #1698 (comment)
The comment mentions doing this “without further dependencies”. Note that python3 is not technically a new dependency, as it is already used in the build system: https://github.com/bitcoin-core/secp256k1/blob/2d9137ce9d7e576bfada2859292ecaa0887a6b8c/Makefile.am#L260-L266
However, directly invoking python3 is not portable and may fail on some systems. The Autotools-based build system should use the configured $(PYTHON) variable (or similar) instead.
@hebasto: Thanks for reviewing!
Same for CMake?
Adding support for test vectors generation with CMake has been proposed before, but was rejected, see #1723.
It would be helpful to align this workflow with existing patterns.
For src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h, we currently run:
$ make clean-testvectors $ make testvectors
We should probably use the same approach for the musig module.
Currently, the {clean-,}testvectors targets build all the available test vectors, include the musig one in this PR branch (if the module is enabled). I guess that’s fine and separate targets only for musig are not needed?
However, directly invoking python3 is not portable and may fail on some systems. The Autotools-based build system should use the configured $(PYTHON) variable (or similar) instead.
Since that affects all existing test vectors generation targets and not only the musig one, that seems to be a topic for a different PR. (Of course, also happy to include a commit directly in this PR, if its seen as uncontroversial and reviewers prefer that.)
While testing this PR by running
0$ make clean-testvectors; make testvectors
I expected to get a non-zero diff after arbitrarily modifying the JSON files. However, that’s not the case. For example, the following diff does not result in changes to the generated header:
0--- a/src/modules/musig/vectors/det_sign_vectors.json
1+++ b/src/modules/musig/vectors/det_sign_vectors.json
2@@ -26,7 +26,7 @@
3 },
4 {
5 "rand": null,
6- "aggothernonce": "0337C87821AFD50A8644D820A8F3E02E499C931865C2360FB43D0A0D20DAFE07EA0287BF891D2A6DEAEBADC909352AA9405D1428C15F4B75F04DAE642A95C2548480",
7+ "aggothernonce": "1337C87821AFD50A8644D820A8F3E02E499C931865C2360FB43D0A0D20DAFE07EA0287BF891D2A6DEAEBADC909352AA9405D1428C15F4B75F04DAE642A95C2548480",
8 "key_indices": [1, 0, 2],
9 "tweaks": [],
10 "is_xonly": [],
Are those JSON files redundant?
@hebasto: Good catch, looks like currently only six out of the eight .json files from the bips repository are used for test vectors generation:
0$ git grep "\.json" ./tools/test_vectors_musig2_generate.py
1tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/key_agg_vectors.json", "r") as f:
2tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/nonce_gen_vectors.json", "r") as f:
3tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/nonce_agg_vectors.json", "r") as f:
4tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/sign_verify_vectors.json", "r") as f:
5tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/tweak_vectors.json", "r") as f:
6tools/test_vectors_musig2_generate.py:with open(sys.argv[1] + "/sig_agg_vectors.json", "r") as f:
Hence, the other two (det_sign_vectors.json and key_sort_vectors.json) are indeed not needed and could be removed. On the other hand, keeping the folder in sync with https://github.com/bitcoin/bips/tree/master/bip-0327/vectors seems to be more consistent. Happy to go either way, curious to hear what maintainers and reviewers prefer.