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
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.
This gives the caller more control about whether the state should
be cleaned (= should be considered secret). Moreover, it gives the
caller the possibility to clean a hash struct without finalizing it.
All of the invocations of secp256k1_memclear() operate on stack
memory and happen after the function is done with the memory object.
This commit replaces existing memset() invocations and also adds
secp256k1_memclear() to code locations where clearing was missing;
there is no guarantee that this commit covers all code locations
where clearing is necessary.
Co-Authored-By: isle2983 <isle2983@yahoo.com>
There are two uses of the secp256k1_fe_clear() function that are now separated
into these two functions in order to reflect the intent:
1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 )
2) zeroing the memory after being used such that no sensitive data remains. ->
remains as fe_clear()
In the latter case, 'magnitude' and 'normalized' need to be overwritten when
VERIFY is enabled.
Co-Authored-By: isle2983 <isle2983@yahoo.com>
We rely on memset() and an __asm__ memory barrier where it's available or
on SecureZeroMemory() on Windows. The fallback implementation uses a
volatile function pointer to memset which the compiler is not clever
enough to optimize.
980c08df80 util: Remove unused (u)int64_t formatting macros (Tim Ruffing)
Pull request description:
We should anyway prefer to use the predefined macros from <inttypes.h>.
If I haven't missed anything, this removes the last OS-specific #if, leaving us only with compiler-specific #if(def)s.
ACKs for top commit:
theStack:
utACK 980c08df80
Tree-SHA512: bcfc962618c6d0343c8231f9ea5ca23029b4e4946c4239cd9732933fe7065963d7c0ef2db60f72b76e0721865a61b8a9957b62398bb2d0b8f6bbc1d25461f1b3
We should anyway prefer to use the predefined macros from <inttypes.h>.
If I haven't missed anything, this removes the last OS-specific #if,
leaving us only with compiler-specific #if(def)s.
096e3e23f6 ci: Update macOS image (Hennadii Stepanov)
Pull request description:
The macOS 12 GHA image has been deprecated since 2024-10-07. See: https://github.com/actions/runner-images/issues/10721.
Draft for now as `./libtool --mode=execute valgrind --error-exitcode=42 ./ctime_tests` fails.
ACKs for top commit:
real-or-random:
ACK 096e3e23f6
Tree-SHA512: 715e7d2638bb7161c756d3856ee7eb6826f2300ab215deb888f040881c6b8cddc311c206f90dd942844ee2e56247e8ca99078a229e80ef086c2a4fdd8937af9d
57eda3ba30 musig: ctimetests: fix _declassify range for generated nonce points (Sebastian Falbesoner)
Pull request description:
As noticed in https://github.com/bitcoin-core/secp256k1/pull/1614#discussion_r1796215582, the area marked as non-secret exceeds the nonce_pts array in the second iteration of the for loop. Fix that by passing the correct size to the _declassify call.
ACKs for top commit:
sipa:
utACK 57eda3ba30
real-or-random:
utACK 57eda3ba30
Tree-SHA512: ff8074e3d1078d66a52d08c661997856ff586b3b4564a865a75212b32fafd7906d58885371bd63005007fde554ebcad121ab66125abe4331cf0aac63fc018ed0
