relab / hotstuff

MIT License
172 stars 53 forks source link

Crypto refactor #36

Closed johningve closed 2 years ago

johningve commented 2 years ago

This is a complete rewrite of the crypto implementations based on discussion in #34.

Important Changes:

codecov-commenter commented 2 years ago

Codecov Report

Merging #36 (e21bf85) into master (25e7ec2) will increase coverage by 5.56%. The diff coverage is 79.68%.

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   53.31%   58.87%   +5.56%     
==========================================
  Files          70       70              
  Lines        6398     6410      +12     
==========================================
+ Hits         3411     3774     +363     
+ Misses       2710     2347     -363     
- Partials      277      289      +12     
Flag Coverage Δ
unittests 58.87% <79.68%> (+5.56%) :arrow_up:

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

Impacted Files Coverage Δ
crypto/bls12/bls12.go 64.12% <69.94%> (+15.02%) :arrow_up:
crypto/crypto.go 70.75% <70.75%> (ø)
crypto/ecdsa/ecdsa.go 72.91% <71.25%> (+13.02%) :arrow_up:
backend/config.go 81.03% <76.56%> (+17.03%) :arrow_up:
twins/network.go 91.20% <90.56%> (+5.41%) :arrow_up:
crypto/cache.go 88.42% <97.95%> (+18.88%) :arrow_up:
backend/server.go 61.68% <100.00%> (+15.38%) :arrow_up:
consensus/consensus.go 73.05% <100.00%> (+9.58%) :arrow_up:
consensus/events.go 100.00% <100.00%> (ø)
consensus/modules.go 100.00% <100.00%> (+6.45%) :arrow_up:
... and 32 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 25e7ec2...e21bf85. Read the comment docs.

johningve commented 2 years ago

@meling There are some changes to various tests because I had to change the testutil.TestModules function and make the testutil.CreateBuilders use the twins network to get the new metadata working. It's kinda ugly now, but I'll probably revisit it at some point and try to clean it up.

meling commented 2 years ago

We should make some progress towards getting this merged. What remains to be discussed and what remains to be implemented?

johningve commented 2 years ago

@meling I split the Verify function into two: Verify and BatchVerify. I think this is more sensible than the approach of using the VerifyOption stuff. These functions are no longer responsible for checking whether the number of signatures constitutes a quorum, I moved that to the higher-level Crypto module's functions.