Finschia / ostracon

Ostracon, a consensus algorithm, is forked from Tendermint Core. We have added VRF to Tendermint BFT. It adds randomness to PoS Validator elections and improves security.
Apache License 2.0
70 stars 28 forks source link

Change to use curve25519-voi's VRF #633

Closed kokeshiM0chi closed 1 year ago

kokeshiM0chi commented 1 year ago

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.

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

zemyblue commented 1 year ago

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 commented 1 year ago

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.

kokeshiM0chi commented 1 year ago

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

codecov[bot] commented 1 year ago

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 ```diff @@ 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](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia) | Coverage Δ | | |---|---|---| | [crypto/crypto.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL2NyeXB0by5nbw==) | `0.00% <ø> (ø)` | | | [crypto/secp256k1/secp256k1.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL3NlY3AyNTZrMS9zZWNwMjU2azEuZ28=) | `69.56% <0.00%> (ø)` | | | [crypto/sr25519/pubkey.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL3NyMjU1MTkvcHVia2V5Lmdv) | `43.58% <0.00%> (ø)` | | | [state/execution.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-c3RhdGUvZXhlY3V0aW9uLmdv) | `66.58% <ø> (ø)` | | | [statesync/stateprovider.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-c3RhdGVzeW5jL3N0YXRlcHJvdmlkZXIuZ28=) | `31.20% <ø> (ø)` | | | [types/block.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-dHlwZXMvYmxvY2suZ28=) | `79.58% <ø> (ø)` | | | [types/validation.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-dHlwZXMvdmFsaWRhdGlvbi5nbw==) | `64.00% <ø> (ø)` | | | [crypto/ed25519/ed25519.go](https://app.codecov.io/gh/Finschia/ostracon/pull/633?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL2VkMjU1MTkvZWQyNTUxOS5nbw==) | `51.38% <100.00%> (+6.26%)` | :arrow_up: | ... and [17 files with indirect coverage changes](https://app.codecov.io/gh/Finschia/ostracon/pull/633/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia)
kokeshiM0chi commented 1 year ago

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