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

batch verification: add ed25519 batch verification implementation #3031

Merged
merged 32 commits into from
Jan 31, 2022

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Oct 10, 2021

Summary

In this change, we plan to integrate the ed25519 batch verification algorithm. This will speed up the process of verifying the digital signature of several transactions by roughly 2.4X.

This PR implements the algorithm according to the following paper:
https://eprint.iacr.org/2020/1244.pdf
This PR fixes a bug in the paper (Listing 1.2) that didn't reject the points (-0,-1) and (-0,1)

Another blog article with similar background: https://hdevalence.ca/blog/2020-10-04-its-25519am

Changes Overview
Changes made on the single ed255519 verification :

  1. Signatures that contain small order group R value will now be accepted.
  2. use the cofactor version of the equation to validate the signature. (multiple the result by the cofactor and compare to natural element)
  3. add ge25519_is_canonical_vartime function (check if a given y point has a canonical or non-canonical encoding ):
    • optimize this function using the the quick check.
    • reject two non-canonical points by comparison (points (-0,-1) and (-0,1), this resolves a bug in the paper).

Add the batch verification implementation from https://github.com/floodyberry/ed25519-donna to our fork of libsodium.
Changes made on the batch verification (to match the single verification):

  1. reject small order A (PK)
  2. reject non-canonical A (PK)
  3. reject non-canonical R
  4. reject non-canonical S
  5. use the cofactor equation

Test Plan

The following tests were added to the libsodium's test suite.

  1. Test against the White Paper's test vector
  2. Test against all non-canonical options for R
  3. Test against all non-canonical options for S

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2021

Codecov Report

Merging #3031 (c273bee) into master (93a372d) will increase coverage by 0.02%.
The diff coverage is 80.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3031      +/-   ##
==========================================
+ Coverage   47.57%   47.60%   +0.02%     
==========================================
  Files         370      370              
  Lines       60060    60109      +49     
==========================================
+ Hits        28572    28612      +40     
- Misses      28178    28186       +8     
- Partials     3310     3311       +1     
Impacted Files Coverage Δ
crypto/compactcert/builder.go 63.63% <0.00%> (ø)
crypto/compactcert/structs.go 100.00% <ø> (ø)
crypto/compactcert/verifier.go 66.66% <0.00%> (ø)
node/netprio.go 0.00% <0.00%> (ø)
data/transactions/verify/txn.go 44.44% <50.00%> (+0.14%) ⬆️
crypto/curve25519.go 61.01% <71.42%> (-0.81%) ⬇️
crypto/batchverifier.go 90.62% <90.69%> (-9.38%) ⬇️
agreement/vote.go 81.81% <100.00%> (ø)
config/consensus.go 84.69% <100.00%> (+0.05%) ⬆️
crypto/multisig.go 45.34% <100.00%> (ø)
... and 12 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 93a372d...c273bee. Read the comment docs.

@id-ms id-ms requested a review from cce October 13, 2021 12:19
@cce
Copy link
Contributor

cce commented Oct 21, 2021

I tried running make check in libsodium-fork and it failed on the "sign" test because the expected output file (sign.exp) no longer matches the output of the sign test program (with the additional messages it prints out), even though the tests themselves pass.

@id-ms id-ms force-pushed the add-batch-verification-algorithm branch from 0d714f2 to 8a7a4e3 Compare November 3, 2021 08:11
@id-ms
Copy link
Contributor Author

id-ms commented Nov 3, 2021

it looks like the batch verification interface uses unsigned long long *.
@cce @tsachiherman, do you think we should change it to size_t?

Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

generally LGTM, with some questions & suggestions for changes

@id-ms id-ms force-pushed the add-batch-verification-algorithm branch from b4c8f30 to c1c0b76 Compare November 12, 2021 04:15
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

it looks like the batch verification interface uses unsigned long long *. @cce @tsachiherman, do you think we should change it to size_t?

It looks like they use unsigned long long for message lengths throughout most of the libsodium API (and size_t for some memory limits and some hashing APIs) so LGTM to keep mlen as unsigned long long to match the rest of the crypto_sign API.

Also, I noticed it should be const unsigned long long * to match the strict const usage in the exported API functions for input args, submitted suggested changes

@id-ms id-ms force-pushed the add-batch-verification-algorithm branch from c1c0b76 to a70c996 Compare November 12, 2021 18:34
@cce
Copy link
Contributor

cce commented Jan 5, 2022

Would it be addressed with the UINT64_C macro?

@tsachiherman
Copy link
Contributor

Would it be addressed with the UINT64_C macro?

could be.. x86 is using little endian, arm is using big endian.
I suggest we test in both to make sure we're good.

@cce
Copy link
Contributor

cce commented Jan 5, 2022

ARM is little endian too ... we would need to use qemu or something to run on big endian

@cce
Copy link
Contributor

cce commented Jan 5, 2022

@algoidan you also have the U8TO64_LE macro you introduced elsewhere that converts from 8 unsigned chars to uint64

@id-ms id-ms force-pushed the add-batch-verification-algorithm branch from 96e5029 to a449a4b Compare January 6, 2022 16:21
@id-ms id-ms force-pushed the add-batch-verification-algorithm branch from a449a4b to 1ba1843 Compare January 6, 2022 17:10
tsachiherman
tsachiherman previously approved these changes Jan 6, 2022
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good to me.

@tsachiherman tsachiherman changed the title Add batch verification algorithm batch verification: add batch verification implementation Jan 31, 2022
@tsachiherman tsachiherman merged commit 1dd40a8 into algorand:master Jan 31, 2022
tsachiherman added a commit that referenced this pull request Feb 10, 2022
## Summary

Create an update path that would enable the following features:
1. Batch Verification (#3031)
2. State Proof Keys (#2990)
3. TEAL 6 (#3397)
4. Expired Participation Keys (#2924)
5. Fix rewards calculation bug (#3403)

## Test Plan

Unit tests added.
tsachiherman pushed a commit that referenced this pull request Mar 12, 2022
Summary
The ed25519 batch verification implementation in #3031 provides a performance improvement for validating multiple signatures (such as multiple transaction signatures). Since each OneTimeSignature used by agreement votes is actually 3 ed25519 signatures, this hooks up the verifier to the batch verification implementation, yielding a ~12% performance improvement in the included benchmark on my computer.

Test Plan
Added benchmark, existing tests should pass.
jannotti pushed a commit to jannotti/go-algorand that referenced this pull request Mar 13, 2022
…rand#3759)

Summary
The ed25519 batch verification implementation in algorand#3031 provides a performance improvement for validating multiple signatures (such as multiple transaction signatures). Since each OneTimeSignature used by agreement votes is actually 3 ed25519 signatures, this hooks up the verifier to the batch verification implementation, yielding a ~12% performance improvement in the included benchmark on my computer.

Test Plan
Added benchmark, existing tests should pass.
@cce cce changed the title batch verification: add batch verification implementation batch verification: add ed25519 batch verification implementation Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants