dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 variable signing precompute table (djb)
Pull request description:
This pull request gives an option to reduce the precomputed table size for the signing context (`ctx`) by setting `#define ECMULT_GEN_PREC_BITS [N_BITS]`.
Motivation: Per #251 and #254, the static table can be reduced to 64kB. However, this is still too big for some of my embedded applications. Setting `#define ECMULT_GEN_PREC_BITS 2` produces a 32kB table at a tradeoff of about 75% of the signing speed. Not defining this value will default to the existing implementation of 4 bits. Statistics:
```
ECMULT_GEN_PREC_BITS = 1
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 195us / avg 200us / max 212us
ECMULT_GEN_PREC_BITS = 2
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 119us / avg 126us / max 134us
ECMULT_GEN_PREC_BITS = 4 (default)
Precomputed table size: 64kB
./bench_sign
ecdsa_sign: min 83.5us / avg 89.6us / max 95.3us
ECMULT_GEN_PREC_BITS = 8
Precomputed table size: 512kB
./bench_sign
ecdsa_sign: min 96.4us / avg 99.4us / max 104us
```
Only values of 2 and 4 make sense. 8 bits causes a larger table size with no increase in speed. 1 bit runs, actually, but does not reduce table size and is slower than 2 bits.
ACKs for top commit:
real-or-random:
ACK dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 verified that all changes to the previous ACKed 1d26b27ac90092306bfbc9cdd5123e8a5035202a were due to the rebase
jonasnick:
ACK dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 read the code and tested various configurations with valgrind
Tree-SHA512: ed6f68ca23ffdc4b59d51525336b34b25521233537edbc74d32dfb3eafd8196419be17f01cbf10bd8d87ce745ce143085abc6034727f742163f7e5f13f26f56e
make ECMULT_GEN_PREC_BITS configurable
ecmult_static_context.h: add compile time config assertion (#3) - Prevents accidentally using a file which was generated with a
different configuration.
README: mention valgrind issue
With --with-ecmult-gen-precision=8, valgrind needs a max stack size
adjustment to not run into a stack switching heuristic:
http://valgrind.org/docs/manual/manual-core.html
> -max-stackframe= [default: 2000000]
> The maximum size of a stack frame. If the stack pointer moves by more than this amount then Valgrind will assume that the program is switching to a different stack.
You may need to use this option if your program has large stack-allocated arrays.
basic-config: undef ECMULT_WINDOW_SIZE before (re-)defining it
a467047e110fb55186df173afa3d5f330f6fa47c Make ./configure string consistent (Tim Ruffing)
Pull request description:
This was forgotten in some PR rebase.
ACKs for top commit:
jonasnick:
ACK a467047e110fb55186df173afa3d5f330f6fa47c
Tree-SHA512: 5aa67e886c165afa97a1e34ccfbd6bb0158ba4d4e5a4aacf6ac8b17ad9ee55132061957fd5ec383a79ad72ec7c92c745d7ad4fddca743b53e4b0e635616b29dc
b64a2e2597b66b57e23f3cb34f9c88809e34d93f Fix a nit in the recovery tests (Elichai Turkel)
Pull request description:
this signature is only valid under recid 1 not 0.
Source: https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/tests_impl.h#L247
(it passes only when the sig is parsed with recid 1)
ACKs for top commit:
real-or-random:
ACK b64a2e2597b66b57e23f3cb34f9c88809e34d93f I only looked at the diff
jonasnick:
ACK b64a2e2597b66b57e23f3cb34f9c88809e34d93f read the code
Tree-SHA512: 8e6744fe87c4078181dd1b334641784bf4fee37eb87346ecf8149482a9ea2c321bbe068e6a9199d836430b54b73848d94473a9aa6b59b4a68921a6321f449736
a11c76c59a431e3492994f71a968a838e398fb58 secp256k1/src/tests.c: Properly handle sscanf return value (Mustapha Abiola)
Pull request description:
This pull request fixes a bug which allows the `sh` variable to be used uninitialised
when sscanf(3) returns EOF.
Signed-off-by: Mustapha Abiola <mustapha@trilemma.net>
ACKs for top commit:
sipa:
ACK a11c76c59a431e3492994f71a968a838e398fb58.
practicalswift:
utACK a11c76c59a431e3492994f71a968a838e398fb58
real-or-random:
ACK a11c76c59a431e3492994f71a968a838e398fb58 I looked at the code
Tree-SHA512: fd9660a18e39ecf9366db94ccbcec2682b020223f4f982a4356ddf56c2fbdafa5edcd830db37be12b661c1ec0b15c57b9f34ba59ef4460187c9c2478376fbc88
74e2dbd JNI: fix use sig array (liuyujun)
Pull request description:
ACKs for commit 74e2db:
sipa:
ACK 74e2dbd68e07f752ac326a578e3071f9efa55e55. This is clearly an improvement.
real-or-random:
ACK 74e2dbd68e07f752ac326a578e3071f9efa55e55 I've read the code but haven't tested it
Tree-SHA512: 850b32e893463be4be28185dcc127d429afe4b6076036a078b7c61d590e0f4ea89127e448760b71c087cf70ffbefc52d87db77a5131bee81f3e4f95cfbd3bd3e
94ae7cb Moved a dereference so the null check will be before the dereferencing (Elichai Turkel)
Pull request description:
Before that even on debug the compiler could've assumed `a` isn't null and optimized `VERIFY_CHECK(a != NULL);` out.
This put the dereference after the check
Resolves#643
ACKs for commit 94ae7c:
sipa:
ACK 94ae7cbf83a34456e5cad721f61ea77fcc023a3f
Tree-SHA512: 8b986f202ede5bde1f14a8ecf25e339d64ee6cd5cb391c5f18b4ff58f946c3845902d1230bc80d110a0a33b37025d281bd4532afbdf03b1c9ca321097374eb8e
2cb73b1 scalar_impl.h: fix includes (Marko Bencun)
Pull request description:
group.h functions are not referenced.
utils.h added as functions like VERIFY_CHECK are used.
ACKs for commit 2cb73b:
sipa:
ACK 2cb73b1064c796f5902189e0850066299e87aa93
Tree-SHA512: b9c7367061c2a22d2c9266c61261edd47798551b03b878ecd2e005d858701487145589793406cb4e88e85cd3c769007132efac9c228d5ee288e487e7d308e1c2
2abcf95 jni: Use only Guava for hex encoding and decoding (Tim Ruffing)
Pull request description:
This removes a dependency on javax.xml.bind, which is no longer
available in JDK >= 11, see
https://openjdk.java.net/jeps/320#Java-EE-modules .
ACKs for commit 2abcf9:
sipa:
ACK 2abcf951af6a9e8aff7398eb9588a50339b720c7, tests pass.
Tree-SHA512: bae4d1285b4a4a0ad62323c25eabcad5f800ddb2d97f2e15085b39982e29248b21e2e8de0d4c07a33a64f071dcdba653f72415558c0f8b619227bc6f6d71eda3
This pull request fixes a bug which allows the `sh` variable to be used uninitialized when sscanf returns EOF.
Signed-off-by: Mustapha Abiola <mustapha@trilemma.net>
6914c25 typo in comment for secp256k1_ec_pubkey_tweak_mul () (philsmd)
Pull request description:
Fixes a typo in secp256k1.h documentation
ACKs for commit 6914c2:
Tree-SHA512: 9b95209b7decab4624054b5e3476e99468f84f84eb270bba997abf73a78acbbf2eaa094dfa367ebfe0b1e553329071e9a0ca8a1e2b31ea7fbc4aad3fb0665e88
cd473e0 Avoid calling secp256k1_*_is_zero when secp256k1_*_set_b32 fails. (Gregory Maxwell)
Pull request description:
Most of the codebase correctly used short-cutting to avoid calling
_is_zero on possibly incompletely initialized elements, but a few
places were missed.
ACKs for commit cd473e:
sipa:
utACK cd473e02c372217c3a6608ce5afaa543ed78f891
jonasnick:
utACK cd473e02c372217c3a6608ce5afaa543ed78f891
Tree-SHA512: d6af2863f6795d2df26f2bd05a4e33085e88c45f7794601ea57e67238a2073ef1ee3ba0feab62a7fcbc0636c48dfd80eea07d0ca4f194414127f914b0478c732
dcf3920 Fix ability to compile tests without -DVERIFY. (Gregory Maxwell)
Pull request description:
Broken by 3f3964e4.
It's important that the tests are also run without -DVERIFY due to
the possibility that side-effects of a VERIFY_CHECK fix a bug that
would otherwise be detected.
Use of the verify_check macro in tests isn't sufficient.
ACKs for commit dcf392:
Tree-SHA512: ff7ca0e89e33f845656a4d7d18c0195d1378b020d67f89e900b18cf3d702aa81dd91ffd05a98953a481b83e4247eaf0c484bea12eab020efb3c966a456e8129f
14c7dbd Simplify control flow in DER parsing (Tim Ruffing)
ec8f20b Avoid out-of-bound pointers and integer overflows in size comparisons (Tim Ruffing)
01ee1b3 Parse DER-enconded length into a size_t instead of an int (Tim Ruffing)
3cb057f Fix possible integer overflow in DER parsing (Tim Ruffing)
Pull request description:
This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed.
I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code.
Best reviewed in individual commits.
ACKs for commit 14c7db:
Tree-SHA512: 312dd3f961739752e1a861e75bd755920f634f87ee9668793e102c224434e8d21367452e114de729322c71a89f4fa82126aa5d32742f2bbbc091777c99515e10
e49f799 Add missing #(un)defines to base-config.h (Tim Ruffing)
77defd2 Add secp256k1_ prefix to default callback functions (Tim Ruffing)
908bdce Include stdio.h and stdlib.h explicitly in secp256k1.c (Tim Ruffing)
5db782e Allow usage of external default callbacks (Tim Ruffing)
6095a86 Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return (Tim Ruffing)
Pull request description:
This is intended for environments without implementations for `abort()`, `fprintf()`, and `stderr`. e.g., embedded systems. Those can provide their own implementations of `default_illegal_callback_fn` and `default_error_callback_fn` at compile time.
If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data `extern` too, because then the initialization lists for `default_illegal_callback` won't contain only constants. (`const` variables are not compile-time constants). So you cannot take callback data in your own default callback function.
As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.
Note that `abort()`, `fprintf()`, and `stderr` are also used in `CHECK`, which is still used in production code if we rely on gmp for scalar and field inversions (e.g., https://github.com/bitcoin-core/secp256k1/blob/master/src/scalar_impl.h#L240). This is not an issue for embedded system which probably don't want to use gmp anyway, but it is probably an issue for the reasons explained in https://github.com/bitcoin-core/secp256k1/pull/566#issuecomment-469111901.
(related downstream: https://github.com/rust-bitcoin/rust-secp256k1/pull/100 @elichai)
ACKs for commit e49f79:
Tree-SHA512: 4dec0821eef4156cbe162bd8cdf0531c1fae8c98cd9db8438170ff1aa0e59b199739eeab293695bb582246812bea5309959f02f1fb74bb57872da54ebc52313f
Most of the codebase correctly used short-cutting to avoid calling
_is_zero on possibly incompletely initialized elements, but a few
places were missed.
98836b1 scratch: replace frames with "checkpoint" system (Andrew Poelstra)
7623cf2 scratch: save a couple bytes of unnecessarily-allocated memory (Andrew Poelstra)
a7a164f scratch: rename `max_size` to `size`, document that extra will actually be allocated (Andrew Poelstra)
5a4bc0b scratch: unify allocations (Andrew Poelstra)
c2b028a scratch space: thread `error_callback` into all scratch space functions (Andrew Poelstra)
0be1a4a scratch: add magic bytes to beginning of structure (Andrew Poelstra)
92a48a7 scratch space: use single allocation (Andrew Poelstra)
Pull request description:
ACKs for commit 98836b:
Tree-SHA512: 6e251f704644a5f61b24aa05c6f7a31ad8c58d147195079d52fe45daacd28a9fd2f4aaf71273183b99b3795a01a88f8389170d4280489b2a28a14a56e03153d7
9ab96f7 Use trivial algorithm in ecmult_multi if scratch space is small (Jonas Nick)
Pull request description:
`ecmult_multi` already selects the trivial algorithm if the scratch space is NULL. With this PR the trivial algorithm is also selected if the scratch space is too small to use pippenger or strauss instead of returning 0. That makes it more easier to avoid consensus relevant inconsistencies just because scratch space construction was messed up.
ACKs for commit 9ab96f:
real-or-random:
utACK 9ab96f7
Tree-SHA512: aa451adf8880af15cf167a59cb07fc411edc43f26c8eb0873bdae2774382ba182e2a1c54487912f8f2999cb0402d554b9d293e2fb9483234471348a1f43c6653
Broken by 3f3964e4.
It's important that the tests are also run without -DVERIFY due to
the possibility that side-effects of a VERIFY_CHECK fix a bug that
would otherwise be detected.
Use of the verify_check macro in tests isn't sufficient.
