60bf8890df ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS (Jonas Nick)
Pull request description:
This bug was introduced in 7506e064d7 by adding
an allocation but not updating the constant.
ACKs for top commit:
robot-dreams:
ACK 60bf8890df
real-or-random:
ACK 60bf8890df
Tree-SHA512: d7782fe9bf09fea8cf22304ab13679223a48f4d8b09081e662ea162a68c4e35f6b5820fbe4c6030fabad02a48dfdd02eb9eef22262c1dbbf02955bb92b75aef8
214042a170 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 214042a170
hebasto:
ACK 214042a170, tested on macOS Big Sur 11.6.1 (20G224, Intel).
Tree-SHA512: 5101636a0a12f1941b01967ca8eab7aa20f44db0d1ef4571a5ad6026bb89494b983465d34d93c8b17a260b695116792991da53d135bc19a3c9e974f5266a90af
812ff5c747 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 812ff5c747
Tree-SHA512: ccfcc64798f5a5eb0c669eb00f4408ab713e6710d67fd15ee2a4dca0d052e27636d7f0ad312aa94be0cd068c7e7874441aa2e114c4118322d0c764398a4ff695
72de1359e9 ci: Enable -g if we set CFLAGS manually (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 72de1359e9
Tree-SHA512: 0456db0ba53410640653e5d11ee4f328be0657e1e9077aa982ed4fd3eb6e326cfc022ec7ab71fc5c62d7942a20bbc7a5e8000cf5b62201fa1c183853d899ea77
16d132215c 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 16d132215c
jonasnick:
ACK 16d132215c
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);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3b157c48ed doc: Suggest keys.openpgp.org as keyserver in SECURITY.md (Tim Ruffing)
73a7472cd0 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 3b157c48ed. I've also verified the key out of band.
jonasnick:
ACK 3b157c48ed
Tree-SHA512: 496f98121f14031bc693aa83bf208b253f79b700b4bca0b629deadc8852f76ef6d69ad90109baa771d7b9f6e4b983e4ed8dca404cf5aceffe9d520d3362b533a
af6abcb3d0 Make bench support selecting which benchmarks to run (Pieter Wuille)
9f56bdf5b9 Merge bench_schnorrsig into bench (Pieter Wuille)
3208557ae1 Merge bench_recover into bench (Pieter Wuille)
855e18d8a8 Merge bench_ecdh into bench (Pieter Wuille)
2a7be678a6 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 af6abcb3d0 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
5324f8942d 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 5324f8942d
jonasnick:
ACK 5324f8942d
real-or-random:
utACK 5324f8942d
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.
2888640132 VERIFY_CHECK precondition for secp256k1_fe_set_int. (Russell O'Connor)
d49011f54c 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 2888640132
jonasnick:
ACK 2888640132
Tree-SHA512: 6ec9b3485380503b11c00f30bfa79f92ba3facb93ee4f3df582b881c4e19fb8ae8b5acd5aeb6326497c290cd0904230d0356f33bd136ca577d2f25616279e090
23e2f66726 bench: don't return 1 in have_flag() if argc = 1 (Jonas Nick)
96b1ad2ea9 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 23e2f66726
Tree-SHA512: 0218aaa0baa4c2f92a7b98c97b8cc3b596e3da44d7f38ab4bdd707a4bdb96bb071b953fc6106cd34977a562278e4eaa860a3a7fa64c323c5117945e7a3107162
This makes the semantic of have_flag more clear and fixes a bug
that was introduced in
2fe1b50df1
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
b4b130678d create csv file from the benchmark output (siv2r)
26a255beb6 Shared benchmark format for command line and CSV outputs (siv2r)
Pull request description:
ACKs for top commit:
real-or-random:
ACK b4b130678d
jonasnick:
ACK b4b130678d
Tree-SHA512: 1eebbdd7701ad21d9647434ff05f23827be217d47870bb05a2fdb12447abc365fc6e56306f344e05d8d2ec1ff5532562131b3876261733e4412117357c5c65f8
044d956305 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 044d956305
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
b53e0cd61f 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 b53e0cd61f
real-or-random:
ACK b53e0cd61f I've inspected the diff and run the tests without asm for a CPU day
Tree-SHA512: 4f79c98371a3dc9da013632210c8db979f910b222291999dfaa0c31849a77eb427361e4ab9206cbfee73c30a8933178784d6cb8e747e8dca6b227eb77fbea2a2
9be7b0f083 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 9be7b0f083
practicalswift:
cr ACK 9be7b0f083
sipa:
ACK 9be7b0f083
Tree-SHA512: a9d028c4cdb37ad0d5fcf0d2f678eef732a653d37155a69a20272c6b283c28e083172485d7a37dc4a7c6100b22a6f5b6a92e729239031be228cc511842ee35e8
bc08599e77 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 bc08599e77
jonasnick:
ACK bc08599e77
Tree-SHA512: 632e6d3cf7bbc5828f5ca1f0f2a92c80bcb681bbcd4320c352b4a86fd521e410c852ccebcfc30fadc8fbf86649267a9e521f53e0f78072a8cd74d8726da28973
189f6bcfef 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 189f6bcfef
elichai:
utACK 189f6bcfef
siv2r:
Tested ACK 189f6bc
Tree-SHA512: 727fe0e40ff61f404780b32dfa4102a58bed9d922e61bd17ddaaf1243b0c06edd9697ff4763b5e92d033e7db3778193bee07d85cfa3b9c46d45e5fec3f568009
d43993724d 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 d43993724d diff looks correct, I didn't run the tests locally
jonasnick:
ACK d43993724d 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.
72713872a8 Add missing static to secp256k1_schnorrsig_sign_internal (Elichai Turkel)
Pull request description:
This function isn't used outside of this module so it should be declared static
ACKs for top commit:
real-or-random:
ACK 72713872a8
jonasnick:
ACK 72713872a8
Tree-SHA512: 6107a2c84c3e11ffd68de22a5288d989a3c71c2ec1ee4827c88f6165fc27ef8339d0f6740928540e8ccd03aff49a2a96149bf698ccebe6d6d8ad6e23e38e8838
adec5a1638 Add missing null check for ctx and input keys in the public API (Elichai Turkel)
f4edfc7581 Improve consistency for NULL arguments in the public interface (Elichai Turkel)
Pull request description:
I went over the public API and added missing explanations on when a pointer can be null and when it cannot,
and added some missing checks for null ctx and null pubkey pointers.
Open questions IMHO:
1. Can `secp256k1_context_create` return NULL? right now it could return null if you replaced the callbacks at compile time to ones that do return(unlike the default ones which never return).
2. Related to the first, should we document that the callbacks should never return? (in the tests we use returning callbacks but we can violate our own API) right now we say the following:
> After this callback returns, anything may happen, including crashing.
Is this enough to document answer `no` for the first question and just saying that if the callback returned then you violated the API so `secp256k1_context_create` can return NULL even though it is promised not to?
Right now we AFAICT we never check if it returns null
Another nit I'm not sure about is wording `(does nothing if NULL)`/`(ignored if NULL)`/`(can be NULL)`
More missing docs:
1. Documenting the `data` argument to the default nonce functions
ACKs for top commit:
ariard:
ACK adec5a16
jonasnick:
ACK adec5a1638
Tree-SHA512: 6fe785776b7e451e9e8cae944987f927b1eb2e2d404dfcb1b0ceb0a30bda4ce16469708920269417e5ada09739723a430e270dea1868fe7d12ccd5699dde5976
Hennadii Stepanov
Tim Ruffing
Jonas Nick
fanquake
Elliott Jin
Dimitris Apostolou
MarcoFalke
Pieter Wuille
siv2r
Russell O'Connor
Elichai Turkel