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.
aeece4459977b69962bcd1e1ee8845c18c74ff8f 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 aeece4459977b69962bcd1e1ee8845c18c74ff8f
jonasnick:
ACK aeece4459977b69962bcd1e1ee8845c18c74ff8f
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.
5f6ceafcfa46a69e901bed87e2c5f323b03b1e8c schnorrsig: allow setting MSGLEN != 32 in benchmark (Jonas Nick)
fdd06b7967196a3b34f73a5b19632637b4bde90a schnorrsig: add tests for sign_custom and varlen msg verification (Jonas Nick)
d8d806aaf386c7ead9431649f899ff82b0185aae schnorrsig: add extra parameter struct for sign_custom (Jonas Nick)
a0c3fc177f7f435e593962504182c3861c47d1be schnorrsig: allow signing and verification of variable length msgs (Jonas Nick)
5a8e4991ad443cc0cc613d80380a2db802a4cbce Add secp256k1_tagged_sha256 as defined in BIP-340 (Jonas Nick)
b6c0b72fb06e3c31121f1ef4403d2a229a31ec1c schnorrsig: remove noncefp args from sign; add sign_custom function (Jonas Nick)
442cee5bafbd7419acadf203ca11569e371f1f85 schnorrsig: add algolen argument to nonce_function_hardened (Jonas Nick)
df3bfa12c3b728241d3e61d13f8c976719a3de41 schnorrsig: clarify result of calling nonce_function_bip340 without data (Jonas Nick)
99e8614812bf23798a48c53649957e26e5b12f4a 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 5f6ceafcfa46a69e901bed87e2c5f323b03b1e8c
Tree-SHA512: cf1716dddf4f29bcacf542ed22622a817d0ec9c20d0592333cb7e6105902c77d819952e776b9407fae1333cbd03d63fded492d3a5df7769dcc5b450d91bb4761
a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6 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 a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
real-or-random:
utACK a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
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.
0302138f7508414e9e5212bc45b4ca4c0e5f081c ci: Make compiler warning into errors on CI (Tim Ruffing)
b924e1e605dcf9f9b362531184d16d643cc3baa9 build: Ensure that configure's compile checks default to -O2 (Tim Ruffing)
7939cd571c7a236f0d46e5cd7b6529ae29757c5a build: List *CPPFLAGS before *CFLAGS like on the compiler command line (Tim Ruffing)
595e8a35d80c932f91e810ce889c48b6efbaf890 build: Enable -Wcast-align=strict warning (Tim Ruffing)
07256267ffa9fb37609ec46260e9990bccd35dc5 build: Use own variable SECP_CFLAGS instead of touching user CFLAGS (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 0302138f7508414e9e5212bc45b4ca4c0e5f081c
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.
9570f674cc729cafcba65f4cce03552d9a6108f4 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 9570f674cc729cafcba65f4cce03552d9a6108f4
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)
8f879c2887e166da2ec959ce78078f7b84ebfdf9 Fix array size in bench_ecmult (Jonas Nick)
2fe1b50df16c9f41ea77b151634d734b930eeddd Add ecmult_gen, ecmult_const and ecmult to benchmark (Jonas Nick)
593e6bad9c5cda05dd72a5bd8266c4880113b4af Clean up ecmult_bench to make space for more benchmarks (Jonas Nick)
Pull request description:
I was trying to determine the impact of ecmult_gen in schnorrsig signing and noticed that there is no way to bench this right now. The new benchmarks look like this:
```
$ ./bench_ecmult
ecmult_gen: min 20.9us / avg 21.2us / max 21.7us
ecmult_const: min 63.9us / avg 64.3us / max 64.8us
ecmult 1: min 49.4us / avg 49.7us / max 50.3us
ecmult 1g: min 39.8us / avg 40.0us / max 40.3us
ecmult 2g: min 27.2us / avg 27.3us / max 27.8us
ecmult_multi 1g: min 39.8us / avg 40.0us / max 40.2us
ecmult_multi 2g: min 27.2us / avg 27.4us / max 27.7us
ecmult_multi 3g: min 22.8us / avg 22.9us / max 23.1us
ecmult_multi 4g: min 20.6us / avg 20.8us / max 21.1us
ecmult_multi 5g: min 19.3us / avg 19.5us / max 19.7us
```
(Turns out ecmult_gen is 37% of the 55.8us that schnorrsig sign takes)
ACKs for top commit:
real-or-random:
ACK 8f879c2887e166da2ec959ce78078f7b84ebfdf9
elichai:
tACK 8f879c2887e166da2ec959ce78078f7b84ebfdf9
Tree-SHA512: 8a739f5de1e2c0467c8d1c3ceeaf453b396a470ea0e8e5bef15fe1b32f3f9633b6b1c7e2ce1d94d736cf3e9adecd8f4f983ad4ba37450cd5991767f1a95db85c
a35fdd3478f7556dfb9b83f32aaa319ccadff9a9 ci: Run PRs on merge result even for i686 (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK a35fdd3478f7556dfb9b83f32aaa319ccadff9a9
Tree-SHA512: 9b800b1136da2ecdaff7fcffaac92d91623c682abed1fa5c2a1fe4384f20d2ff1079786f7216c39f58f5dd025e4ed32237e7aff29f7658a74554f0c298e9148e
This line should have been added in c7f754fe4d5e032fd150c4b9b985855e9fcaa521.
This mistake caused some i686 builds to fail when the PR was not
rebased, see https://cirrus-ci.com/build/5156197872435200.
