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

Change to use curve25519-voi's VRF #633

Merged
merged 10 commits into from
Jun 26, 2023
Merged

Conversation

Kynea0b
Copy link
Contributor

@Kynea0b Kynea0b commented Jun 10, 2023

Overview

The curve25519-voi ed25519 function is also referenced in cometbft/cometbft main or tendermint/tendermint master and is used for signing.
This library also implements VRF. Therefore, it is desirable to use this function for the VRF function.
This change removes libsodium, r2ishiguro, and all other unnecessary VRF libraries.

Ref:
https://github.com/oasisprotocol/curve25519-voi/blob/master/primitives/ed25519/extra/ecvrf/ecvrf.go

Advantages of using curve25519-voi

Briefly describe the advantages of using curve25519-voi's VRF.

  • Compatibility with Golang standard ed25519 library (Signature verification is conditional on adjustment of preset values)
  • It follows the cofactored verification equations required for signature verification in FIPS 186-5.
    Furthermore, by specifying VerifyOption, compatible with validation formulas according to FIPS 186-5, RFC 8032, and ZIP-215.
  • VRF verification also guarantees security based on standards equivalent to the level defined by NIST for signature verification in FIPS 186-5. This code is it!
  • Used for signature generation and verification on the Tendermint Core master branch or CometBFT main branch.
  • Supports wasm.
  • Assuming that calculations will be performed on VM, it reflects recent efforts to reduce calculation costs, and is actively being developed, and it is also quick to respond to vulnerabilities.
  • Vulnerability support is faster.
  • Easy to customize. This is because it is written in Golang and the implementation code is legible.

Background applied to master branch of Tendermint

Prior to this PR, for tendermint/tendermint master branch, hdevalence/ed25519consensus library was an implementation of ed25519 whose library was used for the signature scheme. This is also used in Ostracon. Tendermint seems to have applied curve25519-voi because of its improved batch processing algorithm, performance, and so on.

Refs

curve25519-voi is also refrenced here by cometBFT/cometBFT
https://github.com/cometbft/cometbft/blob/main/p2p/conn/secret_connection.go#L18

Closes: #620

Kynea0b added 2 commits June 10, 2023 12:17
`make build` passed.
The tendermint/tendermint master uses curve25519 at ed25519.go. Therefore, curve25519-voi should be used from the viewpoint of reducing implementation complexity and cost management. Also, curve25519-voi is based on ed25519-dalek and is more optimized than ref10 in cofactor processing.
@Kynea0b Kynea0b force-pushed the test-curve25519-voi branch 2 times, most recently from 6f40a0e to d07203f Compare June 11, 2023 01:30
@Kynea0b Kynea0b self-assigned this Jun 11, 2023
@Kynea0b Kynea0b added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Jun 11, 2023
@Kynea0b Kynea0b changed the title draft: Use curve25519-voi's vrf function Change to use curve25519-voi's vrf Jun 11, 2023
@Kynea0b Kynea0b changed the title Change to use curve25519-voi's vrf Change to use curve25519-voi's VRF Jun 11, 2023
@Kynea0b Kynea0b force-pushed the test-curve25519-voi branch from 29def21 to 217d922 Compare June 13, 2023 06:56
@zemyblue
Copy link
Member

I think this proposal is very great. The libsodium vrf was not modified in the very long times. And it has compile and linking issue because it is c++. So it is very good suggestion.

But migration should be considered since the vrf function is not compatible.
Let's check it.

@Yawning
Copy link
Contributor

Yawning commented Jun 15, 2023

Oh neat more people using my library. Ok, I'm glad people think the code is legible.

But migration should be considered since the vrf function is not compatible.

It is the same IETF draft, my implementation just significantly post-dates when algorand did theirs, so it was after the spec changes broke compatibility. My implementation provides v7-v10 and v11+, while your libsodium fork implements v3.

The incompatibility comes from all the changes made to v7 of the draft.

@Kynea0b
Copy link
Contributor Author

Kynea0b commented Jun 15, 2023

Thank you very much for your comment!!! The code is legible and the comments are polite and helpful. I really appreciate your explanation!!! @Yawning

@Kynea0b Kynea0b force-pushed the test-curve25519-voi branch 2 times, most recently from 50ad616 to 0bc1441 Compare June 25, 2023 06:31
@Kynea0b Kynea0b force-pushed the test-curve25519-voi branch from 0bc1441 to c975e06 Compare June 25, 2023 06:33
@Kynea0b Kynea0b marked this pull request as ready for review June 25, 2023 14:51
@Kynea0b Kynea0b requested review from torao, tnasu and ulbqb as code owners June 25, 2023 14:51
.github/workflows/coverage.yml Show resolved Hide resolved
test/kms/bench_test.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #633 (903db19) into main (06e3b94) will decrease coverage by 0.17%.
The diff coverage is 77.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
- Coverage   66.21%   66.04%   -0.17%     
==========================================
  Files         277      275       -2     
  Lines       36992    36930      -62     
==========================================
- Hits        24494    24391     -103     
- Misses      10725    10772      +47     
+ Partials     1773     1767       -6     
Impacted Files Coverage Δ
crypto/crypto.go 0.00% <ø> (ø)
crypto/secp256k1/secp256k1.go 69.56% <0.00%> (ø)
crypto/sr25519/pubkey.go 43.58% <0.00%> (ø)
state/execution.go 66.58% <ø> (ø)
statesync/stateprovider.go 31.20% <ø> (ø)
types/block.go 79.58% <ø> (ø)
types/validation.go 64.00% <ø> (ø)
crypto/ed25519/ed25519.go 51.38% <100.00%> (+6.26%) ⬆️

... and 17 files with indirect coverage changes

tnasu
tnasu previously approved these changes Jun 26, 2023
Copy link
Member

@ulbqb ulbqb left a comment

Choose a reason for hiding this comment

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

@Kynea0b
Copy link
Contributor Author

Kynea0b commented Jun 26, 2023

Thank you!! I fixed as pointed out. PTAL @ulbqb

README.md Outdated Show resolved Hide resolved
@ulbqb ulbqb self-requested a review June 26, 2023 07:50
ulbqb
ulbqb previously approved these changes Jun 26, 2023
Copy link
Member

@ulbqb ulbqb left a comment

Choose a reason for hiding this comment

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

LGTM

@ulbqb ulbqb self-requested a review June 26, 2023 09:00
@Kynea0b Kynea0b merged commit 4127ae7 into Finschia:main Jun 26, 2023
@Kynea0b Kynea0b deleted the test-curve25519-voi branch June 26, 2023 10:49
0Tech pushed a commit that referenced this pull request Nov 15, 2023
* draft: Use curve25519-voi's vrf function

`make build` passed.
The tendermint/tendermint master uses curve25519 at ed25519.go. Therefore, curve25519-voi should be used from the viewpoint of reducing implementation complexity and cost management. Also, curve25519-voi is based on ed25519-dalek and is more optimized than ref10 in cofactor processing.

* fix: Delete r2ishiguro and libsodium

* refactor: Mark unused parameters

* comment: fix the comment for `VRFVerify`

* fix: Leave the necessary jobs in coverage.yml and delete one unnecessary file

* fix: delete unnecessary line and describe length of proof for curve25519-voi

* typo

* fix: remove an unnecessary line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove r2ishiguro
5 participants