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

Allow ostracon to verify the old r2ishiguro vrf proofs #652

Merged
merged 14 commits into from
Jul 13, 2023

Conversation

0Tech
Copy link
Contributor

@0Tech 0Tech commented Jul 4, 2023

Description

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

  • the legacy r2ishiguro Verify() and ProofToHash().
  • the legacy r2ishiguro prove() INTO the test.

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

@0Tech 0Tech self-assigned this Jul 4, 2023
@CLAassistant
Copy link

CLAassistant commented Jul 4, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 4, 2023

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
@@            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 Coverage Δ
statesync/stateprovider.go 31.20% <0.00%> (ø)
crypto/ed25519/internal/r2ishiguro/vrf.go 82.35% <82.35%> (ø)
crypto/ed25519/migration.go 83.33% <83.33%> (ø)
crypto/ed25519/ed25519.go 54.16% <100.00%> (+2.77%) ⬆️
state/execution.go 66.58% <100.00%> (ø)
types/validation.go 52.63% <100.00%> (-11.37%) ⬇️

... and 14 files with indirect coverage changes

@ulbqb
Copy link
Member

ulbqb commented Jul 4, 2023

@0Tech
What about using the old ostracon?

@0Tech
Copy link
Contributor Author

0Tech commented Jul 4, 2023

@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.

@0Tech 0Tech marked this pull request as ready for review July 4, 2023 07:20
@0Tech 0Tech requested review from torao, tnasu and ulbqb as code owners July 4, 2023 07:20
@ulbqb ulbqb added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Jul 4, 2023
go.mod Outdated Show resolved Hide resolved
crypto/ed25519/migration.go Outdated Show resolved Hide resolved
crypto/ed25519/migration.go Outdated Show resolved Hide resolved
crypto/ed25519/migration.go Outdated Show resolved Hide resolved
@0Tech 0Tech requested a review from zemyblue July 5, 2023 05:06
@ulbqb
Copy link
Member

ulbqb commented Jul 5, 2023

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

@0Tech
Copy link
Contributor Author

0Tech commented Jul 5, 2023

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

@ulbqb
Here is the line:

state, err = h.replayBlock(state, storeBlockHeight, proxyApp.Consensus())

@ulbqb
Copy link
Member

ulbqb commented Jul 5, 2023

I am concerned about the following points.

@0Tech
Copy link
Contributor Author

0Tech commented Jul 6, 2023

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.

crypto/ed25519/migration.go Outdated Show resolved Hide resolved
@0Tech
Copy link
Contributor Author

0Tech commented Jul 6, 2023

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

@ulbqb
Copy link
Member

ulbqb commented Jul 7, 2023

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

@tnasu
Copy link
Member

tnasu commented Jul 7, 2023

@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.

@Kynea0b
Copy link
Contributor

Kynea0b commented Jul 7, 2023

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

@ulbqb
Copy link
Member

ulbqb commented Jul 7, 2023

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

@Kynea0b
Copy link
Contributor

Kynea0b commented Jul 7, 2023

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
Copy link
Member

ulbqb commented Jul 7, 2023

It seems better not to support libsodium.

ulbqb
ulbqb previously approved these changes Jul 7, 2023
@tnasu
Copy link
Member

tnasu commented Jul 10, 2023

@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.

tnasu
tnasu previously approved these changes Jul 10, 2023
@0Tech 0Tech dismissed stale reviews from tnasu and ulbqb via 8e5c1c9 July 10, 2023 05:39
@0Tech 0Tech requested review from tnasu and ulbqb July 10, 2023 05:39
@0Tech 0Tech marked this pull request as draft July 12, 2023 01:54
@0Tech 0Tech marked this pull request as ready for review July 13, 2023 01:37
@0Tech 0Tech merged commit e506c71 into Finschia:main Jul 13, 2023
@0Tech 0Tech deleted the migration branch July 13, 2023 02:17
0Tech added a commit that referenced this pull request Nov 15, 2023
* Add r2ishiguro implementation as the legacy

* Add tests

* Lint

* Chore

* Apply suggestions from code review

Modify the comments

* Add version control

* Refactor

* Fix test and refactor

* Update crypto/ed25519/migration.go

Remove old comment

* Fix race

* Remove backup file

* Use Finschia/r2ishiguro_vrf

* Use the tag for Finschia/r2ishiguro_vrf
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.

migration vrf library from r2ishiguro to curve25519-voi
6 participants