Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.60.0-b1 with updated libsecp256k1 and beast b24 #1983

Merged
merged 4 commits into from Feb 1, 2017

Conversation

Projects
None yet
4 participants
@vinniefalco
Copy link
Contributor

vinniefalco commented Jan 27, 2017

This brings the secp256k1 subtree up to the tip of the master branch in the upstream repository (https://github.com/bitcoin-core/secp256k1)

The review needs to provide special attention to the treatment of non-canonical signatures. In particular we need to know whether or not calls to secp256k1_ecdsa_signature_normalize should be inserted, and where (if any).

Additionally, the beast subtree is brought up to 1.0.0-b24 which includes support for the permessage-deflate extension (defaulting to off). There should be no behavioral changes in rippled.

@vinniefalco vinniefalco requested review from nbougalis and ximinez Jan 27, 2017

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from db30f9b to 707db69 Jan 27, 2017

@nbougalis

This comment has been minimized.

Copy link
Member

nbougalis commented Jan 27, 2017

VS project seems to be out of date, according to Travis

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from 707db69 to aabc97e Jan 27, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 27, 2017

Codecov Report

Merging #1983 into develop will increase coverage by -0.2%.

@@            Coverage Diff             @@
##           develop    #1983     +/-   ##
==========================================
- Coverage    67.31%   67.12%   -0.2%     
==========================================
  Files          686      686             
  Lines        49320    49329      +9     
==========================================
- Hits         33201    33111     -90     
- Misses       16119    16218     +99
Impacted Files Coverage Δ
src/ripple/protocol/impl/BuildInfo.cpp 82.6% <ø> (ø)
src/ripple/protocol/SecretKey.h 100% <ø> (ø)
src/ripple/protocol/PublicKey.h 100% <ø> (ø)
src/ripple/protocol/impl/secp256k1.h 100% <ø> (ø)
src/ripple/protocol/impl/PublicKey.cpp 87.69% <53.84%> (-3.15%)
src/ripple/protocol/impl/SecretKey.cpp 82.85% <68.42%> (-4.28%)
src/ripple/core/DeadlineTimer.h 33.33% <ø> (-66.67%)
src/ripple/ledger/impl/CachedSLEs.cpp 0% <ø> (-50%)
src/ripple/app/ledger/LedgerHistory.h 25% <ø> (-50%)
src/ripple/shamap/FullBelowCache.h 75% <ø> (-15%)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8a5f5b...b6126f2. Read the comment docs.

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from aabc97e to 0ed7911 Jan 28, 2017

@vinniefalco vinniefalco changed the title 0.60.0-b1 with updated libsecp256k1 0.60.0-b1 with updated libsecp256k1 and beast b24 Jan 28, 2017

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

MarkusTeufelberger commented Jan 28, 2017

While you're updating dependencies, maybe have a look at RocksDB too. :-)

@vinniefalco

This comment has been minimized.

Copy link
Contributor Author

vinniefalco commented Jan 28, 2017

Oh heh not a bad idea!

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

MarkusTeufelberger commented Jan 29, 2017

Latest snappy release (https://github.com/google/snappy) also claims some performance improvements and the protobuf library included in rippled is also getting a bit dusty. SOCI doesn't seem to have had releases since 2015 and most commits seem to be around adding tests and tooling, not fixing security bugs.

https://github.com/open-source-parsers/jsoncpp is also used but not included as subtree, I don't know how much of the upstream code is even left in rippled.

LZ4 might also profit from an update, though I'm not so sure where LZ4 is used in rippled. In case you want to look at updating compression libraries, evaluating zstd (https://github.com/facebook/zstd) might be interesting too, as well as checking out Zopfli and Brotli by Google. Apparently the series "Silicon Valley" created a bit of interest in compression technologies.

@vinniefalco

This comment has been minimized.

Copy link
Contributor Author

vinniefalco commented Jan 29, 2017

We could look into that, but I rather not go updating everything at once!

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

MarkusTeufelberger commented Jan 30, 2017

Sure, I was just looking through the dependencies and was kinda baffled when some were nearly older than the C++ dialect you're using. ;-)

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from 0ed7911 to 8bbb3e8 Jan 30, 2017

@vinniefalco

This comment has been minimized.

Copy link
Contributor Author

vinniefalco commented Jan 30, 2017

Rebased on 0.50.1

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from cd7ae93 to a3446fe Jan 31, 2017

@vinniefalco

This comment has been minimized.

Copy link
Contributor Author

vinniefalco commented Jan 31, 2017

Rebased on 0.50.2

@@ -276,11 +455,13 @@ class SecretKey_test : public beast::unit_test::suite

void run() override
{
testCanonicality();

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jan 31, 2017

Member

Weird indentation

return Buffer(sig, siglen);

unsigned char sig[72];
size_t len = sizeof(sig);

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jan 31, 2017

Member

std::size_t?

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Jan 31, 2017

Author Contributor

Intentional, see secp256k1.h

sk.data(), secp256k1_nonce_function_rfc6979,
nullptr);
if (result != 1)
BOOST_ASSERT(sk.size() == 32);

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jan 31, 2017

Member

I think this is better suited as a LogicError: if the secret key isn't 32 bytes, something is very, very wrong.

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Jan 31, 2017

Author Contributor

Its an assert because SecretKey is sized at compile time to 32 bytes. No need to incur a runtime penalty for something which is impossible.

This comment has been minimized.

Copy link
@nbougalis

nbougalis Feb 1, 2017

Member

👍 We should make size be constexpr so this could be a static_assert.

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Feb 1, 2017

Author Contributor

Nah, no need for that. We might want larger secrets in the future.

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from 5174e67 to 88401f7 Jan 31, 2017

@nbougalis nbougalis referenced this pull request Feb 1, 2017

Merged

Proposed 0.60.0-b2 #1992

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from 1b13f07 to 2549ab5 Feb 1, 2017

vinniefalco added some commits Feb 1, 2017

Squashed 'src/secp256k1/' changes from 0cbc860..9d560f9
9d560f9 Merge #428: Exhaustive recovery
2cee5fd exhaustive tests: add recovery module
8225239 Merge #433: Make the libcrypto detection fail the newer API.
12de863 Make the libcrypto detection fail the newer API.
678b0e5 exhaustive tests: remove erroneous comment from ecdsa_sig_sign
2928420 Merge #427: Remove Schnorr from travis as well
03ff8c2 group_impl.h: remove unused `secp256k1_ge_set_infinity` function
a724d72 configure: add --enable-coverage to set options for coverage analysis
b595163 recovery: add tests to cover API misusage
8eecc4a Remove Schnorr from travis as well
6f8ae2f ecdh: test NULL-checking of arguments
25e3cfb ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign
a8abae7 Merge #310: Add exhaustive test for group functions on a low-order subgroup
b4ceedf Add exhaustive test for verification
83836a9 Add exhaustive tests for group arithmetic, signing, and ecmult on a small group
20b8877 Add exhaustive test for group functions on a low-order subgroup
80773a6 Merge #425: Remove Schnorr experiment
e06e878 Remove Schnorr experiment
04c8ef3 Merge #407: Modify parameter order of internal functions to match API parameter order
6e06696 Merge #411: Remove guarantees about memcmp-ability
40c8d7e Merge #421: Update scalar_4x64_impl.h
a922365 Merge #422: Restructure nonce clearing
3769783 Restructure nonce clearing
0f9e69d Restructure nonce clearing
9d67afa Update scalar_4x64_impl.h
7d15cd7 Merge #413: fix auto-enabled static precompuatation
00c5d2e fix auto-enabled static precompuatation
91219a1 Remove guarantees about memcmp-ability
7a49cac Merge #410: Add string.h include to ecmult_impl
0bbd5d4 Add string.h include to ecmult_impl
353c1bf Fix secp256k1_ge_set_table_gej_var parameter order
541b783 Fix secp256k1_ge_set_all_gej_var parameter order
7d893f4 Fix secp256k1_fe_inv_all_var parameter order
c5b32e1 Merge #405: Make secp256k1_fe_sqrt constant time
926836a Make secp256k1_fe_sqrt constant time
e2a8e92 Merge #404: Replace 3M + 4S doubling formula with 2M + 5S one
8ec49d8 Add note about 2M + 5S doubling formula
5a91bd7 Merge #400: A couple minor cleanups
ac01378 build: add -DSECP256K1_BUILD to benchmark_internal build flags
a6c6f99 Remove a bunch of unused stdlib #includes
65285a6 Merge #403: configure: add flag to disable OpenSSL tests
a9b2a5d configure: add flag to disable OpenSSL tests
b340123 Merge #402: Add support for testing quadratic residues
e6e9805 Add function for testing quadratic residue field/group elements.
efd953a Add Jacobi symbol test via GMP
fa36a0d Merge #401: ecmult_const: unify endomorphism and non-endomorphism skew cases
c6191fd ecmult_const: unify endomorphism and non-endomorphism skew cases
0b3e618 Merge #378: .gitignore build-aux cleanup
6042217 Merge #384: JNI: align shared files copyright/comments to bitcoinj's
24ad20f Merge #399: build: verify that the native compiler works for static precomp
b3be852 Merge #398: Test whether ECDH and Schnorr are enabled for JNI
aa0b1fd build: verify that the native compiler works for static precomp
eee808d Test whether ECDH and Schnorr are enabled for JNI
7b0fb18 Merge #366: ARM assembly implementation of field_10x26 inner (rebase of #173)
001f176 ARM assembly implementation of field_10x26 inner
0172be9 Merge #397: Small fixes for sha256
3f8b78e Fix undefs in hash_impl.h
2ab4695 Fix state size in sha256 struct
6875b01 Merge #386: Add some missing `VERIFY_CHECK(ctx != NULL)`
2c52b5d Merge #389: Cast pointers through uintptr_t under JNI
43097a4 Merge #390: Update bitcoin-core GitHub links
31c9c12 Merge #391: JNI: Only call ecdsa_verify if its inputs parsed correctly
1cb2302 Merge #392: Add testcase which hits additional branch in secp256k1_scalar_sqr
d2ee340 Merge #388: bench_ecdh: fix call to secp256k1_context_create
093a497 Add testcase which hits additional branch in secp256k1_scalar_sqr
a40c701 JNI: Only call ecdsa_verify if its inputs parsed correctly
faa2a11 Update bitcoin-core GitHub links
47b9e78 Cast pointers through uintptr_t under JNI
f36f9c6 bench_ecdh: fix call to secp256k1_context_create
bcc4881 Add some missing `VERIFY_CHECK(ctx != NULL)` for functions that use `ARG_CHECK`
6ceea2c align shared files copyright/comments to bitcoinj's
70141a8 Update .gitignore
7b549b1 Merge #373: build: fix x86_64 asm detection for some compilers
bc7c93c Merge #374: Add note about y=0 being possible on one of the sextic twists
e457018 Merge #364: JNI rebased
86e2d07 JNI library: cleanup, removed unimplemented code
3093576 JNI library
bd2895f Merge pull request #371
e72e93a Add note about y=0 being possible on one of the sextic twists
3f8fdfb build: fix x86_64 asm detection for some compilers
e5a9047 [Trivial] Remove double semicolons
c18b869 Merge pull request #360
3026daa Merge pull request #302
03d4611 Add sage verification script for the group laws
a965937 Merge pull request #361
83221ec Add experimental features to configure
5d4c5a3 Prevent damage_array in the signature test from going out of bounds.
419bf7f Merge pull request #356
6c527ec Merge pull request #357
445f7f1 Fix for Windows compile issue
03d84a4 Benchmark against OpenSSL verification
2bfb82b Merge pull request #351
06aeea5 Turn secp256k1_ec_pubkey_serialize outlen to in/out
970164d Merge pull request #348
64666251 Improvements for coordinate decompression
e2100ad Merge pull request #347
8e48787 Change secp256k1_ec_pubkey_combine's count argument to size_t.
c69dea0 Clear output in more cases for pubkey_combine, adds tests.
269d422 Comment copyediting.
b4d17da Merge pull request #344
4709265 Merge pull request #345
26abce7 Adds 32 static test vectors for scalar mul, sqr, inv.
5b71a3f Better error case handling for pubkey_create & pubkey_serialize, more tests.
3b7bc69 Merge pull request #343
eed87af Change contrib/laxder from headers-only to files compilable as standalone C
d7eb1ae Merge pull request #342
7914a6e Make lax_der_privatekey_parsing.h not depend on internal code
73f64ff Merge pull request #339
9234391 Overhaul flags handling
1a36898 Make flags more explicit, add runtime checks.
1a3e03a Merge pull request #340
96be204 Add additional tests for eckey and arg-checks.
bb5aa4d Make the tweak function zeroize-output-on-fail behavior consistent.
4a243da Move secp256k1_ec_privkey_import/export to contrib.
1b3efc1 Move secp256k1_ecdsa_sig_recover into the recovery module.
e3cd679 Eliminate all side-effects from VERIFY_CHECK() usage.
b30fc85 Avoid nonce_function_rfc6979 algo16 argument emulation.
70d4640 Make secp256k1_ec_pubkey_create skip processing invalid secret keys.
6c476a8 Minor comment improvements.
131afe5 Merge pull request #334
0c6ab2f Introduce explicit lower-S normalization
fea19e7 Add contrib/lax_der_parsing.h
3bb9c44 Rewrite ECDSA signature parsing code
fa57f1b Use secp256k1_rand_int and secp256k1_rand_bits more
49b3749 Add new tests for the extra testrand functions
f684d7d Faster secp256k1_rand_int implementation
251b1a6 Improve testrand: add extra random functions
31994c8 Merge pull request #338
f79aa88 Bugfix: swap arguments to noncefp
c98df26 Merge pull request #319
67f7da4 Extensive interface and operations tests for secp256k1_ec_pubkey_parse.
ee2cb40 Add ARG_CHECKs to secp256k1_ec_pubkey_parse/secp256k1_ec_pubkey_serialize
7450ef1 Merge pull request #328
68a3c76 Merge pull request #329
98135ee Merge pull request #332
37100d7 improve ECDH header-doc
b13d749 Fix couple of typos in API comments
7c823e3 travis: fixup module configs
cc3141a Merge pull request #325
ee58fae Merge pull request #326
213aa67 Do not force benchmarks to be statically linked.
338fc8b Add API exports to secp256k1_nonce_function_default and secp256k1_nonce_function_rfc6979.
52fd03f Merge pull request #320
9f6993f Remove some dead code.
357f8cd Merge pull request #314
118cd82 Use explicit symbol visibility.
4e64608 Include public module headers when compiling modules.
1f41437 Merge pull request #316
fe0d463 Merge pull request #317
cfe0ed9 Fix miscellaneous style nits that irritate overactive static analysis.
2b199de Use the explicit NULL macro for pointer comparisons.
9e90516 Merge pull request #294
dd891e0 Get rid of _t as it is POSIX reserved
201819b Merge pull request #313
912f203 Eliminate a few unbraced statements that crept into the code.
eeab823 Merge pull request #299
486b9bb Use a flags bitfield for compressed option to secp256k1_ec_pubkey_serialize and secp256k1_ec_privkey_export
05732c5 Callback data: Accept pointers to either const or non-const data
1973c73 Bugfix: Reinitialise buffer lengths that have been used as outputs
788038d Use size_t for lengths (at least in external API)
c9d7c2a secp256k1_context_set_{error,illegal}_callback: Restore default handler by passing NULL as function argument
9aac008 secp256k1_context_destroy: Allow NULL argument as a no-op
64b730b secp256k1_context_create: Use unsigned type for flags bitfield
cb04ab5 Merge pull request #309
a551669 Merge pull request #295
81e45ff Update group_impl.h
85e3a2c Merge pull request #112
b2eb63b Merge pull request #293
dc0ce9f [API BREAK] Change argument order to out/outin/in
6d947ca Merge pull request #298
c822693 Merge pull request #301
6d04350 Merge pull request #303
7ab311c Merge pull request #304
5fb3229 Fixes a bug where bench_sign would fail due to passing in too small a buffer.
263dcbc remove unused assignment
b183b41 bugfix: "ARG_CHECK(ctx != NULL)" makes no sense
6da1446 build: fix parallel build
5eb4356 Merge pull request #291
c996d53 Print success
9f443be Move pubkey recovery code to separate module
d49abbd Separate ECDSA recovery tests
439d34a Separate recoverable and normal signatures
a7b046e Merge pull request #289
f66907f Improve/reformat API documentation secp256k1.h
2f77487 Add context building benchmarks
cc623d5 Merge pull request #287
de7e398 small typo fix
9d96e36 Merge pull request #280
432e1ce Merge pull request #283
14727fd Use correct name in gitignore
356b0e9 Actually test static precomputation in Travis
ff3a5df Merge pull request #284
2587208 Merge pull request #212
a5a66c7 Add support for custom EC-Schnorr-SHA256 signatures
d84a378 Merge pull request #252
72ae443 Improve perf. of cmov-based table lookup
92e53fc Implement endomorphism optimization for secp256k1_ecmult_const
ed35d43 Make `secp256k1_scalar_add_bit` conditional; make `secp256k1_scalar_split_lambda_var` constant time
91c0ce9 Add benchmarks for ECDH and const-time multiplication
0739bbb Add ECDH module which works by hashing the output of ecmult_const
4401500 Add constant-time multiply `secp256k1_ecmult_const` for ECDH
e4ce393 build: fix hard-coded usage of "gen_context"
b8e39ac build: don't use BUILT_SOURCES for the static context header
baa75da tests: add a couple tests
ae4f0c6 Merge pull request #278
995c548 Introduce callback functions for dealing with errors.
c333074 Merge pull request #282
18c329c Remove the internal secp256k1_ecdsa_sig_t type
74a2acd Add a secp256k1_ecdsa_signature_t type
23cfa91 Introduce secp256k1_pubkey_t type
4c63780 Merge pull request #269
3e6f1e2 Change rfc6979 implementation to be a generic PRNG
ed5334a Update configure.ac to make it build on OpenBSD
1b68366 Merge pull request #274
a83bb48 Make ecmult static precomputation default
166b32f Merge pull request #276
c37812f Add gen_context src/ecmult_static_context.h to CLEANFILES to fix distclean.
125c15d Merge pull request #275
76f6769 Fix build with static ecmult altroot and make dist.
5133f78 Merge pull request #254
b0a60e6 Merge pull request #258
733c1e6 Add travis build to test the static context.
fbecc38 Add ability to use a statically generated ecmult context.
4fb174d Merge pull request #263
4ab8990 Merge pull request #270
bdf0e0c Merge pull request #271
31d0c1f Merge pull request #273
eb2c8ff Add missing casts to SECP256K1_FE_CONST_INNER
55399c2 Further performance improvements to _ecmult_wnaf
99fd963 Add secp256k1_ec_pubkey_compress(), with test similar to the related decompress() function.
145cc6e Improve performance of _ecmult_wnaf
36b305a Verify the result of GMP modular inverse using non-GMP code
e2a07c7 Fix compilation with C++
2b4cf41 Use pkg-config always when possible, with failover to manual checks for libcrypto

git-subtree-dir: src/secp256k1
git-subtree-split: 9d560f992db26612ce2630b194aef5f44d63a530

@vinniefalco vinniefalco force-pushed the vinniefalco:secp256k1 branch from 2549ab5 to b6126f2 Feb 1, 2017

@nbougalis nbougalis merged commit b6126f2 into ripple:develop Feb 1, 2017

2 of 4 checks passed

codecov/patch 62.5% of diff hit (target 67.31%)
Details
codecov/project 67.12% (-0.2%) compared to d8a5f5b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vinniefalco vinniefalco deleted the vinniefalco:secp256k1 branch Feb 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.