The existing code needs to deal with the edge case that bit_pos >= 256,
which would lead to an out-of-bounds read from secp256k1_scalar.
Instead, recode the scalar into an array of uint32_t with enough zero
padding at the end to alleviate the issue. This also simplifies the
code, and is necessary for a security improvement in a follow-up
commit.
Original code by Peter Dettman, with modifications by Pieter Wuille.
This introduces the signed-digit multi-comb multiplication algorithm
for constant-time G multiplications (ecmult_gen). It is based on
section 3.3 of "Fast and compact elliptic-curve cryptography" by
Mike Hamburg (see https://eprint.iacr.org/2012/309).
Original implementation by Peter Dettman, with changes by Pieter Wuille
to use scalars for recoding, and additional comments.
Instead of having the starting point of the ecmult_gen computation be
offset, do it with the final point. This enables reasoning over the
set of points reachable in intermediary computations, which can be
leveraged by potential future optimization.
Because the final point is in affine coordinates, its projective
blinding is no longer possible. It will be reintroduced again in
a different way, in a later commit.
Also introduce some more comments and more descriptive names.
f7f0184ba1 msan: notate more variable assignments from assembly code (Cory Fields)
a61339149f change inconsistent array param to pointer (Cory Fields)
Pull request description:
This was missed in 31ba404944 because older versions of clang did not complain about it. But clang-17, at least, does.
The array-as-a-param makes this annoying because `sizeof(l)` is not helpful. I'd be happy to change the size calculation if there are any better suggestions or strong preferences.
ACKs for top commit:
sipa:
utACK f7f0184ba1
real-or-random:
ACK f7f0184ba1 tests work fine with clang 17 and `./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang`
Tree-SHA512: 8ab22209ef322a10f500b123c82ae5e7141ae1da0e7a890cbf90bd7d2eb11f397db4ccfe15a1666f2f49228585cccbf5bec741effebd1e2c6012cb7ea1689675
a5e8ab2484 ci: Add sanitizer env variables to debug output (Tim Ruffing)
84a93de4d2 ci: Add workaround for ASLR bug in sanitizers (Tim Ruffing)
Pull request description:
Fixes#1506.
This also adds the sanitizer env variables to our debug output as suggested in the same issue.
ACKs for top commit:
sipa:
utACK a5e8ab2484
jonasnick:
ACK a5e8ab2484
Tree-SHA512: 5162d14eeec01e088c600ed77e21c5ffd4dec23327b7e81b5ecac59b7c535cac97cd7b7b744c767766036dfc6d9152a9933eb326cf4065d56c46e2ee858da662
3a9b1d46a3 New Experimental Module: Incremental Half-Aggregation for Schnorr Signatures (Benedikt)
Pull request description:
Revisited PR #130 by jonasnick.
I am happy to hear your thoughts.
**Summary of changes compared to #130:**
- Address comments from rustyrussell
- Use tagged hash
- Compute hashes with common prefix by copying midstate
- Allow Incremental Aggregation and make code consistent with the [draft spec](https://github.com/BlockstreamResearch/cross-input-aggregation/blob/master/half-aggregation.mediawiki)
ACKs for top commit:
real-or-random:
ACK 3a9b1d46a3
Tree-SHA512: 27239033f8b28ecf87ea310b3dd5a19dbbe6fd07495db71ef7017f8f444ec25a12897087d1bea0a2e9c3df77d7f17c38b183d7fe768858da2180f26624add4aa
"... neither can be equal to b." could suggest that the values are not
allowed to be identical, but what is meant here is that the mentioned
inputs shouldn't point to the same object.
Currently the `run_sqr` test doesn't do anything with the
result of the `fe_sqr` call. Improve that by checking that
the equation `(x+y)*(x-y) = x^2 - y^2` holds for some random
values y, as suggested in issue #1471 by real-or-random.
The existing loop for generating the x values is kept as-is.
31ba404944 msan: notate variable assignments from assembly code (Cory Fields)
e7ea32e30a msan: Add SECP256K1_CHECKMEM_MSAN_DEFINE which applies to memory sanitizer and not valgrind (Cory Fields)
Pull request description:
msan isn't smart enough to see that these are set without some help.
This was pointed out here: https://github.com/bitcoin-core/secp256k1/pull/1169#issuecomment-1370003449
With this commit, msan output is clean even with x86 asm turned on.
ACKs for top commit:
real-or-random:
utACK 31ba404944
hebasto:
re-ACK 31ba404944.
Tree-SHA512: c9c51fe542247e1e0a93f6d0063d119cf777ca8c1b7e9c8e45e168a2020dc503872eb2a78004725de81267a3ce78c923be1f8546fb92a3e95fc7ef034e5ba932
`check_fe_equal` is a wrapper around `secp256k1_fe_equal` that takes
care of normalization. Since it doesn't check anything itself, the
CHECK macro is needed at the call-sites to actually ensure equality.
The previous implementation returns an off-curve point for the input t=0.
This rewrite addresses that issue by implicity returning the on-curve point
(d, sqrt(1 + b)), which is the point that the paper Indifferentiable Hashing
to Barreto–Naehrig Curves suggests returning in this case.
Note: At the moment it is cryptographically impossible for the input t to be 0.
