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
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
447334cb06 include: Avoid visibility("default") on Windows (Tim Ruffing)
Pull request description:
Fixes#1421. See code comments for rationale.
Related meta-bug: #1181. This reminds me that we should move forward with #1359.
ACKs for top commit:
fanquake:
ACK 447334cb06
hebasto:
ACK 447334cb06, tested on Ubuntu 24.04 using the following commands:
theuni:
ACK 447334cb06
Tree-SHA512: aaa47d88fd1b1f85c3e879a2b288c0eb3beebad0cc89e85f05d0b631f83e58d5a324fb441911970865eaa292f6820d03a1b516d6e8de37a87510e2082acc6e28
8be3839fb2 Remove unused scratch space from API (Jonas Nick)
Pull request description:
We had already merged this in #1305, but it was reverted before a release (#1311) because this change is not backwards compatible but at the time we only wanted to make a patch release in order to fix an actual issue.
Due to the musig module, the next release will increment the version number from 0.5.x to 0.6.0, so it would be a good time to remove the scratch space from the API.
ACKs for top commit:
sipa:
utACK 8be3839fb2
real-or-random:
utACK 8be3839fb2
Tree-SHA512: ecd6bc1d925992f9df8e26820388fc436bbb6bc5f250950edf00406f006ca0df52ab8cd56a1b7541e57af0682ddadf6d34bd638b27557d301a5dff6c327a5ebc
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.
c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" (Hennadii Stepanov)
26e4a7c214 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
While testing https://github.com/bitcoin-core/secp256k1/pull/1551, I noticed that when cross-compiling a shared library with examples for Windows, the `ctest` fails to run examples with Wine. Adjusting the `PATH` variable in 4af241b320/examples/CMakeLists.txt (L16-L18) does not help because `WINEPATH` is expected.
Another issue with the current implementation is that the examples cannot run individually on Windows.
This PR resolves both issues by reverting the implementation from https://github.com/bitcoin-core/secp256k1/pull/1290 in favour of the reworked and improved implementation from https://github.com/bitcoin-core/secp256k1/pull/1233.
ACKs for top commit:
theuni:
Concept ACK and utACK c232486d84.
real-or-random:
utACK c232486d84
Tree-SHA512: 479b71d15d5d5670f6f69da3da599240c345711003383ca805c821b67065c9baaf269f987792cf1029211cdbfe799aecd401e6940a471539e3929b4a90e0781d
ef7ff03407 f can never equal -m (Russell O'Connor)
Pull request description:
In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
ensuring that f is not -m.
ACKs for top commit:
sipa:
ACK ef7ff03407
real-or-random:
utACK ef7ff03407
Tree-SHA512: a8a8dcbad4dff36b9c49e40e07b212312cbf915132aea008eab6ea61b35bddb6d7782229c2cc528fb404d05132482c602cad768414d76153bb425a3d23714fff
168c92011f build: allow enabling the musig module in cmake (Jonas Nick)
f411841a46 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) (Jonas Nick)
0be79660f3 util: add constant-time is_zero_array function (Jonas Nick)
c8fbdb1b97 group: add ge_to_bytes_ext and ge_from_bytes_ext (Jonas Nick)
85e224dd97 group: add ge_to_bytes and ge_from_bytes (Jonas Nick)
Pull request description:
EDIT: based on #1518. Closes#1452. Most of the code is a copy from [libsecp256k1-zkp](https://github.com/BlockstreamResearch/secp256k1-zkp). The API added in this PR is identical with the exception of two modifications:
1. I removed the unused `scratch_space` argument from `secp256k1_musig_pubkey_agg`. This argument was intended to allow using `ecmult_multi` algorithms for key aggregation in the future. But at this point it's unclear whether the `scratch_space` object will remain in its current form (see #1302).
2. Support for adaptor signatures was removed and therefore the `adaptor` argument of `musig_nonce_process` was also removed.
In contrast to the module in libsecp256k1-zkp, the module is non-experimental. I slightly cleaned up parts of the module, adjusted the code to the new definition of the VERIFY_CHECK macro and applied some simplifications that were possible because the module is now in the upstream repo (`ge_from_bytes`, `ge_to_bytes`). You can follow the changes I made to the libsecp256k1-zkp module at https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/.
ACKs for top commit:
sipa:
reACK 168c92011f
real-or-random:
reACK 168c92011f
theStack:
re-ACK 168c92011f
Tree-SHA512: e3a599a8d5a466107b9a86f76582b8fb9dc87ec95416c784c3ef39d1c64686e6c739806ed6ba62c91793eb7fa418a6270cf999027ee7bd3dd85c67bc2c74f677
