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

Add Zkverse Milestone1 deliver #835

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Add Zkverse Milestone1 deliver #835

merged 3 commits into from
Dec 12, 2023

Conversation

VegeBun-csj
Copy link
Contributor

@VegeBun-csj VegeBun-csj commented Apr 17, 2023

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#1563

@dsm-w3f dsm-w3f self-assigned this Apr 18, 2023
@dsm-w3f
Copy link
Contributor

dsm-w3f commented Apr 19, 2023

@VegeBun-csj thank you for the milestone submission. Please see the evaluation document and provide proper answers and fixes. After that, let me know to continue with the evaluation.

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj thank you for the milestone submission. Please see the evaluation document and provide proper answers and fixes. After that, let me know to continue with the evaluation.

Thanks for your review sir. I will fix it:)

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj thank you for the milestone submission. Please see the evaluation document and provide proper answers and fixes. After that, let me know to continue with the evaluation.

Hi sir. I have fixed the problem with latest repo in you evaludation with broken link, docker and code quality, thank you for reviewing again :)

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Apr 24, 2023

@VegeBun-csj thank you for the improvements and fixes. I tried docker again an it is still failing, however now with another error msg. Please see the evaluation document for details.

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj thank you for the improvements and fixes. I tried docker again an it is still failing, however now with another error msg. Please see the evaluation document for details.

Thank you for your review sir, i have fixed the docker-compose.yml, you can run it with latest substrate-zk and i think it will works. Looking for your review:)

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Apr 27, 2023

@VegeBun-csj thank you for the improvements and fixes. I was able to run with docker although some improvements could be applied in this part. See details in the evaluation document. We are almost finishing this evaluation. I asked a ZK specialist from W3F to check the concepts in the tutorial. Let's wait for the feedback.

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj thank you for the improvements and fixes. I was able to run with docker although some improvements could be applied in this part. See details in the evaluation document. We are almost finishing this evaluation. I asked a ZK specialist from W3F to check the concepts in the tutorial. Let's wait for the feedback.

thanks very much sir.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented May 4, 2023

@VegeBun-csj just for updating. Our ZK specialists are in high demand right now. This verification could take a little more time to complete.

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj just for updating. Our ZK specialists are in high demand right now. This verification could take a little more time to complete.

Okay. Looking forward to the review. Thanks very much.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented May 8, 2023

@VegeBun-csj we are ready to provide feedback regarding the tutorial provided. Can you post the .md file in a place where we can make comments directly in the text? I suggest hackmd (https://hackmd.io/). After that, provide me the links for the two parts of the tutorial that we gonna provide feedback to you. Thanks for patiently waiting for that.

@InaOana
Copy link

InaOana commented May 10, 2023

Dear @VegeBun-csj, thank you for your milestone delivery. I have been asked by the grants team to provide feedback related to your milestone. Just to briefly introduce myself, I am a cryptographer/mathematician and co-author of PLONK. I have read your submission; it would have been very useful if that contained documents that easily allowed us to give inline comments and feedback. Since those have not been provided, I am going to share a list of comments and it will be up to you to map them to the text/tutorials you shared with us.
Overall comment: I have tried to read the text both as a novice that knows nothing about zk and as a more seasoned zk contributor. Unfortunately, in both cases, the prose descriptions are confusing, contradictory at points and also factually incorrect or missing important references. Overall, the quality and clarity of the writing can be improved a lot. Even if tutorials are addressed to developers, I am sure they would appreciate a clearer (if necessary a shorter) and more informative text.

  • The text provides no link or reference to any external resource. It would help to add links to, for example, original zk paper, link to the original powers of tau paper and for the 2 phase filecoin trusted setup and another link, for contrast, to a trusted setup for a universal srs
  • You say "Non-interactivity: - the proof is a string"> I am sorry, I do not not understand. In any case, the proof is not a string but a sequence of field and elliptic curve points. And, even if you see that as a string, it's a string for both the interactive and non-interactive case. Please remove this explanation for non-interactivity and keep only what is in between ().
  • "Some theories mentioned above may be boring" -> by theories do you mean theoretical concepts? That has completely different meaning.
  • "You may not have heard of some steps, but it's not important, just take them step by step." Please, there should be a better way to say whatever you have in mind here.
  • "CRS is a fixed, publicly known parameter used to verify all zero-knowledge proofs." Not correct, one crs may help prove/verify one or multiple NP relations but certainly not all proofs or relations. See the blogposts I have shared above on how general a crs can be.
  • "The role of CRS in ZKSNARKs is crucial. It is generated by a trusted party and used to decrypt and decode the data in the proof to verify its validity. " --> Incorrect and unclear, crs may well be generated by via an MPC as per the blogposts above in which case it's not generated by a trusted party; I really do not understand the "decrypt and decode the data part"
  • "Therefore, ZKSNARKs typically employ techniques to reduce the size and cost of generating the CRS, such as using the hash value of the common reference string as the root node and storing the intermediate values of the proof using a Merkle tree to efficiently verify the correctness of the proof." I do not understand what you are saying here. I think you are confusing/mixing concepts of proof and crs.
  • "The ceremony was proposed by the core developers of zkSNARKs in 2017" --> I think you mean core developers of Zcash; please add references as required to the "Generate the CRS section"
  • "In ZKSNARK, the role of the arithmetic circuit is to transform a set of public inputs into a set of private outputs and verify whether these outputs satisfy specific conditions without revealing any information about the inputs. " --> Again unclear and incorrect; so far you have not explained what public inputs and witnesses are; you have not even mentioned witnesses I think; public inputs are not transformed into private outputs via arithmetisation but the predicates that define an NP relation are expressed into a low level language based on addition and multiplication operations.
  • After reading the paragraph on R1CS I cannot tell what R1CS is and when is used. Either some concrete details should be given (i.e., the definition of an R1CS constraint) or the text should be shortened and cleaned and links should be provided to more in depth and correct/complete tutorials.
  • The equations that define a QAP constraint are in contradiction. See Mapping from R1CS to QAP vs Verification.
    When and why a QAP is needed (with links to examples maybe) is still not clear.

Above are some examples of how to improve the quality and content of the prose/description.

I will also leave with you a list of reliable and good quality tutorials on topics very similar to yours. Please read them, update your description where necessary and give reference where due or when in-depth examples or concepts are missing a sufficient explanation in your presentation.

Thank you.

@VegeBun-csj
Copy link
Contributor Author

@InaOana Thank you for your suggestion sir. I'm very sorry, but some of my words were not very precise and there are some issues with my expression. I will strictly follow your requirements to make the necessary modifications.

@github-actions github-actions bot added the stale label May 25, 2023
@semuelle
Copy link
Member

semuelle commented Jun 1, 2023

@VegeBun-csj, please ping me here once you have made the changes so we can have another look.

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Jun 5, 2023

@semuelle @dsm-w3f @InaOana
Sorry for the late, we have improved the doc, If anything problem, i will make the necessary corrections promptly.

@semuelle semuelle requested a review from dsm-w3f June 15, 2023 13:10
@dsm-w3f
Copy link
Contributor

dsm-w3f commented Jun 15, 2023

@VegeBun-csj thank you for the improvements and fixes. I asked a W3F ZK specialist to help with this reevaluation. We should provide feedback soon, most probably next week.

@dsm-w3f dsm-w3f removed the stale label Jun 15, 2023
@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj thank you for the improvements and fixes. I asked a W3F ZK specialist to help with this reevaluation. We should provide feedback soon, most probably next week.

thanks very much!

@VegeBun-csj
Copy link
Contributor Author

Hi sir @dsm-w3f @drskalman , are there any updates on the review?

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Jun 30, 2023

@VegeBun-csj My apologies for taking a long time on this evaluation. Our researchers are in a high demand. Hope to hear from us in two weeks.

@VegeBun-csj
Copy link
Contributor Author

@VegeBun-csj My apologies for taking a long time on this evaluation. Our researchers are in a high demand. Hope to hear from us in two weeks.

Okay, looking forward to your reply, thanks very much!

Copy link
Contributor

@dsm-w3f dsm-w3f left a comment

Choose a reason for hiding this comment

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

@VegeBun-csj thank you for waiting for our feedback. @drskalman informed me that not all requests made by @InaOana were performed. Can you give us an overview of the changes that were performed and the ones that were not and the reasons for that? Please see Zkvers/substrate-zk#4.

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Jul 17, 2023

Hi,sir @dsm-w3f . I have indeed made modifications according to @InaOana's requirements before, may there be some areas where the modifications are not perfect enough? I can provide the records of the previous modifications in here.

@drskalman
Copy link
Contributor

Here for example seems the comment about CRS definition is completely ignored: https://github.com/Zkvers/substrate-zk/pull/4/files#diff-97ffb3af858b14f58c555ef4b2d2bfc1ff02abe3dace5878ae4f89b03003a7fcR42

@drskalman
Copy link
Contributor

@VegeBun-csj I rebased the PR please fix the remaining comments on the PR by means of github suggestions. It took me couple of hours to deal with the rebasing.

@VegeBun-csj
Copy link
Contributor Author

@drskalman @keeganquigley thanks bro, i will fix the left work quickly.

@VegeBun-csj
Copy link
Contributor Author

@drskalman hi sir. I have updated the doc with this commit and explain with some suggestions in this pr

@drskalman
Copy link
Contributor

drskalman commented Nov 17, 2023

@VegeBun-csj you could actually suggest deleting my comments when you believe it is addressed in the text. That way i can commit your suggestions and eventually we could merge the Pull request at the end.

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Nov 17, 2023

@VegeBun-csj you could actually suggest deleting my comments when you believe it is addressed in the text. That way i can commit your suggestions and eventually we could merge the Pull request at the end.

@drskalman due to the latest modification on my master branch which conflict your pr, i reset the latest commit to the old commit which is coordinate with your pr and the backup latest commit to the branch update-doc. Finally, i merged your pr and open another pr from update-doc, i will merge it into master. So all issues have been solved. Maybe need you review the latest tutorial again. if everything is ok, we can conclude this milstone?

@VegeBun-csj
Copy link
Contributor Author

@drskalman @keeganquigley hey bro. Are there any other questions about the latest modification? Looking forward to receiving your reply :)

@drskalman
Copy link
Contributor

@VegeBun-csj It would have been good if you would have requested my review on the PR before merging it. Anyway, I left my review on the merged PR few days ago. I'm not sure if you had the chance to see them.

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Nov 23, 2023

@VegeBun-csj It would have been good if you would have requested my review on the PR before merging it. Anyway, I left my review on the merged PR few days ago. I'm not sure if you had the chance to see them.

@drskalman Of course. I see them and I have optimized the document based on your advice. You can review the latest document for your audit. The PR part may conflict with my latest branch, which is why I did it that way. If there are still any issues, please let me know. Thanks!

@VegeBun-csj
Copy link
Contributor Author

@drskalman @keeganquigley Hey, you guys. Any progress or problems in this deliver?

@drskalman
Copy link
Contributor

As I mentioned I'm unable to re-read the whole document every time I make a suggestion if you could kindly make the correction on the PR, that would save me a lot of time searching the documents for the changes. Thanks a lot.

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Nov 29, 2023

@drskalman This is your pr. This is my commit and some comments about your suggestions. You can check it clearly.

Because you hadn't provided any specific feedback or suggestions on my previous submissions, I assumed you thought my changes were fine, so I went ahead and merged them. I'm really sorry. Are there any questions about this? Thanks!

@VegeBun-csj
Copy link
Contributor Author

@drskalman hi bro. Is there any progress in the current review? I have already placed the significant changes I made in the above reply. I apologize for making your job busier earlier. Looking forward to your response. Thank you.

@keeganquigley
Copy link
Contributor

keeganquigley commented Dec 12, 2023

Thanks @VegeBun-csj I think we can go ahead and merge this since the other PRs are now merged. Thanks for your work and glad we were able to see this wrapped up! The final evaluation is here.

@keeganquigley keeganquigley merged commit ff06ab9 into w3f:master Dec 12, 2023
Copy link

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

Copy link

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we've created a badge for projects that successfully deliver their first milestone. Please use the badge only in reference to the work that has been completed as part of this grant, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant. Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation's guidelines in doing so. If you haven't already, reach out to grantsPR@web3.foundation for feedback on your announcement and cross-promotion.

Thank you for your contribution, and good luck! If you have any remaining milestone, let us know if you encounter any delays by leaving a comment on the application PR or submitting an amendment.

@VegeBun-csj
Copy link
Contributor Author

@keeganquigley Hi sir. I have completed the final form, and I appreciate your approval and patient guidance very much.

@VegeBun-csj
Copy link
Contributor Author

@keeganquigley Hi sir, may I ask when the grant will be distributed? I have already filled out the form 3 weeks ago. Thanks

@semuelle
Copy link
Member

semuelle commented Jan 2, 2024

Hey @VegeBun-csj, sorry for the long wait. I sent an inquiry to our finance team, so you should hear back from one of us tomorrow.

@semuelle
Copy link
Member

semuelle commented Jan 2, 2024

@VegeBun-csj, there is an issue with the invoice. We cannot find the listed VAT registration number in the registry. VAT should only be added for Swiss-based companies. Could you verify that the information is correct and/or correct it and resubmit?

@semuelle
Copy link
Member

pinging @VegeBun-csj

@VegeBun-csj
Copy link
Contributor Author

VegeBun-csj commented Jan 14, 2024

@semuelle sorry bro. i missed and will resubmit the invoice today.

@semuelle
Copy link
Member

@VegeBun-csj, did you submit another invoice? I'm not seeing it.

@VegeBun-csj
Copy link
Contributor Author

@semuelle hi sir. i have changed the invoice and resubmited. Apologies for the delay in my previous message. I appreciate your review. Thank you very much. Any problems ping me. Thanks!

@VegeBun-csj
Copy link
Contributor Author

@semuelle hi sir. Is there any issue with the invoice?

@semuelle
Copy link
Member

Sorry for the delay, @VegeBun-csj. It should be paid this week.

@VegeBun-csj
Copy link
Contributor Author

@semuelle got it, thanks :)

@RouvenP
Copy link

RouvenP commented Jan 26, 2024

hi @VegeBun-csj we just sent the payment

@VegeBun-csj
Copy link
Contributor Author

hi @VegeBun-csj we just sent the payment

thanks a lot. Received, Good job :)

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.

None yet

7 participants