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
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
b9ebee1490cc10286780c824a2bfac6bbb961cee fix a couple things to make Elements 22's linter happy (Andrew Poelstra)
Pull request description:
In Elements 22 the linter looks for executable files that don't have a properly-formed shebang. For some reason it wants `/usr/bin/env bash` rather than `/bin/bash`, and also one of our source files was erroneously 755.
ACKs for top commit:
real-or-random:
ACK b9ebee1490cc10286780c824a2bfac6bbb961cee
Tree-SHA512: 00da8fefd67c1882c6cec39dc81ce67ae3f52f902ddf72545e902b8f5bc7cd7c1249bf71027c530245c403a99c86ffbb61a89bc18c27c5ec975f6f653200766c
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.
72713872a8597884918bcf1edbc12f5c969ca680 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 72713872a8597884918bcf1edbc12f5c969ca680
jonasnick:
ACK 72713872a8597884918bcf1edbc12f5c969ca680
Tree-SHA512: 6107a2c84c3e11ffd68de22a5288d989a3c71c2ec1ee4827c88f6165fc27ef8339d0f6740928540e8ccd03aff49a2a96149bf698ccebe6d6d8ad6e23e38e8838
adec5a16383f1704d80d7c767b2a65d9221cee08 Add missing null check for ctx and input keys in the public API (Elichai Turkel)
f4edfc758142d6e100ca5d086126bf532b8a7020 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 adec5a16383f1704d80d7c767b2a65d9221cee08
Tree-SHA512: 6fe785776b7e451e9e8cae944987f927b1eb2e2d404dfcb1b0ceb0a30bda4ce16469708920269417e5ada09739723a430e270dea1868fe7d12ccd5699dde5976
This header contains a static array that replaces the ecmult_context pre_g and pre_g_128 tables.
The gen_ecmult_static_pre_g program generates this header file.
clang 7 to 11 (and maybe earlier versions) warn about recid being
potentially unitiliazed in "CHECK(recid >= 0 [...]", which was mitigated
in commit 3d2cf6c5bd35b0d72716b47bdd7e3892388aafc4 by initializing recid
to make clang happy but VG_UNDEF'ing the variable after initializiation
in order to ensure valgrind's memcheck analysis will still be sound and
complain if recid is not actually written to when creating a signature.
However, it turns out that at least for binaries produced by clang 11
(but not clang 7), valgrind complains about a branch on unitialized data
in the recid variable in that line before *and* after the aforementioned
commit. While the complaint after the commit could be spurious (clang
knows that recid is initialized, so it's fine to access it even though
the access is stupid), the complaint before the commit indicates a real
problem: it might be the case that clang is performing a wrong
optimization that leads to a situation where recid is really not
guaranteed to be initialized when it's accessed. As a result, clang
warns about this and generates code that just accesses the variable.
I'm not going to bother with this further because this is fixed in
clang 12 and the problem is just in our test code, not in the tested
code.
This commit rewrites the code in a way that groups the signing together
with the CHECK such that it's very easy to figure out for clang that
recid will be initialized properly. This seems to circument the issue.
8f093be374da794b835302bfb81a72e2bdd51d26 musig: use tagged hash for the list of pubkeys to aggregate (Jonas Nick)
a6a768a4bf3a243609e508c492307cb0fe754bda musig: make key agg test vector more precise (Jonas Nick)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 5369dc5b4039dd4cda2c50282db2882c088b96e1daa5801240f92be1832ed8f29317fdbfc3cab211707155c284a68dc593967f3141703e2544f6b8dc1553e44d
Also add a specific test that fails adaptor sig deserialization because with the
correct size buffer that's not guaranteed anymore with the existing test.
Previously the ECDSA signature had an overflowing s value, which after the sync
with upstream results in a failing VERIFY_CHECK in the inversion function.
However, normally parsed signatures shouldn't contain overflowing s values.
unsigned char foo[4] = "abcd" is not valid C++ because the string
literal "abcd" does not fit into foo due to the terminating NUL
character. This is valid in C, it will just omit the NUL character.
Fixes#962.
