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

Allow ostracon to verify the old r2ishiguro vrf proofs #652

Closed 0Tech closed 1 year ago

0Tech commented 1 year ago

Description

This patch would allow ostracon to verify the old r2ishiguro vrf proofs. Basically, it would import:

It would also add the routing logic to minimize the changes. I have checked the patch in finschia in-place migration scenario.

Refer: #633 Closes: #653

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

codecov[bot] commented 1 year ago

Codecov Report

Merging #652 (4d97976) into main (5a2453e) will increase coverage by 0.02%. The diff coverage is 82.71%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #652 +/- ## ========================================== + Coverage 66.16% 66.18% +0.02% ========================================== Files 275 277 +2 Lines 36930 37001 +71 ========================================== + Hits 24433 24489 +56 - Misses 10733 10749 +16 + Partials 1764 1763 -1 ``` | [Impacted Files](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia) | Coverage Δ | | |---|---|---| | [statesync/stateprovider.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-c3RhdGVzeW5jL3N0YXRlcHJvdmlkZXIuZ28=) | `31.20% <0.00%> (ø)` | | | [crypto/ed25519/internal/r2ishiguro/vrf.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL2VkMjU1MTkvaW50ZXJuYWwvcjJpc2hpZ3Vyby92cmYuZ28=) | `82.35% <82.35%> (ø)` | | | [crypto/ed25519/migration.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL2VkMjU1MTkvbWlncmF0aW9uLmdv) | `83.33% <83.33%> (ø)` | | | [crypto/ed25519/ed25519.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-Y3J5cHRvL2VkMjU1MTkvZWQyNTUxOS5nbw==) | `54.16% <100.00%> (+2.77%)` | :arrow_up: | | [state/execution.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-c3RhdGUvZXhlY3V0aW9uLmdv) | `66.58% <100.00%> (ø)` | | | [types/validation.go](https://app.codecov.io/gh/Finschia/ostracon/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-dHlwZXMvdmFsaWRhdGlvbi5nbw==) | `52.63% <100.00%> (-11.37%)` | :arrow_down: | ... and [14 files with indirect coverage changes](https://app.codecov.io/gh/Finschia/ostracon/pull/652/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia)
ulbqb commented 1 year ago

@0Tech What about using the old ostracon?

0Tech commented 1 year ago

@0Tech What about using the old ostracon?

The new ostracon must be able to verify the old r2ishiguro proof, because the verification is required in the replay of the last block. I have confirmed the replay was required during in-place migration, and the migration failed without this patch.

ulbqb commented 1 year ago

@0Tech I would like to know where the last block is validated on in-place migration.

0Tech commented 1 year ago

@0Tech I would like to know where the last block is validated on in-place migration.

@ulbqb Here is the line: https://github.com/Finschia/ostracon/blob/5a2453efb1d32045a01afa8f490f7ae660886420/consensus/replay.go#L418

ulbqb commented 1 year ago

I am concerned about the following points.

0Tech commented 1 year ago

I am concerned about the following points.

* [This implementation](https://github.com/Finschia/ostracon/pull/652/files#diff-472d49584a011f21f3257f63be2718a5f462acb603b97be080a37cab81e8e7a9R66) means that r2ishiguro keep to be used until validator publish voi vrf.

That's the point. A node should keep using the old implementation before it applies a block with the new implementation. Also, "good" validators would not create a proof using the old implementation, which means from some point after the upgrade, the proofs would be of the new implementation, voi vrf.

0Tech commented 1 year ago

I've tested in-place migration again at this point.

ulbqb commented 1 year ago

@torao @tnasu I think libsodium should also be supported. What do you think?

tnasu commented 1 year ago

@ulbqb I don't think so. We know the validator with r2ishiguro on Finschia Network. So this PR is created. But, there is nothing to run the validator with libsodium. I think we don't need to support libsodium at that time.

kokeshiM0chi commented 1 year ago

There was also a bug recently in libsodium. The corresponding costs are not realistic. @ulbqb @tnasu

ulbqb commented 1 year ago

@kokeshiM0chi Does it mean that migrating from libsodium to voi is very difficult?

kokeshiM0chi commented 1 year ago

It is difficult to manage. Of course, the migration is also difficult. There is no merit to spend the cost here. Even Algorand's libsodium and libsodium have differences in the border case. @ulbqb

ulbqb commented 1 year ago

It seems better not to support libsodium.

tnasu commented 1 year ago

@ulbqb As a future correspondence, let's guide spec v3 libsodium as deprecated. Also, libsodium is not compatible between versions, so if there is a node that continues to use something that cannot be upgraded, it will be a problem in the future. r2ishiguro is spec v0, but I believe there is this PR for production Node. In addition, curve25519-voi is spec v7-v10 and v11+, but it is necessary to keep it in mind carefully including compatibility in future operation.