59860bcc24 gha: Print all *.log files, in a separate action (Tim Ruffing)
Pull request description:
Before this commit, we didn't print *_example.log files and
test_suite.log.
Printing is now handled in a separate action, which avoids code
duplication and makes the ci.yml file more readable. This changes the
folding/grouping of the log output in the GitHub Actions CI, but I
think the new variant is as good as the old one.
Furthermore, the condition for printing the logs is changed from
"always()" to "!cancelled()". This ensures that logs will still be
printed if previous steps such as the CI script failed, but that they
won't be printed if the entire run is cancelled (e.g., by clicking a
button in the UI or through a force-push to the PR). This is in line
with a recommendation in the GHA docs:
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always
ACKs for top commit:
hebasto:
ACK 59860bcc24.
sipa:
ACK 59860bcc24
Tree-SHA512: ca11f5e5f01660964276b9c2e11c22caeed8492e9c7ffaa2078aaaa733005c63242fc93a1056124fb8f1f83019d46818c12b10142fb10f43270a8562fd10885a
Before this commit, we didn't print *_example.log files and
test_suite.log.
Printing is now handled in a separate action, which avoids code
duplication and makes the ci.yml file more readable. This changes the
folding/grouping of the log output in the GitHub Actions CI, but I
think the new variant is as good as the old one.
Furthermore, the condition for printing the logs is changed from
"always()" to "!cancelled()". This ensures that logs will still be
printed if previous steps such as the CI script failed, but that they
won't be printed if the entire run is cancelled (e.g., by clicking a
button in the UI or through a force-push to the PR). This is in line
with a recommendation in the GHA docs:
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always
4c50d73dd9 ci: Add new "Windows (clang-cl)" job (Hennadii Stepanov)
84c0bd1f72 cmake: Adjust diagnostic flags for clang-cl (Hennadii Stepanov)
Pull request description:
When building with `clang-cl` on Windows, the output is cluttered with warning messages because compiler diagnostic flags are not applied correctly:
```
> cmake -B build -G Ninja -DCMAKE_C_COMPILER="C:\Users\hebasto\Downloads\clang+llvm-18.1.8-x86_64-pc-windows-msvc\bin\clang-cl.exe"
> cmake --build build
[1/16] Building C object src\CMakeFiles\bench.dir\bench.c.obj
In file included from C:\Users\hebasto\secp256k1\src\bench.c:11:
C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function]
34 | static void print_buf_plain(const unsigned char *buf, size_t len) {
| ^~~~~~~~~~~~~~~
1 warning generated.
[2/16] Building C object src\CMakeFiles\secp256k1_precomputed.dir\precomputed_ecmult_gen.c.obj
In file included from C:\Users\hebasto\secp256k1\src\precomputed_ecmult_gen.c:3:
In file included from C:\Users\hebasto\secp256k1\src\group.h:10:
In file included from C:\Users\hebasto\secp256k1\src\field.h:10:
C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function]
34 | static void print_buf_plain(const unsigned char *buf, size_t len) {
| ^~~~~~~~~~~~~~~
```
This PR resolves this issue.
---
**Additional note for reviewers:** The VS builtin clang can also be used assuming that the following VS components are installed:
The user can generate a build system on Windows as follows:
- Using the default "Visual Studio" generator:
```
cmake -B build -T ClangCL
```
- Using the "Ninja" generator:
```
cmake -B build -G Ninja -DCMAKE_C_COMPILER=clang-cl
```
---
Required for downstream projects which aim to build with `clang-cl` (see https://github.com/bitcoin/bitcoin/issues/31456).
ACKs for top commit:
real-or-random:
utACK 4c50d73dd9
Tree-SHA512: 439eb53afd7be65d538cd569f3d095f58325bd26ffc5014ca5f94320689a45b20c9a5a963170578214a20fd3233ec15ef6ab75ab96ce3a4314c282b1b6229ca1
These function aliases have been described as DEPRECATED in the public
API docs already many years ago (see #701, commit 41fc7856), and in
addition explicit deprecation warnings are shown by the compiler at
least since the first official release 0.2.0 (see PR #1089, commit
fc94a2da), so it should be fine to just remove them by now.
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
1823594761 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` (Sebastian Falbesoner)
Pull request description:
Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`, public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly passed to `secp256k1_eckey_pubkey_serialize` in newly proposed code (this is currently the case for several modules in secp256k1-zkp, see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/300), which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that the `compressed` argument is either 0 or 1.
ACKs for top commit:
real-or-random:
utACK 1823594761
stratospher:
ACK 1823594. Got tests failures when passing public API flags to `secp256k1_eckey_pubkey_serialize`.
Tree-SHA512: ca542afc87f33e436ba33dc55b285dfe3759007c446ef94503bc1044c7a0a7f7b2208ae82e2c9743fc5fa38cf386127f3fbfa02d2c242f28fab3041ee46f153b
13d389629a CONTRIBUTING: mention that `EXIT_` codes should be used (Sebastian Falbesoner)
c855581728 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner)
965393fcea examples: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner)
Pull request description:
This simple PR addresses #1609 for all example and internal binaries. Alternative to #1618, which is stale (the author confirmed to me that they are not working on that PR anymore). The last commits adds a suggestion to CONTRIBUTING.md, not sure though if we want to go that far.
ACKs for top commit:
jonasnick:
ACK 13d389629a
real-or-random:
utACK 13d389629a
Tree-SHA512: 513eba4b712ba3d5f23a5fdc51cb27c5347b29bcaba39501345913c220be6f093a41186911032d2ddc898b848de84f05f374b3554ffcf92610728b2a23c0bb36
2ac9f558c4 doc: Improve cmake instructions in README (Fabian Jahr)
Pull request description:
Minor improvement suggestion for the readme. I find this alternative way of using cmake a bit more comfortable because I don't like to change the directory.
It's just a suggestion based on personal preference, if this is too minor of an improvement feel free to close.
ACKs for top commit:
hebasto:
ACK 2ac9f558c4.
real-or-random:
utACK 2ac9f558c4
Tree-SHA512: 5f7bc8b5ff91fb7a115a0e57224c66b018cfc824784e0def1064d07f9be66efe55e1a71e034f6a3d6489e063995c1ae17a9e91c990a0944d600cc957c038909d
Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`,
public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly
passed to newly proposed code (this is currently the case for several modules in
secp256k1-zkp, see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/300).
which is currently not detected. To avoid this in the future, a VERIFY_CHECK
is added to check that the `compressed` argument is either 0 or 1.
39705450eb Fix some misspellings (Nicolas Iooss)
Pull request description:
Hello,
Some files contained English misspellings or math issues (`lamba` instead of `lambda`), mainly in comments. Fixing them helps readability.
By the way, the misspellings found in the Wycheproof test vector file were also reported upstream: https://github.com/C2SP/wycheproof/issues/124
ACKs for top commit:
real-or-random:
utACK 39705450eb
Tree-SHA512: 36327e8bb58ef3c0408cf4966bb33f51c84b1614809d8711d86eaf3d4e5336ae8c663593cb5f0e9c56adbb2d7f2ca62a9b84cae1b76b9811c110f87f1defa624
39d5dfd542 release: prepare for 0.6.0 (Jonas Nick)
df2eceb279 build: add ellswift.md and musig.md to release tarball (Jonas Nick)
a306bb7e90 tools: fix check-abi.sh after cmake out locations were changed (Jonas Nick)
145868a84d Do not export `secp256k1_musig_nonce_gen_internal` (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
sipa:
utACK 39d5dfd542
real-or-random:
ACK 39d5dfd542 mod the CI results
Tree-SHA512: 9b4623ca03aafcd1e04b0809382faeb3b427d3d07062f065177c7608e4feb30abd52cb10fa8c06b7ae17a82b32455e995b6bd39e3ef6239d5fc65c78873385b0
765ef53335 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner)
349e6ab916 Introduce separate _clear functions for hash module (Tim Ruffing)
99cc9fd6d0 Don't rely on memset to set signed integers to 0 (Tim Ruffing)
97c57f42ba Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing)
9bb368d146 Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing)
e3497bbf00 Separate between clearing memory and setting to zero in tests (Tim Ruffing)
d79a6ccd43 Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing)
1c08126222 Add secp256k1_memclear() for clearing secret data (Tim Ruffing)
e7d384488e Don't clear secrets in pippenger implementation (Tim Ruffing)
Pull request description:
This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master.
Some changes to the original PR:
* the clearing function now has the `secp256k1_` prefix again, since the related helper `_memczero` got it as well (see PR #835 / commit e89278f211)
* the original commit b17a7df814 ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR #943 (commit d49011f54c)
* clearing of stack memory with `secp256k1_memclear` is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered)
So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in https://github.com/bitcoin-core/secp256k1/pull/636#issuecomment-620118629), happy to go deeper there if this gets Concept ACKed.
The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. 5af122c3df/crypto/mem.c (L335) and d456068672/include/linux/string.h (L348) / d456068672/include/linux/compiler.h (L102)Fixes#185.
ACKs for top commit:
sipa:
reACK 765ef53335
real-or-random:
ACK 765ef53335
Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959
694342fdb7 Name public API structs (Ava Chow)
Pull request description:
Closes#1627
ACKs for top commit:
real-or-random:
utACK 694342fdb7
jonasnick:
ACK 694342fdb7
Tree-SHA512: 4e03d97e7c072fc7ddefe3f679878aa8a806f3f557a736c9a1b9137972798c953cb21b91491d65f7ba5d75d7119e3224ce60309a0ff93fcf9a64b57b4a426655
0f73caf7c6 test, ci: Lower default iteration count to 16 (Hennadii Stepanov)
Pull request description:
The number of test iterations in the CI remains the same.
Resolves https://github.com/bitcoin-core/secp256k1/issues/1561.
```
$ ./build/src/tests
test count = 16
random seed = 59ea2b21267ec0ef0b4d13821292489f
random run = 2936c044f82c7598a866869b9d954d42
no problems found
```
ACKs for top commit:
sipa:
utACK 0f73caf7c6
jonasnick:
ACK 0f73caf7c6
Tree-SHA512: 84b265dc5d2780b3ea0a38f50ac8871d850ef2c97f33a0a5816baf20ac71c01db8b85696b343b089d7116d9cdb9450a6ca668229d95e54a39920d0e91a3127b3
The number of test iterations in the CI remains unchanged.
Additionally, the minimum iteration counts to enable the
`test_ecmult_constants_2bit` test is adjusted from 35 to 16, so it is
run by default.
Quoting sipa (see https://github.com/bitcoin-core/secp256k1/pull/1479#discussion_r1790079414):
"When performing an EC multiplication A = aG for secret a, the resulting
_affine_ coordinates of A are presumed to not leak information about a (ECDLP),
but the same is not necessarily true for the Jacobian coordinates that come
out of our multiplication algorithm."
For the ECDH point multiplication result, the result in Jacobi coordinates should be
cleared not only to avoid leaking the scalar, but even more so as it's a representation
of the resulting shared secret.
