- `secp256k1_scalar_set_int` in scalar_low uses input mod EXHAUSTIVE_TEST_ORDER
- directly store s in sig64 without reducing it mod the group order for testing
5a95a268b9 tests: introduce helper for non-zero `random_fe_test` results (Sebastian Falbesoner)
304421d57b tests: refactor: remove duplicate function `random_field_element_test` (Sebastian Falbesoner)
Pull request description:
There are several instances in the tests where random non-zero field elements are generated by calling `random_fe_test` in a do/while-loop with is-zero condition. This PR deduplicates all these by introducing a `random_fe_non_zero_test` helper. Note that some instances checked the is-zero condition via `secp256k1_fe_normalizes_to_zero_var`, which is unnecessary, as the result of `random_field_element_test` is already normalized (so strictly speaking, this is not a pure refactor, and there could be tiny run-time improvements, though I doubt that's measurable).
Additionally, the first commit removes the function `random_field_element_test` as it is logically a duplicate of `random_fe_test`.
ACKs for top commit:
real-or-random:
ACK 5a95a268b9
Tree-SHA512: 920404f38ebe8b84bfd52f3354dc17ae6a0fd6355f99b78c9aeb53bf21f7eca5fd4518edc8a422d84f430ae95864661b497de42a3ab7fa9c49515a1df2f1d466
There are several instances in the tests where random non-zero field
elements are generated by calling `random_fe_test` in a do/while-loop.
This commit deduplicates all these by introducing a
`random_fe_non_zero_test` helper. Note that some instances checked the
is-zero condition via `secp256k1_fe_normalizes_to_zero_var`, which is
unnecessary, as the result of `random_fe_test` is already normalized (so
strictly speaking, this is not a pure refactor).
There is a function `random_fe_test` which does exactly the
same, so use that instead. Note that it's also moved up before the
`random_group_element_test` function, in order to avoid needing a forward
declaration.
be8ff3a02a field: Static-assert that int args affecting magnitude are constant (Tim Ruffing)
Pull request description:
See #1001.
Try to revert the lines in `tests.c` to see the error message in action.
ACKs for top commit:
sipa:
ACK be8ff3a02a. Verified by introducing some non-constant expressions and seeing compilation fail.
theStack:
ACK be8ff3a02a
Tree-SHA512: 8befec6ee64959cdc7f3e29b4b622410794cfaf69e9df8df17600390a93bc787dba5cf86239de6eb2e99c038b9aca5461e4b3c82f0e0c4cf066ad7c689941b19
7d8d5c86df tests: refactor: take use of `secp256k1_ge_x_on_curve_var` (Sebastian Falbesoner)
Pull request description:
The recently merged ellswift PR (#1129) introduced a helper `secp256k1_ge_x_on_curve_var` to check if a given X coordinate is on the curve (i.e. the expression x^3 + 7 is square, see commit 79e5b2a8b8). This can be used for code deduplication in the `ecmult_const_mult_xonly` test.
(Found this instance via `$ git grep add_int.*SECP256K1_B`, I think it's the only one where the helper can be used.)
ACKs for top commit:
sipa:
utACK 7d8d5c86df
real-or-random:
utACK 7d8d5c86df
Tree-SHA512: aebff9b5ef2f6f6664ce89e4e1272cb55b6aac81cfb379652c4b7ab30dd1d7fd82a2c3b47c7b7429755ba28f011a3a9e2e6d3aa5c77d3b105d159104c24b89f3
c862a9fb49 ci: Adjust Docker image to Debian 12 "bookworm" (Hennadii Stepanov)
a1782098a9 ci: Force DWARF v4 for Clang when Valgrind tests are expected (Hennadii Stepanov)
8a7273465b Help the compiler prove that a loop is entered (Tim Ruffing)
Pull request description:
Since the [release](https://www.debian.org/News/2023/20230610.html) of Debian 12 "bookworm", it has become the "stable" one that our `ci/linux-debian.Dockerfile` relies on.
Last time the Docker image was built basing on Debian Bullseye.
Changes in packages are significant, for instance:
- `gcc` 10.2. --> 12.2
- `clang` 11.0 --> 14.0
- `wine` 5.0 --> 8.0
which requires certain adjustments provided in this PR.
The first commit has been cherry-picked from https://github.com/bitcoin-core/secp256k1/pull/1313.
ACKs for top commit:
sipa:
utACK c862a9fb49
real-or-random:
ACK c862a9fb49
Tree-SHA512: 2a62a8865f904a460274f1f3ec02d2b0b72c84b25722a383c6455cfe672c1d93382941a5027e8dceb2c0f5fe0f0efd49a0ed6b72303982f9e32991f1535538eb
The recently merged ellswift PR (#1129) introduced a helper
`secp256k1_ge_x_on_curve_var` to check if a given X coordinate is
valid (i.e. the expression x^3 + 7 is square, see commit
79e5b2a8b8). This can be used for code
deduplication in the `ecmult_const_mult_xonly` test.
67887ae65c Fix a typo in the error message (Hennadii Stepanov)
Pull request description:
The code has been copy-pasted from the `precompute_ecmult_gen.c` source file.
ACKs for top commit:
real-or-random:
ACK 67887ae65c
Tree-SHA512: d6874949310197e5d2d6c43f5a7c2165b4ee0f6cbe3cc1491d0f97163fa5329ebeab2b2adf10246c87382016fbe738c69dfd3f2253e93c906bf404cbf439b12a
7c7467ab7f Refer to ellswift.md in API docs (Pieter Wuille)
c32ffd8d8c Add ellswift to CHANGELOG (Pieter Wuille)
Pull request description:
A follow-up with a CHANGELOG entry for #1129.
ACKs for top commit:
real-or-random:
ACK 7c7467ab7f
theStack:
ACK 7c7467ab7f
Tree-SHA512: 4f066e4b8d5e130f2b5bea0ed4c634e9426bc576342aad6c306e0805a8354e27a5e679b15ec869d4e7d36eb5d53174e46b3bf5e15d19a7e165afc82e46ddfcf5
bc7c8db179 abi: Use dllexport for mingw builds (Cory Fields)
Pull request description:
Addresses the first part of #1181. See the discussion there for more context and history.
After this, all that remains is a (platform-independent) exports checker for c-i. Or perhaps a linker script or .def file could be tricked into testing as a side-effect.
This should fix mingw exports, specifically hiding the following:
`secp256k1_pre_g_128`
`secp256k1_pre_g`
`secp256k1_ecmult_gen_prec_table`
This changes our visibility macros to look more like [gcc's recommendation](https://gcc.gnu.org/wiki/Visibility#How_to_use_the_new_C.2B-.2B-_visibility_support).
Edit:
Note that we could further complicate this by supporting `__attribute__ ((dllexport))` as well, though I didn't bother as I'm not sure what compiler combo would accept that but not the bare dllexport syntax.
Edit2:
As the title implies, this affects this ABI and could affect downstream libs/apps in unintended ways (though it's hard to imagine any real downside). Though because it's win32 only, I'm imagining very little real-world impact at all.
ACKs for top commit:
hebasto:
re-ACK bc7c8db179, only a comment has been adjusted since my recent [review](https://github.com/bitcoin-core/secp256k1/pull/1295#pullrequestreview-1414928537),
real-or-random:
utACK bc7c8db179
Tree-SHA512: 378e15556da49494f551bdf4f7b41304db9d03a435f21fcc947c9520aa43e3c655cfe216fba57a5179a871c975c806460eef7c33b105f2726e1de0937ff2444e
5b7bf2e9d4 Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin-core/secp256k1/issues/1324.
As the `__shiftright128` [docs](https://learn.microsoft.com/en-us/cpp/intrinsics/shiftright128) state:
> The `Shift` value is always modulo 64...
it is not applicable for the `n >= 64` branch.
ACKs for top commit:
sipa:
utACK 5b7bf2e9d4
real-or-random:
ACK 5b7bf2e9d4 tested with MSVC x64
Tree-SHA512: bc4c245a9da83c783a0479e751a4bc2ec77a34b99189fcc4431033a5420c93b610f3b960d3f23c15bce2eb010beba665b3e84d468b3fdab3d5846d4f27016898
e449af6872 Drop no longer needed `#include "../include/secp256k1.h"` (Hennadii Stepanov)
Pull request description:
The removed header includes have not been needed since https://github.com/bitcoin-core/secp256k1/pull/1231.
Test suggestions:
1. Using Autottols-based build system:
```
./autogen.sh
./configure
make clean-precomp
make
```
2. Using CMake-based build system:
```
cmake -B build -DCMAKE_C_INCLUDE_WHAT_YOU_USE="include-what-you-use"
cmake --build build --target secp256k1_precomputed
```
ACKs for top commit:
sipa:
utACK e449af6872
real-or-random:
utACK e449af6872
Tree-SHA512: 5aed7a88e1e03fcc2306c43817712c0652ecf6145679dd17f4719376818d372f619e4180bdaee548f2e82aaccbe6a2ff4c37203121d939af545128c8c48b933e
The scheme implemented is described below, and largely follows the paper
"SwiftEC: Shallue–van de Woestijne Indifferentiable Function To Elliptic Curves",
by Chavez-Saab, Rodriguez-Henriquez, and Tibouchi
(https://eprint.iacr.org/2022/759).
A new 64-byte public key format is introduced, with the property that *every*
64-byte array is an encoding for a non-infinite curve point. Each curve point
has roughly 2^256 distinct encodings. This permits disguising public keys as
uniformly random bytes.
The new API functions:
* secp256k1_ellswift_encode: convert a normal public key to an ellswift 64-byte
public key, using additional entropy to pick among the many possible
encodings.
* secp256k1_ellswift_decode: convert an ellswift 64-byte public key to a normal
public key.
* secp256k1_ellswift_create: a faster and safer equivalent to calling
secp256k1_ec_pubkey_create + secp256k1_ellswift_encode.
* secp256k1_ellswift_xdh: x-only ECDH directly on ellswift 64-byte public keys,
where the key encodings are fed to the hash function.
The scheme itself is documented in secp256k1_ellswift.h.
f1652528be Normalize ge produced from secp256k1_pubkey_load (stratospher)
Pull request description:
The output `ge` in secp256k1_pubkey_load is normalized when `sizeof(secp256k1_ge_storage) = 64` but not when it's not 64. ARG_CHECK at the end of the function assumes normalization. So normalize ge in the other code path too.
context: [#1129(comment)](https://github.com/bitcoin-core/secp256k1/pull/1129/files#r1196167066)
ACKs for top commit:
sipa:
utACK f1652528be
real-or-random:
ACK f1652528be tested by changing the two `== 64` checks to `== 65`
Tree-SHA512: 0de1caad85ccdb42053f8e09576135257c88fda88455ef25e7640049c05a1e03d1e9bae1cd132d2e6fc327fd79929257a8b21fe1cc41c82374b6cd88e6744aa3
7067ee54b4 tests: add tests for `secp256k1_{read,write}_be64` (Sebastian Falbesoner)
740528caad scalar: use newly introduced `secp256k1_{read,write}_be64` helpers (4x64 impl.) (Sebastian Falbesoner)
Pull request description:
This is a simple follow-up to #1339, as suggested in comment https://github.com/bitcoin-core/secp256k1/pull/1339#issuecomment-1587508040.
ACKs for top commit:
stratospher:
ACK 7067ee5.
real-or-random:
utACK 7067ee54b4
Tree-SHA512: f9bc2ab610099948ffac1e6bb3c822bd90b81a7110ab74cec03175e2c92ed27694a15f9cdaa7c4f1b460fe459f61c3d1d102c99592169f127fdd7539a1a0c154
The output ge is normalized when sizeof(secp256k1_ge_storage) = 64
but not when it's not 64. ARG_CHECK at the end of the function
assumes normalization. So normalize ge in the other code path too.
887183e7de scalar: use `secp256k1_{read,write}_be32` helpers (4x64 impl.) (Sebastian Falbesoner)
52b84238de scalar: use `secp256k1_{read,write}_be32` helpers (8x32 impl.) (Sebastian Falbesoner)
Pull request description:
This refactoring PR takes use of the `secp256k1_{read,write}_be32` helpers (introduced in PR #1093, commit 8d89b9e6e5) in the scalar <-> byte array conversion functions, for both the 8x32 and 4x64 implementations. (An alternative for the latter would be to introduce special helpers for reading/writing uint64_t in big endian `secp256k1_{read,write}_be64`).
Verified via `objdump -D libsecp256k1.a` that `secp256k1_scalar_set_b32` for 4x64 compiles to the same code on master and the PR (`secp256k1_scalar_get_b32` is apparently always inlined) on amd64 with clang 13.0.0.
ACKs for top commit:
sipa:
utACK 887183e7de
Tree-SHA512: 915cb4624c6da0530dce4ec3ac48e88dd735386302cd2e15759e3c30102d81186f382ffe71493ddd0538069f1b558db543d9bb900dfdb69acb60effedc33f705
f3644287b1 docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub` (Sebastian Falbesoner)
Pull request description:
ACKs for top commit:
real-or-random:
ACK f3644287b1 because it's consistent with the other docs
jonasnick:
ACK f3644287b1
Tree-SHA512: cc4db4637301335ea9d23ac43bb3a78de54af79a5262dba2013945f87d80670c7ae1e106101a59c04225eb077e9a9e0ecc9d9d3bfe2d11cdc90f098ebd479f49
From an API perspective, the functions `secp256k1_keypair_pub` and
`secp256k1_keypair_xonly_pub` always succeed (i.e. return the value 1),
so the other cases in the `pubkey` parameter descriptions never happen
and can hence be removed.
Note that the "1 always" return value description was previously done in
commit b8f8b99f0f (PR #1089), which also
explains why invalid inputs for the affected functions are in practice
only possible in violation of the type system.
db29bf220c ci: Remove quirk that runs dummy command after wineserver (Tim Ruffing)
c7db4942b3 ci: Fix error D8037 in `cl.exe` (Hennadii Stepanov)
7dae115861 Revert "ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe" (Hennadii Stepanov)
Pull request description:
Since the 2146cbfaf0, the `msvc-wine` effectively initializes the WINE prefix when running the `install.sh` script. See [`install.sh`#L143](2146cbfaf0/install.sh (L143)):
```sh
WINEDEBUG=-all wine64 wineboot &>/dev/null
```
Our following `wine64 wineboot --init` just messes up with the prefix.
This PR fixes this issue.
Also https://github.com/bitcoin-core/secp256k1/pull/1327 has been reverted as apparently it does not work. And https://github.com/bitcoin-core/secp256k1/pull/1320 has been combined into this one.
ACKs for top commit:
real-or-random:
ACK db29bf220c
Tree-SHA512: 59e61bde0060f67501f93da8b4e193f2bfcda85d849c16bb017e38af7aa9e3b569fe2fd4aa5cdb658c3b2345cc42fad98323e329b519389b2e881ecfd403d147
605e07e365 fix input range comment for `secp256k1_fe_add_int` (Sebastian Falbesoner)
Pull request description:
This seems to be a typo that was introduced with commit 4371f98346 (PR #1066).
ACKs for top commit:
sipa:
ACK 605e07e365
real-or-random:
ACK 605e07e365
Tree-SHA512: 7ee99cf7140c698d1146072734ba986de7328f78b2c076ee445067ef64a6a335c8669f1e733e10f5e14f98b566c799cc4c51b3eb0f036cd178b3c93476c6df2e
