acb7f97eb82dfbbdb797354e1550b910055b4422 README: add a section for test coverage (Marko Bencun)
Pull request description:
It is a hassle to figure out the exact commands to create a good
report.
ACKs for top commit:
real-or-random:
ACK acb7f97eb82dfbbdb797354e1550b910055b4422
sipa:
ACK acb7f97eb82dfbbdb797354e1550b910055b4422
Tree-SHA512: d39f3e0b289229b2ce085406f6d716fdd54038df9ee5273a18a05140d1eddd4149149e881cc7a13f2126347217b9c56a0c12adf558c49879c5f556695242afc6
d567b779fe446fd18820a9d2968ecb703c8dea19 Clarify comments about use of rzr on ge functions and abs function. (Gregory Maxwell)
2241ae6d14df187e2c8d6fe5b44e3d850474af38 Remove secret-dependant non-constant time operation in ecmult_const. (Gregory Maxwell)
Pull request description:
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
ACKs for top commit:
real-or-random:
ACK d567b779fe446fd18820a9d2968ecb703c8dea19 I read the diff carefully and tested the code with ECDH enabled and various settings, also on valgrind
sipa:
ACK d567b779fe446fd18820a9d2968ecb703c8dea19
Tree-SHA512: f00a921dcc6cc024cfb3ac1a34c1be619b96f1f17ec0ee0f3ff4ea02035ee288e55469491ed3183e2c4e5560cc068c10aafb657dff95a610706e5b9a8cd13966
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
2e759ec753446aab0272ba32c5f1b7dc3a4dc75c Overhaul README.md (Tim Ruffing)
Pull request description:
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key"
cc @gmaxwell who suggested a similar wording for the disclaimer.
ACKs for top commit:
sipa:
ACK 2e759ec753446aab0272ba32c5f1b7dc3a4dc75c
jonasnick:
ACK 2e759ec753446aab0272ba32c5f1b7dc3a4dc75c
Tree-SHA512: 2e1c87e7fa28d9dab682af227f845e7d48ac79a9fbe10be47ae4567abc2e066ba2f852c000db7d697ece8e4bbeeb851ea647465f870ac29dc3654031bf15a1ad
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key
* Define "experimental"
Co-Authored-By: Gregory Maxwell <greg@xiph.org>
bde2a32286c697dd1056aa3eb1ea2a5353f0bede Convert bench.h to fixed-point math (Wladimir J. van der Laan)
Pull request description:
Convert `bench.h` to fixed-point math, removing all use of float math from the repository:
- Use 64-bit integer microsecond timestamps
- Use decimal fixed-point math for formatting numbers
It turned out to be a little trickier than I expected because of formatting and rounding. But, output should be the same before and after.
I used the following to test the number formatting: https://gist.github.com/laanwj/f971bfbe018e39c19677a21ff954d0c7
ACKs for top commit:
real-or-random:
ACK bde2a32286c697dd1056aa3eb1ea2a5353f0bede I've read the code in detail and I've tested it. I haven't explicitly tested the formatting function with known/hardcoded inputs.
Tree-SHA512: 41ab6024b88c65a4b194272097c70d527bedb396dc7ab9d3d93165f1a19d31092798370f66399443a8d5393d0a6dcf5825679de5a325550865cfdef3586bf64c
- Use 64-bit integer microsecond timestamps
- Use fixed-point math for formatting numbers
Then, remove "except in benchmarks" exception from `README.md`.
78c38363412db3ea1cd1f0cc42dd1624c078ee32 Add SECURITY.md (Jonas Nick)
Pull request description:
Fixes#646
WIP because the secp256k1-security@bitcoincore.org email address doesn't exist yet. But it seems like the right place for vulnerability reports. security@bitcoincore.org would have the downside that it perhaps reaches more people than necessary. Ideally secp256k1-security would just forward to the three maintainers listed in SECURITY.md. @sipa @apoelstra is it okay to put you there? Fwiw I'm opting out for now because three people should be enough.
@sipa do you know who to talk to about adding secp256k1-security@bitcoincore.org and the specifics about how it would work?
ACKs for top commit:
real-or-random:
ACK 78c38363412db3ea1cd1f0cc42dd1624c078ee32 I looked at the diff and verified my fingerprint
Tree-SHA512: 53a989615665cf8cf0c6a70d3bc2c4b71b68178cae40b2a7881aa9eba24732d126ba1e258a9fc127c69b47bb3025943097300cfcbbe18736cbf92ff4f3a901e0
a0771d1 Explicitly disable buffering for stderr in tests (Jonas Nick)
fb424fb Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run. (Jonas Nick)
Pull request description:
…by removing stdout buffering and always cat tests.log after a travis run. Fixes#645.
I noticed that according to the [doc](https://www.gnu.org/software/automake/manual/html_node/Parallel-Test-Harness.html) tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call `fflush` twice.
Travis is instructed to always show the seed which seems helpful with `after_script` by `cat`ing `./tests.log`. In case the tests fail it looks like https://travis-ci.org/jonasnick/secp256k1/jobs/606446234.
ACKs for commit a0771d:
real-or-random:
ACK a0771d15e67d3fe6ac1791f81d9731f73c550e5e I looked at the diff and checked that it does not break the tests
Tree-SHA512: 3ba37c2d9169867112981bba3d56680000651ef22ef684c3703f26ed3f71bf415fb23875d30059c8247ea9520c9cfad2c9207badf1b33da8fa3b7b7235a8bf16
dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7 travis: Added a valgrind test without endro and enabled recovery+ecdh (Elichai Turkel)
b4c1382a87dde22d0a5075e56fb7f5d2a09f7cc7 Add valgrind check to travis (Elichai Turkel)
Pull request description:
As discussed in https://github.com/bitcoin-core/secp256k1/pull/687
This adds valgrind check to the repo.
It doesn't run on recovery+ecdh because of the time.
No openssl because of uninitialized mem.
I debated between with and without ASM, but decided with ASM because it might be more fragile(?).
I wasn't sure if I should pass `-DVALGRIND` via `CFLAGS` or `CPPFLAGS`, it seems like because this is only C then there shouldn't even be `CPPFLAGS` but looks like we use `CPPFLAGS` in other places for the preprocessor definitions.
If people are worried about the time it takes we can mark it as `allow_failure` although I don't think it's a problem here because there's only a handful of PRs and they're usually open for weeks.
ACKs for top commit:
real-or-random:
ACK dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7 I looked at the diff
jonasnick:
ACK dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7
Tree-SHA512: 72d7f1f4c8dd4c58501ac1003b28296d6fd140a8f7711e9e3b3c04a3fbce358ff1c89d2e1d1c5489d7668d3019981264c5cadecae3d9b48cd38c9463e287d8ad
362bb25608dbcd724a07dd5170c4ebe081c3dd84 Modified bench_scalar_split so it won't get optimized out (Elichai Turkel)
73a30c6b58f078b42a03a222c55bfe8b4dd86a2b Added accumulators and checks on benchmarks so they won't get optimized out (Elichai Turkel)
Pull request description:
As asked https://github.com/bitcoin-core/secp256k1/pull/667#issuecomment-546885951 this is the parts of #667 that don't require an assembly memory fence.
I splitted them to 2 commits, one with obvious easy ones. and another that changes the logic a bit to achieve this (See https://github.com/bitcoin-core/secp256k1/pull/667#discussion_r337248398 )
ACKs for top commit:
jonasnick:
ACK 362bb256
real-or-random:
ACK 362bb25608dbcd724a07dd5170c4ebe081c3dd84 I read the diff and I ran the benchmarks
Tree-SHA512: d5e47f5d64c3b035155276f057671ceb7f5852f24c7102fee4d0141aabebf882039f3eae0d152bae89d0603bc09fa6ad9f7bc6b8c0f74a668ee252c727517804
5c5f71e Fix ASM setting in travis (Jonas Nick)
Pull request description:
Without this PR the `ASM` setting isn't taken into account in travis.
ACKs for commit 5c5f71:
real-or-random:
ACK 5c5f71eea5167b0dd9dbef246fc70132c50c9af3 I read the diff
Tree-SHA512: 741650e4b9163e0e7341fa59b9859da85d0e34fa59980e68eacf59388879281b640836532acb3d8121da18d8e75a7c2993defada6329df830a99472b71cc17fe
bae1bea3c4b46a2fb5ca76ff6bf1e98d43cff52f Make no-float policy explicit (Tim Ruffing)
Pull request description:
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
Closes#683.
ACKs for top commit:
jonasnick:
ACK bae1bea3c4b46a2fb5ca76ff6bf1e98d43cff52f
Tree-SHA512: e0027d6dda1a3e4b7d146fd3bea04e05473e08e25c0d0730018768be00351dfcf51b87b47b9e27953a21d42e0621433f13cbe55e4c20a7f7086e0191dff607a6
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
b76142f Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var which was removed in 47045270fa90f81205d989f7107769bce1e71c4d (Jonas Nick)
Pull request description:
...which was removed in 47045270fa90f81205d989f7107769bce1e71c4d. h/t @roconnor-blockstream
ACKs for commit b76142:
Tree-SHA512: 05fcd7aa5d765f1f5d31b93d40c2621e1dd9674a0db136a1e1cb216d6c01f5be1580275700cbdc08feda8f165b3b349640472d0bdec770bebb23f952225e3f52
0d82732 Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow. (Russell O'Connor)
8fe63e5 Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands. (roconnor-blockstream)
Pull request description:
Avoid possible, but unlikely undefined behaviour in `scalar_low_impl`'s `secp256k1_scalar_cadd_bit`.
Thanks to elichai2 who noted that the literal `1` is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour.
Using the unsigned literal `1u` addresses the issue.
ACKs for commit 0d8273:
real-or-random:
ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76
jonasnick:
ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76
Tree-SHA512: 905be3b8b00aa5cc9bd6dabb543745119da8f34181d37765071f28abbc1d6ff3659e3f195b72c2f2d003006678823919668bc0d169ac8b8d4bcc5da671813c99
59782c68b41e4262f003135717705990b3fdc3ae Remove mention of ec_privkey_export because it doesn't exist (Jonas Nick)
Pull request description:
Fixes#663
There is `ec_privkey_export_der` but it takes `0` for uncompressed and not `SECP256K1_EC_UNCOMPRESSED` (which is `2`).
ACKs for top commit:
real-or-random:
ACK 59782c68b4
apoelstra:
utACK 59782c68b4
Tree-SHA512: 6167581df74264be576f921d04bb8e23e16fa3b823bac4b45299079ceee38d6c74dd14a55b7b976a2cee9bdbd74dd6e3b39c0482808c1b8e65c8c80743f113a2
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
