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
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
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 3d2cf6c5bd 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.
aeece44599 gen_context: Don't use any ASM (Tim Ruffing)
Pull request description:
See https://github.com/bitcoin/bitcoin/issues/22441 , we need to wait for the testing results there.
ACKs for top commit:
sipa:
utACK aeece44599
jonasnick:
ACK aeece44599
Tree-SHA512: 52ff90f3dedda90124140de1c2c1c065a2f9374930d6b988d35c37f5eeae97f7d557b7ab0cf99d22add5a76ff8a3e06226572e43949e12d1048cb323d1b3d92b
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.
5f6ceafcfa schnorrsig: allow setting MSGLEN != 32 in benchmark (Jonas Nick)
fdd06b7967 schnorrsig: add tests for sign_custom and varlen msg verification (Jonas Nick)
d8d806aaf3 schnorrsig: add extra parameter struct for sign_custom (Jonas Nick)
a0c3fc177f schnorrsig: allow signing and verification of variable length msgs (Jonas Nick)
5a8e4991ad Add secp256k1_tagged_sha256 as defined in BIP-340 (Jonas Nick)
b6c0b72fb0 schnorrsig: remove noncefp args from sign; add sign_custom function (Jonas Nick)
442cee5baf schnorrsig: add algolen argument to nonce_function_hardened (Jonas Nick)
df3bfa12c3 schnorrsig: clarify result of calling nonce_function_bip340 without data (Jonas Nick)
99e8614812 README: mention schnorrsig module (Jonas Nick)
Pull request description:
This is a work in progress because I wanted to put this up for discussion before writing tests. It addresses the TODOs that didn't make it in the schnorrsig PR and changes the APIs of `schnorrsig_sign`, `schnorrsig_verify` and `hardened_nonce_function`.
- Ideally, the new `aux_rand32` argument for `sign` would be const, but didn't find a solution I was happy with.
- Support for variable length message signing and verification supports the [suggested BIP amendment](https://github.com/sipa/bips/issues/207#issuecomment-673681901) for such messages.
- ~~`sign_custom` with its opaque config object allows adding more arguments later without having to change the API again. Perhaps there are other sensible customization options, but I'm thinking of [sign-to-contract/covert-channel](https://github.com/bitcoin-core/secp256k1/pull/590) in particular. It would require adding the fields `unsigned char *s2c_data32` and `secp256k1_s2c_opening *s2c_opening` to the config struct. The former is the data to commit to and the latter is written to by `sign_custom`.~~ (EDIT: see below)
ACKs for top commit:
ariard:
utACK 5f6ceaf
LLFourn:
utACK 5f6ceafcfa
Tree-SHA512: cf1716dddf4f29bcacf542ed22622a817d0ec9c20d0592333cb7e6105902c77d819952e776b9407fae1333cbd03d63fded492d3a5df7769dcc5b450d91bb4761
a1ee83c654 tests_exhaustive: check the result of secp256k1_ecdsa_sign (Nicolas Iooss)
Pull request description:
Hello,
In `test_exhaustive_sign`, if `secp256k1_ecdsa_sign` fails, the signature which is then loaded by `secp256k1_ecdsa_signature_load` is garbage. Exit early with an error when this occurs.
By the way, I am wondering whether attribute `SECP256K1_WARN_UNUSED_RESULT` should be added to function `secp256k1_ecdsa_sign`: as (according to the documentation of this function) the nonce generation function may fail, it seems to be a good idea to force callers to check the value returned by this function. What do you think about this?
ACKs for top commit:
sipa:
ACK a1ee83c654
real-or-random:
utACK a1ee83c654
Tree-SHA512: d8c186afecbd95522e909c269255e8879695bf9df2de91f0f9303e575e18f03cafc66683d863e6cf9892fe61b668eab00d586861c39013292b71484a962f846d
If `secp256k1_ecdsa_sign` fails, the signature which is then loaded by
`secp256k1_ecdsa_signature_load` is garbage. Exit early with an error
when this occurs.
0302138f75 ci: Make compiler warning into errors on CI (Tim Ruffing)
b924e1e605 build: Ensure that configure's compile checks default to -O2 (Tim Ruffing)
7939cd571c build: List *CPPFLAGS before *CFLAGS like on the compiler command line (Tim Ruffing)
595e8a35d8 build: Enable -Wcast-align=strict warning (Tim Ruffing)
07256267ff build: Use own variable SECP_CFLAGS instead of touching user CFLAGS (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 0302138f75
Tree-SHA512: 619eb6b512ae0eb8c51134f5bb1b7bc7a397321dc51073ae3117f9433505ec19b407518b47a181163e1a841216b20487c7a50c6f5045faffa5cfa7fad0b8c906
Fixes one of the items in #923, namely the warnings of the form
'_putenv' redeclared without dllimport attribute:
previous dllimport ignored [-Wattributes]
This also cleans up the way we add CFLAGS, in particular flags enabling
warnings. Now we perform some more fine-grained checking for flag
support, which is not strictly necessary but the changes also help to
document autoconf.ac.
Function `test_inverse_scalar` contains:
(var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, x); /* l = 1/x */
The two sides of the condition are the same function. This seems to be
an error, as there also exists a non-var function, named
`secp256k1_scalar_inverse`.
Make `test_inverse_scalar` use this other function when `var` is false.
This issue was found using clang's static analyzer, which reported a
"Logic error: Identical expressions in conditional expression" (with
checker `alpha.core.IdenticalExpr`).
Varlen message support for the default sign function comes from recommending
tagged_sha256. sign_custom on the other hand gets the ability to directly sign
message of any length. This also implies signing and verification support for
the empty message (NULL) with msglen 0.
Tests for variable lengths follow in a later commit.
This makes the default sign function easier to use while allowing more granular
control through sign_custom.
Tests for sign_custom follow in a later commit.
9570f674cc Avoid passing out-of-bound pointers to 0-size memcpy (Pieter Wuille)
Pull request description:
Doing so could be considered UB in a pedantic interpretation of the standard. Avoid it.
Closes#876.
ACKs for top commit:
practicalswift:
cr ACK 9570f674cc729cafcba65f4cce03552d9a6108f4: patch looks correct
real-or-random:
ACK 9570f674cc
Tree-SHA512: f991462d72e39f14e609021b8427c2e6756009bc8cd21efca2da46ec9410250725a4fed662df20fcdcfd10a4dc59038f13e8c166362b2eadde4366586b9ca72b
Bitcoin Core's `configure` script uses `AC_CHECK_PROG` to find brew in the `PATH` [1]. If found, this macro will set `BREW=brew`. When building with dependencies however the `BREW` variable is set to `no` on macOS via `depends/<host_prefix>/share/config.site` [2] and this overrides `AC_CHECK_PROG` results [3]. Ideally, secp256k1's `configure` script should follow the same logic but this is not what happens because secp256k1's `configure` uses `AC_PATH_PROG` instead which respects preset variable values (in this case for variable `BREW`) only if they are a valid path (i.e., they match `[\\/*] | ?:[\\/]*` [4]), and `no` is not a path.
This commit changes `AC_PATH_PROG` to `AC_CHECK_PROG` to be consistent with Core's `AC_CHECK_PROG`. Both of these macros are supposed to find executables in the `PATH` but the difference is that former is supposed to return the full path whereas the latter is supposed to find only the program. As a result, the latter will accept even non-paths `no` as an override. Not knowing the full path is not an issue for the `configure` script because it will only execute `BREW` immediately afterwards, which works fine without the full path. (In particular, `PATH` cannot have changed in between [5].)
[1] https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L684
[2] https://github.com/bitcoin/bitcoin/blob/master/depends/config.site.in#L73-L76
[3] 6d38e9fa2b/lib/autoconf/programs.m4 (L47)
[4] 6d38e9fa2b/lib/autoconf/programs.m4 (L127)
[5] [3ab1178](3ab1178d54)
siv2r
Tim Ruffing
Jonas Nick
Elichai Turkel
Russell O'Connor
Nicolas Iooss
Pieter Wuille
UdjinM6