Exhaustive recovery #428

pull apoelstra wants to merge 7 commits into bitcoin-core:master from apoelstra:exhaustive-recovery changing 12 files +350 −32

apoelstra commented at 10:45 PM on November 28, 2016: contributor

Builds on #426. Bumps branch coverage for modules/recovery/main_impl.h to 100%.

ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign

Whenever ecdsa_sig_sign is called, in the case that r == 0 or r overflows,
we want to retry with a different nonce rather than fail signing entirely.
Because of this, we always check the nonce conditions before calling
sig_sign, so these checks should always pass (and in particular, they
are inaccessible through the API and appear as uncovered code in test
coverage).

25e3cfbf9b

ecdh: test NULL-checking of arguments
Boosts the ECDH module to 100% coverage
6f8ae2f3c0
recovery: add tests to cover API misusage b595163992
configure: add --enable-coverage to set options for coverage analysis a724d7296d

group_impl.h: remove unused `secp256k1_ge_set_infinity` function

Also remove `secp256k1_fe_verify` from field_*_.impl.h when VERIFY is not defined

03ff8c2d0a

exhaustive tests: remove erroneous comment from ecdsa_sig_sign

Mathematically, we always overflow when using the exhaustive tests (because our
scalar order is 13 and our field order is on the order of 2^256), but the
`overflow` variable returned when parsing a b32 as a scalar is always set
to 0, to prevent infinite (or practically infinite) loops searching for
non-overflowing scalars.

678b0e5466

exhaustive tests: add recovery module 2cee5fd4c9

in src/tests_exhaustive.c:None in d0ab623a9c outdated

 396 | +                     * but pubkey recovery is impossible in the exhaustive tests (the reason
 397 | +                     * being that there are 12 nonzero r values, 12 nonzero points, and no
 398 | +                     * overlap between the sets, so there are no valid signatures). */
 399 | +
 400 | +                    /* Verify by converting to a standard signature and calling verify */
 401 | +                    secp256k1_ecdsa_recoverable_signature_save(&rsig, &r_s, &s_s, recid);

gmaxwell commented at 9:41 PM on December 16, 2016:

recid is used without initialization here.

apoelstra force-pushed on Dec 16, 2016
apoelstra commented at 9:51 PM on December 16, 2016: contributor

Fixed uninitialized recid and also destroyed ctx so that valgrind would run clean on exhaustive tests.
gmaxwell commented at 10:08 PM on December 16, 2016: contributor

ACK.
sipa merged this on Dec 28, 2016
sipa closed this on Dec 28, 2016
sipa referenced this in commit 9d560f992d on Dec 28, 2016
sipa commented at 12:35 PM on December 28, 2016: contributor

ACK
apoelstra deleted the branch on Jun 19, 2017

Contributors

apoelstra

gmaxwell

sipa

Linked (view graph)

#426 Increase test coverage and add a configure flag to ease coverage analysis