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

[crypto] Update relic to 9206ae5 #846

Merged
merged 5 commits into from
Jul 13, 2021
Merged

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Jun 16, 2021

The diff contains:

  • a lot of changes that concern parts of the library we don't use (other curves a la BN, BLS24-X, BLS12-383 ...), integer protocols (ETRS), field extension machinery ...
  • otherwise irrelevant changes, e.g. CI/CD
  • some memory bug fixing

Formatted Log
Full Changeset

Benchmark Diff:

benchstat ~/old_relic.txt ~/new_relic.txt

name                           old time/op  new time/op  delta
ScalarMult/G1-8                 144µs ± 0%   142µs ± 0%   ~     (p=1.000 n=1+1)
ScalarMult/G2-8                 329µs ± 0%   326µs ± 0%   ~     (p=1.000 n=1+1)
HashToG1-8                      231µs ± 0%   230µs ± 0%   ~     (p=1.000 n=1+1)
CheckG1-8                       210µs ± 0%   209µs ± 0%   ~     (p=1.000 n=1+1)
BLSBLS12381Sign-8               477µs ± 0%   473µs ± 0%   ~     (p=1.000 n=1+1)
BLSBLS12381Verify-8            2.71ms ± 0%  2.64ms ± 0%   ~     (p=1.000 n=1+1)
BatchVerify/happy_path-8       81.5ms ± 0%  81.5ms ± 0%   ~     (p=1.000 n=1+1)
BatchVerify/unhappy_path-8      117ms ± 0%   115ms ± 0%   ~     (p=1.000 n=1+1)
VerifySignatureManyMessages-8  3.14ms ± 0%  3.12ms ± 0%   ~     (p=1.000 n=1+1)
Aggregate/PrivateKeys-8        71.6µs ± 0%  70.1µs ± 0%   ~     (p=1.000 n=1+1)
Aggregate/PublicKeys-8         2.85ms ± 0%  2.81ms ± 0%   ~     (p=1.000 n=1+1)
Aggregate/Signatures-8         47.9ms ± 0%  48.0ms ± 0%   ~     (p=1.000 n=1+1)
SimpleKeyGen-8                 20.2ms ± 0%  20.3ms ± 0%   ~     (p=1.000 n=1+1)

Fixed bugs:

  • Unexpected failure of ep2_mul[_lwnaf] above the prime group order #64

Closed issues:

  • Other way to construct towered extension fields #203
  • blake2.h:101:5: error: size of array element is not a multiple of its alignment #202
  • ECIES 160bit #201
  • Compilation with "ARITH gmp" fails #200
  • Support for armv8-a ? #198
  • Function name bn_init conflicts with OpenSSL when used in tandem #196
  • 16-bit MSP430 #193
  • Modular exponentiation returns 1 if exponent is 0 and modulo is 1 #185
  • Compilation of RELIC with bls12-446 and bls12-455 fails #182
  • test_bn fails with BLS12-381 preset #181
  • [BUG] undefined reference to bench_init', bench_clean' #180
  • Tests FTBFS because of missing symbol in header #179
  • Builds are broken #178
  • compile error inlining failed in call to always_inline ‘_mm_alignr_epi8’ on unbantu20.04 gcc9 #177
  • bn_write_str buffer overflow #176
  • ECDSA verify succeeds when it should fail #175
  • ec_mul_gen hangs with curve SECG_K256 #174
  • Wrong square root computation #173
  • Out-of-bounds read via bn_sqr_basic #172
  • OSS-Fuzz integration #171
  • Building Relic with Curve NIST_P256 throws FATAL ERROR in relic_fp_prime.c:120 #170
  • Compressing (packing) a point to binary array does not comply with X9.62 standard #169
  • ‘ctx_t’ {aka ‘struct _ctx_t’} has no member named ‘total’ #168
  • relic does not work with C++ #167
  • Memory leak in ep2_curve_init/clean with ALLOC=DYNAMIC #166
  • *_is_valid() functions produce false negative for not normalized points #147
  • Bench and Test doesnt build #122

Merged pull requests:

@huitseeker huitseeker requested a review from tarakby as a code owner June 16, 2021 16:58
@huitseeker huitseeker changed the title Update relic to 9206ae50b667de160fcc385ba3dc2c920143ab0a [crypto] Update relic to 9206ae5 Jun 16, 2021
@huitseeker huitseeker marked this pull request as draft June 16, 2021 17:17
@tarakby
Copy link
Contributor

tarakby commented Jun 16, 2021

This is amazing! Thank you @huitseeker 🙏
I can continue looking at these changes and fix the failing unit tests 👌

@huitseeker
Copy link
Contributor Author

huitseeker commented Jun 16, 2021

@tarakby I'm looking at the failing test, which is a change in the semantics of interpreting a zero slice as zero since relic-toolkit/relic@16f3c55 (see the last file)

@huitseeker huitseeker force-pushed the update-relic branch 2 times, most recently from 62cf898 to d0dbaea Compare June 18, 2021 14:42
@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2021

Codecov Report

Merging #846 (f80a224) into master (f4a99c7) will increase coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #846      +/-   ##
==========================================
+ Coverage   56.51%   56.89%   +0.38%     
==========================================
  Files         427      429       +2     
  Lines       25126    25652     +526     
==========================================
+ Hits        14199    14595     +396     
- Misses       9008     9083      +75     
- Partials     1919     1974      +55     
Flag Coverage Δ
unittests 56.89% <ø> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
model/flow/service_event.go 40.00% <0.00%> (-16.72%) ⬇️
model/flow/identifier.go 75.00% <0.00%> (-5.00%) ⬇️
engine/execution/state/delta/view.go 73.07% <0.00%> (-3.76%) ⬇️
engine/execution/state/delta/delta.go 66.66% <0.00%> (-3.61%) ⬇️
engine/common/rpc/convert/convert.go 31.31% <0.00%> (-3.04%) ⬇️
model/flow/role.go 89.36% <0.00%> (-1.95%) ⬇️
engine/consensus/sealing/core.go 60.98% <0.00%> (-0.76%) ⬇️
engine/verification/verifier/engine.go 52.97% <0.00%> (-0.64%) ⬇️
cmd/util/ledger/migrations/storage_v4.go 41.56% <0.00%> (-0.61%) ⬇️
model/flow/ledger.go 12.50% <0.00%> (-0.55%) ⬇️
... and 36 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 ce77c63...f80a224. Read the comment docs.

@huitseeker huitseeker marked this pull request as ready for review June 18, 2021 15:06
@tarakby tarakby self-assigned this Jun 18, 2021
Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for compiling all the relevant commits and updates 🏄

I have made a suggestion branch to compile some changes. It mainly adds two points compared to your branch:

  • COMP is not taken into account by the cmake pre-build, I updated it to CFLAGS.
  • I added initializations for other bn_st variables used in the package. They need to be properly initialized after relic trimming was updated).

Feel free to cherry-pick the commits if you agree to them 🙏

crypto/bls12381_utils.go Outdated Show resolved Hide resolved
crypto/bls12381_utils.c Outdated Show resolved Hide resolved
@huitseeker huitseeker force-pushed the update-relic branch 2 times, most recently from 3b24ded to d5abcf2 Compare July 12, 2021 13:55
@huitseeker
Copy link
Contributor Author

@tarakby I think I merged all the relic update related changes from your branch, thanks for the commits!

@tarakby
Copy link
Contributor

tarakby commented Jul 12, 2021

Thanks @huitseeker for adding the commits!

Everything looks good, we just need to delete the 2 lines commented here and here since the initializations are now happening before calling the functions.

Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏄 🏄‍♀️

Copy link
Member

@Kay-Zee Kay-Zee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly focused my attention on the two new go helper functions and their usages, making sure those made sense.

Did not fully follow the C changes but nothing obvious jumped out at me looking through them.

huitseeker and others added 5 commits July 13, 2021 18:40
The diff contains:
- a lot of changes that concern parts of the library we don't use (other curves a la BN, BLS24-X, BLS12-383 ...), integer protocols (ETRS), field extension machinery ...
- otherwise irrelevant changes, e.g. CI/CD
- some memory bug fixing

[Full Changeset](https://github.com/relic-toolkit/relic/compare/7a9bba7f..9206ae5)

**Fixed bugs:**

- Unexpected failure of ep2\_mul\[\_lwnaf\] above the prime group order [\onflow#64](relic-toolkit/relic#64)

**Closed issues:**

- Other way to construct towered extension fields [\onflow#203](relic-toolkit/relic#203)
- blake2.h:101:5: error: size of array element is not a multiple of its alignment [\onflow#202](relic-toolkit/relic#202)
- ECIES 160bit [\onflow#201](relic-toolkit/relic#201)
- Compilation with "ARITH gmp" fails [\onflow#200](relic-toolkit/relic#200)
- Support for armv8-a ? [\onflow#198](relic-toolkit/relic#198)
- Function name bn\_init conflicts with OpenSSL when used in tandem [\onflow#196](relic-toolkit/relic#196)
- 16-bit MSP430 [\onflow#193](relic-toolkit/relic#193)
- Modular exponentiation returns 1 if exponent is 0 and modulo is 1 [\onflow#185](relic-toolkit/relic#185)
- Compilation of RELIC with bls12-446 and bls12-455 fails [\onflow#182](relic-toolkit/relic#182)
- test\_bn fails with BLS12-381 preset [\onflow#181](relic-toolkit/relic#181)
- \[BUG\] undefined reference to `bench_init', `bench\_clean' [\onflow#180](relic-toolkit/relic#180)
- Tests FTBFS because of missing symbol in header [\onflow#179](relic-toolkit/relic#179)
- Builds are broken [\onflow#178](relic-toolkit/relic#178)
- compile error  inlining failed in call to always\_inline ‘\_mm\_alignr\_epi8’ on unbantu20.04 gcc9 [\onflow#177](relic-toolkit/relic#177)
- bn\_write\_str buffer overflow [\onflow#176](relic-toolkit/relic#176)
- ECDSA verify succeeds when it should fail [\onflow#175](relic-toolkit/relic#175)
- ec\_mul\_gen hangs with curve SECG\_K256 [\onflow#174](relic-toolkit/relic#174)
- Wrong square root computation [\onflow#173](relic-toolkit/relic#173)
- Out-of-bounds read via bn\_sqr\_basic [\onflow#172](relic-toolkit/relic#172)
- OSS-Fuzz integration [\onflow#171](relic-toolkit/relic#171)
- Building Relic with Curve NIST\_P256 throws FATAL ERROR in relic\_fp\_prime.c:120 [\onflow#170](relic-toolkit/relic#170)
- Compressing \(packing\) a point to binary array does not comply with X9.62 standard [\onflow#169](relic-toolkit/relic#169)
-  ‘ctx\_t’ {aka ‘struct \_ctx\_t’} has no member named ‘total’ [\onflow#168](relic-toolkit/relic#168)
- relic does not work with C++ [\onflow#167](relic-toolkit/relic#167)
- Memory leak in ep2\_curve\_init/clean with ALLOC=DYNAMIC [\onflow#166](relic-toolkit/relic#166)
- \*\_is\_valid\(\) functions produce false negative for not normalized points [\onflow#147](relic-toolkit/relic#147)
- Bench and Test doesnt build [\onflow#122](relic-toolkit/relic#122)

**Merged pull requests:**

- Add pairing delegation protocols [\onflow#199](relic-toolkit/relic#199) ([dfaranha](https://github.com/dfaranha))
- Fix support for Win64/MSVC targets. [\onflow#197](relic-toolkit/relic#197) ([dfaranha](https://github.com/dfaranha))
- Simplify generator getting for Gt. [\onflow#194](relic-toolkit/relic#194) ([luozejiaqun](https://github.com/luozejiaqun))
- cmake: Always use user defined CFLAGS, not only for release builds [\onflow#187](relic-toolkit/relic#187) ([xdustinface](https://github.com/xdustinface))
- Fix MinGW build [\onflow#186](relic-toolkit/relic#186) ([xdustinface](https://github.com/xdustinface))
- Remove debug printf in bn\_mxp\_slide [\onflow#184](relic-toolkit/relic#184) ([guidovranken](https://github.com/guidovranken))
- Remove ALLOC = STACK to simplify memory allocation. [\onflow#183](relic-toolkit/relic#183) ([dfaranha](https://github.com/dfaranha))
- Update relic\_alloc.h [\onflow#165](relic-toolkit/relic#165) ([aguycalled](https://github.com/aguycalled))
- Add correct support for FreeBSD and NetBSD [\onflow#164](relic-toolkit/relic#164) ([hoffmang9](https://github.com/hoffmang9))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants