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

revert!: deprecate VDF crate #683

Merged
merged 4 commits into from
Sep 18, 2024
Merged

revert!: deprecate VDF crate #683

merged 4 commits into from
Sep 18, 2024

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Sep 13, 2024

closes: #649

This PR:

Deprecates the VDF crate because the original implemented MinRoot is insecure. Also we didn't and won't implement the verification part.

This PR does not:

Key places to review:


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@mrain mrain requested a review from alxiong September 13, 2024 17:03
@mrain mrain requested a review from ss-es September 13, 2024 17:09
fn setup<R: ark_std::rand::CryptoRng + ark_std::rand::RngCore>(
difficulty: u64,
prng: Option<&mut R>,
) -> Result<Self::PublicParameter, VDFError> {
Copy link

Choose a reason for hiding this comment

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

Is it possible to make setup and eval infallible (not return a Result)? It looks like they can't fail in either implementation

not really a big deal: we can of course always handle the failure in HotShot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep it. Error may occur when we are implementing Verifiable DF.

) -> Result<(Self::Output, Self::Proof), VDFError> {
let mut output = *input;
for _ in 0..pp.difficulty {
output = sha3::Keccak256::digest(&input).into();
Copy link

Choose a reason for hiding this comment

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

maybe I'm not reading this right, but should this be digest(&output)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stupid me! fixed in ffc603b

Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

  1. why are we keeping the minroot? afaik, no one, not even ourselves, is using it
  2. why sh3 hashchain? is it being used or planned anywhere?

@mrain
Copy link
Contributor Author

mrain commented Sep 16, 2024

  1. why are we keeping the minroot? afaik, no one, not even ourselves, is using it

No particular reason. Maybe we'll comeback to it sometime later (very not likely)

  1. why sh3 hashchain? is it being used or planned anywhere?

I just picked a random hash function. Do you have other suggestions or would you prefer a somewhat generic implementation cc @ss-es

@alxiong
Copy link
Contributor

alxiong commented Sep 16, 2024

i suggest we completely remove this. it's confusing already when we call it vdf without the verifiable property.

on the other hand, if we only need a delay function, do we really need to incorporate in jellyfish? a trivial hashchain implemented downstream to start with seems like a fine solution.

@mrain
Copy link
Contributor Author

mrain commented Sep 16, 2024

a trivial hashchain implemented downstream to start with seems like a fine solution.

That's a good idea. Since we don't have verification anyway and it's simple enough.

Shall we just completely remove this vdf crate?

@alxiong
Copy link
Contributor

alxiong commented Sep 18, 2024

Shall we just completely remove this vdf crate?

I'd agree.

@mrain mrain changed the title feat(vdf): deprecate minroot and add hashchain delay function BREAKING CHANGE: deprecate VDF crate Sep 18, 2024
@mrain mrain changed the title BREAKING CHANGE: deprecate VDF crate revert!: deprecate VDF crate Sep 18, 2024
@mrain mrain requested a review from alxiong September 18, 2024 13:24
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

I agree with the removal.

@mrain mrain merged commit 04200fb into main Sep 18, 2024
6 of 8 checks passed
@mrain mrain deleted the cl/df-fix branch September 18, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[security] MinRoot delay function is "vulnerable"
3 participants