592661c22f56736099f83700be8cf280f8a963ff ci: move test environment variable declaration to .cirrus.yml (siv2r)
dcbe84b84182bb077bc8639536a778a3129b1b3e bench: add --help option to bench. (siv2r)
Pull request description:
Fixes#1005
**Changes:**
- added `--help` option to `bench.c`
- `help()` function prints the help to command-line
- `have_invalid_args()` checks if the user has entered an invalid argument
- moved `secp256k1_bench_iters` and `secp256k1_test_iters` environment variables declaration to `.cirrus.yml`
ACKs for top commit:
sipa:
utACK 592661c22f56736099f83700be8cf280f8a963ff
real-or-random:
ACK 592661c22f56736099f83700be8cf280f8a963ff
Tree-SHA512: ebc6a2e6e47b529212efa1c9b75cc79649fca7f42aa75ce46502db24ac94f46b6cef59c828d13265d1fa69187a81c140d1951e7daeb7c8e008a6c1ad75259741
The following functions were created:
1. bench.c: help()
- prints the help to the command-line
2. bench.h: have_invalid_args()
- takes a list of arguments that the user is allowed to enter on the command-line
- returns 1 if the user entered an invalid argument
- returns 0 if all the user entered arguments are valid
2b7c7497ef66eae3a178b666fe17af40495322a6 build: replace backtick command substitution with $() (fanquake)
Pull request description:
This is only needed for the very oldest of non-POSIX-compatible shells.
Note that this code will also only be executed on macOS, where it'd be
very unlikely to run into such a shell anyways.
Followup to https://github.com/bitcoin-core/secp256k1/pull/1019#pullrequestreview-815300521. I had thought there were more usages of this
syntax, but seems like it's just the one.
See:
https://github.com/koalaman/shellcheck/wiki/SC2006
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
ACKs for top commit:
real-or-random:
ACK 2b7c7497ef66eae3a178b666fe17af40495322a6
hebasto:
ACK 2b7c7497ef66eae3a178b666fe17af40495322a6, verified that this is the only case.
Tree-SHA512: 6192f5efe437ff428ce7843ac595049a1aa7969a9e696f649cfd4820b28fc96ad0fabd6eec0ec1ca404763f02e64af6a99e57666a00d8749c6212a0646211991
This is only needed for the very oldest of non-POSIX-compatible shells.
Note that this code will also only be executed on macOS, where it'd be
very unlikely to run into such a shell.
Followup to #1019.
See:
https://github.com/koalaman/shellcheck/wiki/SC2006
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
60bf8890df5360148df921f26d8dc4d667dd5926 ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS (Jonas Nick)
Pull request description:
This bug was introduced in 7506e064d791e529d2e57bb52c156deb33b897ef by adding
an allocation but not updating the constant.
ACKs for top commit:
robot-dreams:
ACK 60bf8890df5360148df921f26d8dc4d667dd5926
real-or-random:
ACK 60bf8890df5360148df921f26d8dc4d667dd5926
Tree-SHA512: d7782fe9bf09fea8cf22304ab13679223a48f4d8b09081e662ea162a68c4e35f6b5820fbe4c6030fabad02a48dfdd02eb9eef22262c1dbbf02955bb92b75aef8
214042a170c860523b7aad2a251b0dbfbaefb235 build: don't append valgrind CPPFLAGS if not installed (fanquake)
Pull request description:
Valgrinds CPPFLAGS, i.e `-I/usr/local/opt/valgrind/include`, are currently added to CPPFLAGS, regardless of whether valgrind is installed. This changes configure so that they are only added if valgrind is available. i.e the output of `brew list --versions valgrind` is non-null.
ACKs for top commit:
real-or-random:
ACK 214042a170c860523b7aad2a251b0dbfbaefb235
hebasto:
ACK 214042a170c860523b7aad2a251b0dbfbaefb235, tested on macOS Big Sur 11.6.1 (20G224, Intel).
Tree-SHA512: 5101636a0a12f1941b01967ca8eab7aa20f44db0d1ef4571a5ad6026bb89494b983465d34d93c8b17a260b695116792991da53d135bc19a3c9e974f5266a90af
812ff5c74745e451f1a9de83b5bd0d0c18c75e5f doc: remove use of 0xa0 "no break space" (fanquake)
Pull request description:
This is miscellaneous, but I don't think these were being used on purpose?
ACKs for top commit:
siv2r:
ACK 812ff5c. The non-breaking space character is replaced with whitespace. Tested with [NBSP highlighter extension](https://marketplace.visualstudio.com/items?itemName=viktorzetterstrom.non-breaking-space-highlighter) on vscode.
real-or-random:
ACK 812ff5c74745e451f1a9de83b5bd0d0c18c75e5f
Tree-SHA512: ccfcc64798f5a5eb0c669eb00f4408ab713e6710d67fd15ee2a4dca0d052e27636d7f0ad312aa94be0cd068c7e7874441aa2e114c4118322d0c764398a4ff695
72de1359e953390dc2f1ab5a59dd1a4057000acb ci: Enable -g if we set CFLAGS manually (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 72de1359e953390dc2f1ab5a59dd1a4057000acb
Tree-SHA512: 0456db0ba53410640653e5d11ee4f328be0657e1e9077aa982ed4fd3eb6e326cfc022ec7ab71fc5c62d7942a20bbc7a5e8000cf5b62201fa1c183853d899ea77
16d132215cab68e57798927301268518bc1c3bf8 refactor: Use (int)&(int) in boolean context to avoid compiler warning (MarcoFalke)
Pull request description:
This one should *really* be only a refactor with the goal to silence static analysis warnings. clang-14 (trunk) recently added one in commit f62d18ff14 and I expect other tools will offer similar warnings.
Follow up to #1006, which was not a refactor.
ACKs for top commit:
real-or-random:
ACK 16d132215cab68e57798927301268518bc1c3bf8
jonasnick:
ACK 16d132215cab68e57798927301268518bc1c3bf8
Tree-SHA512: c465522ea4ddb58b5974c95bc36423c453e6fcf5948cb32114172113b5244209ceaa9418ec86ebe210390ae5509c2f24a42c41a7353de4cfb8fd063b0d5c0e79
This fixes a compiler warning:
./src/ecdsa_impl.h:312:12: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
return !secp256k1_scalar_is_zero(sigr) & !secp256k1_scalar_is_zero(sigs);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3b157c48edb8ab080683232125dc7ec058bbd24c doc: Suggest keys.openpgp.org as keyserver in SECURITY.md (Tim Ruffing)
73a7472cd0335f2aa7eaf2c514e909ce36aba291 doc: Replace apoelstra's GPG key by jonasnick's GPG key (Tim Ruffing)
Pull request description:
I have verified the new key via other secure channels.
This closes#1003 .
We can skip the second commit but I expect https://github.com/bitcoin/bitcoin/pull/23466/ to be merged. If it won't be merged, we could still revert.
ACKs for top commit:
sipa:
ACK 3b157c48edb8ab080683232125dc7ec058bbd24c. I've also verified the key out of band.
jonasnick:
ACK 3b157c48edb8ab080683232125dc7ec058bbd24c
Tree-SHA512: 496f98121f14031bc693aa83bf208b253f79b700b4bca0b629deadc8852f76ef6d69ad90109baa771d7b9f6e4b983e4ed8dca404cf5aceffe9d520d3362b533a
af6abcb3d0097a7f7892fb8b54a4c6363e5c2c7f Make bench support selecting which benchmarks to run (Pieter Wuille)
9f56bdf5b9ba2e22e77c6adaaeb8302398732df3 Merge bench_schnorrsig into bench (Pieter Wuille)
3208557ae1062f7fcce25d5f2c5a29dc34a18895 Merge bench_recover into bench (Pieter Wuille)
855e18d8a809b98a622ab55765792aca132ea640 Merge bench_ecdh into bench (Pieter Wuille)
2a7be678a660d58742b1e767444c0fe75fa22592 Combine bench_sign and bench_verify into single bench (Pieter Wuille)
Pull request description:
This combines `bench_verify`, `bench_sign`, `bench_ecdh`, `bench_recovery`, and `bench_schnorrsig` into a single `bench` binary.
I don't think there is a good reason to have this many binaries, and it complicates build config and CI.
ACKs for top commit:
real-or-random:
ACK af6abcb3d0097a7f7892fb8b54a4c6363e5c2c7f diff looks good, command line options work, valgrind is happy
siv2r:
tACK af6abcb, the command-line options work as expected on my ubuntu machine. The diff looks good.
Tree-SHA512: 39c099b508c70136eaac8a429591b4250a8c22e423fa922d83928ea9273d8f2c1136317427563b28f249c02cf83d1c73ea787c6d26aa88545236241641965705
5324f8942dd322448fae6c9b225ecac2854fa7e2 Make aux_rnd32==NULL behave identical to 0x0000..00. (Pieter Wuille)
Pull request description:
BIP340's default signing algorithm always requires an aux_rnd argument, but permits using an all-zero one when no randomness is available.
Make secp256k1_schnorrsig_sign follow this even when aux_rnd32==NULL, by treating the same as if an all-zero byte array was provided as input.
ACKs for top commit:
junderw:
ACK 5324f89
elichai:
ACK 5324f8942dd322448fae6c9b225ecac2854fa7e2
jonasnick:
ACK 5324f8942dd322448fae6c9b225ecac2854fa7e2
real-or-random:
utACK 5324f8942dd322448fae6c9b225ecac2854fa7e2
Tree-SHA512: caa1d5a0eacea3239d8aaace5284eedcd850058bbe759768e626233a010199db6c637618aedccfb51fe94ec8d28f45bc0c441be77e2e12fa2a393b9cc3a5d3ae
BIP340's default signing algorithm always requires an aux_rnd argument,
but permits using an all-zero one when no randomness is available.
Make secp256k1_schnorrsig_sign follow this even when aux_rnd32==NULL,
by treating the same as if an all-zero byte array was provided as
input.
2888640132eb64ed30a8a208931f27447c3e0366 VERIFY_CHECK precondition for secp256k1_fe_set_int. (Russell O'Connor)
d49011f54c2b31807158bdf06364f331558cccc7 Make _set_fe_int( . , 0 ) set magnitude to 0 (Tim Ruffing)
Pull request description:
Also set the magnitude to 0 when setting the value to 0.
ACKs for top commit:
real-or-random:
ACK 2888640132eb64ed30a8a208931f27447c3e0366
jonasnick:
ACK 2888640132eb64ed30a8a208931f27447c3e0366
Tree-SHA512: 6ec9b3485380503b11c00f30bfa79f92ba3facb93ee4f3df582b881c4e19fb8ae8b5acd5aeb6326497c290cd0904230d0356f33bd136ca577d2f25616279e090
23e2f66726f930ac01d5075106aa16a4073442b4 bench: don't return 1 in have_flag() if argc = 1 (Jonas Nick)
96b1ad2ea9f9d9419e566b95162487c48902c3eb bench_ecmult: improve clarity of output (Jonas Nick)
Pull request description:
Previously "ecmult{,_multi} xg" meant multiplication with (x - 1) random points
and base point G. Now
- xP means multiplication with x random points and
- xP & G means multiplication with x random points and G
ACKs for top commit:
siv2r:
tACK 23e2f66
real-or-random:
ACK 23e2f66726f930ac01d5075106aa16a4073442b4
Tree-SHA512: 0218aaa0baa4c2f92a7b98c97b8cc3b596e3da44d7f38ab4bdd707a4bdb96bb071b953fc6106cd34977a562278e4eaa860a3a7fa64c323c5117945e7a3107162
This makes the semantic of have_flag more clear and fixes a bug
that was introduced in
2fe1b50df16c9f41ea77b151634d734b930eeddd
Add ecmult_gen, ecmult_const and ecmult to benchmark
where the behavior introduced by this commit was already assumed. If
bench_ecmult was called without arguments, have_flag("simple") returned 1 and no
scratch space was allocated which led to very wrong output.
Previously "ecmult{,_multi} xg" meant multiplication with (x - 1) random points
and base point G. Now
- ecmult_{,multi_}xp means multiplication with x random points and
- ecmult_{,multi_}xp_g means multiplication with x random points and G
b4b130678db31a7cabc2cde091bc4acbca92b7a3 create csv file from the benchmark output (siv2r)
26a255beb673217c839dcc51790d9a484f9a292d Shared benchmark format for command line and CSV outputs (siv2r)
Pull request description:
ACKs for top commit:
real-or-random:
ACK b4b130678db31a7cabc2cde091bc4acbca92b7a3
jonasnick:
ACK b4b130678db31a7cabc2cde091bc4acbca92b7a3
Tree-SHA512: 1eebbdd7701ad21d9647434ff05f23827be217d47870bb05a2fdb12447abc365fc6e56306f344e05d8d2ec1ff5532562131b3876261733e4412117357c5c65f8
044d95630556dda5492a70af056bc277f0b79ebc Fix G.y parity in sage code (Pieter Wuille)
Pull request description:
I'm not sure if `EllipticCurve.lift_x` has well-defined Y coordinate or not, but at least my current version of Sage computes the wrong G. Fix this.
ACKs for top commit:
real-or-random:
ACK 044d95630556dda5492a70af056bc277f0b79ebc
Tree-SHA512: afb919af29027da2bb3c58628924f9740672d3c347ad39cc663c9c399b1aa8536256fd3fd4e1e54457e38344704d47f281d82488da413f4e6e67e191decc960f
1. add `print_output_table_header_row` func to print the table header for benchmark output
2. modify the following benchmarks to include the table header
- bench_ecdh.c
- bench_ecmult.c
- bench_internal.c
- bench_recover.c
- bench_schnorrsig.c
- bench_sign.c
- bench_verify.c
b53e0cd61fce0bcef178f317537c91efc9afd04d Avoid overly-wide multiplications (Peter Dettman)
Pull request description:
Speeds up bench_ecdh, bench_sign, bench_verify relative to master by 5+% at -O3, haswell.
ACKs for top commit:
sipa:
ACK b53e0cd61fce0bcef178f317537c91efc9afd04d
real-or-random:
ACK b53e0cd61fce0bcef178f317537c91efc9afd04d I've inspected the diff and run the tests without asm for a CPU day
Tree-SHA512: 4f79c98371a3dc9da013632210c8db979f910b222291999dfaa0c31849a77eb427361e4ab9206cbfee73c30a8933178784d6cb8e747e8dca6b227eb77fbea2a2
9be7b0f08340a063d961547b5d2663405f3fc162 Avoid computing out-of-bounds pointer. (Tim Ruffing)
Pull request description:
This is a pedantic case of UB.
Spotted in #879.
ACKs for top commit:
elichai:
ACK 9be7b0f08340a063d961547b5d2663405f3fc162
practicalswift:
cr ACK 9be7b0f08340a063d961547b5d2663405f3fc162
sipa:
ACK 9be7b0f08340a063d961547b5d2663405f3fc162
Tree-SHA512: a9d028c4cdb37ad0d5fcf0d2f678eef732a653d37155a69a20272c6b283c28e083172485d7a37dc4a7c6100b22a6f5b6a92e729239031be228cc511842ee35e8
bc08599e776aff33c834ef829843ec5f629d1f39 Remove OpenSSL testing support (Pieter Wuille)
Pull request description:
This removes the ability to test against OpenSSL, as well as the OpenSSL verification benchmark.
The motivation is that OpenSSL 3 is deprecating part of the API used here (see #869), and I'm not sure it's worth maintaining. We do lose the fact that this is the only test that verifies randomly-generated cases against an independent implementation. On the other hand, there are tons of existing fixed tests now that test all kinds of edge cases already.
ACKs for top commit:
elichai:
tACK bc08599
real-or-random:
ACK bc08599e776aff33c834ef829843ec5f629d1f39
jonasnick:
ACK bc08599e776aff33c834ef829843ec5f629d1f39
Tree-SHA512: 632e6d3cf7bbc5828f5ca1f0f2a92c80bcb681bbcd4320c352b4a86fd521e410c852ccebcfc30fadc8fbf86649267a9e521f53e0f78072a8cd74d8726da28973
189f6bcfef6578b89e21f937b24060f74bd18f00 Fix unused parameter warnings when building without VERIFY (Jonas Nick)
Pull request description:
This commit makes `./configure --enable-coverage && make check` free of warnings.
ACKs for top commit:
practicalswift:
cr ACK 189f6bcfef6578b89e21f937b24060f74bd18f00
elichai:
utACK 189f6bcfef6578b89e21f937b24060f74bd18f00
siv2r:
Tested ACK 189f6bc
Tree-SHA512: 727fe0e40ff61f404780b32dfa4102a58bed9d922e61bd17ddaaf1243b0c06edd9697ff4763b5e92d033e7db3778193bee07d85cfa3b9c46d45e5fec3f568009
d43993724deb5fdc1d2162f7423f8e8398103dd5 tests: remove `secp256k1_fe_verify` from tests.c and modify `secp256k1_fe_from_storage` to call `secp256k1_fe_verify` (siv2r)
Pull request description:
ACKs for top commit:
roconnor-blockstream:
utACK d439937 diff looks correct, I also didn't run the tests locally.
real-or-random:
utACK d43993724deb5fdc1d2162f7423f8e8398103dd5 diff looks correct, I didn't run the tests locally
jonasnick:
ACK d43993724deb5fdc1d2162f7423f8e8398103dd5 ran tests with `--enable-coverage`
Tree-SHA512: c3c9ecf8e9b7dfdcd1144ddcf8bcc637996c699dbd0fc6223e6186d082908728468fa276b09c6f344e036ca05f54432dde6366a83eb39f915a334164faadd556
1. secp256k1_fe_verify is removed from tests since, it throws an error if VERIFY is not defined during compilation.
(Ex: ./configure --enable-coverage)
2. `secp256k1_fe_from_storage` calls `secp256k1_fe_verify` in the VERIFY build to check for invalid field element.
