-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor and WHIR Integration #20
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
Conversation
|
Thanks for all the work! Fixed a bug, and revived the markdown. Some thoughts on the refactoring:
|
|
Thanks for the review and the progress :) FRI-Regimes: I agree that it is a bit weird. In the long term, I think we should have proximity gaps / CA regimes and both WHIR and FRI VMs should call them to get their soundness errors. Right now, the reason why it is still called FRI-regime is that the functions that it provides are still FRI-specific, e.g., the name Structure: I think what you propose here makes sense to me. Btw, it was a bit tricky to avoid circular dependency between modules here, which is why the class |
a428e00 to
ef05fa4
Compare
asn-d6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial review
| @@ -0,0 +1,3 @@ | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, it would be good if we could unify soundcalc/proxgaps/ and soundcalc/regimes/, as they essentially do things related to proximity gaps.
| import math | ||
|
|
||
| @dataclass(frozen=True) | ||
| class WHIRBasedVMConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an equivalent of trace_length for WHIR VMs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 2 ** log_degree?
Co-authored-by: George Kadianakis <[email protected]>
This PR does a major refactoring of the codebase with the aim to allow integrating zkVMs that are based on WHIR.
Related to #19
Status